VCHIQ breaking with namespaced PIDs (e.g. inside containers)

[lorenzo-stoakes]

Introduction

We have encountered an issue with VCHIQ via a simple EGL application which fails to operate correctly in a namespaced process hierarchy, which results in EGL applications not functioning correctly inside of a container.

This can easily be repro'd by attempting to run the firmware project's hello triangle example inside a container/in the host OS direct, or if you don't want to use containers, via @petrosagg 's example repro code which simply creates a pid namespace and attempts to invoke the same example.

Typically, the application shows a frame or 2 then freezes, with vcdbg log msg outputting messages like:

025819.942: *** No KHAN handle found for pid 24
025836.615: *** No KHAN handle found for pid 24
025853.285: *** No KHAN handle found for pid 24

This message can be traced back to code contained inside the proprietary start.elf binary (and is perhaps coming from the GPU itself.)

When run outside of a namespaced process, the application functions entirely correctly.

Previous reports

We've previously reported this over at the firmware project, along with a PR which does a major hack to work around the problem - associating a namespaced pid with a VCHIQ instance rather than a global pid, but is not a good solution as it potentially results in collisions.

Work-in-progress patch

diff vs. rpi-4.1.y

Important: This is an spelunky proof-of-concept RFC patch, see below for more details.

Description

We've spent some time working on a solution to this problem and have a rather rough patch in development which takes a significantly saner approach:

1. When messages are queued by userspace via a VCHIQ_IOC_QUEUE_MESSAGE ioctl (in vchiq_ioctl), we manually alter the elements of the message to replace the namespaced PID with a global PID for the GPU to consume.
2. When the kernel calls back into userland (in make_service_callback), and a PID is included in the header, we manually alter that header to replace the global PID with the namespaced PID.
3. When a bulk request specifies a service client ID (which is equivalent to a PID in the VCHIQ code), we replace the namespaced PID with a global PID.
4. When instance information is dumped from /dev/vchiq, the code now does not dump data on instances with PIDs with a different namespace and shows the namespaced PID for each instance, this avoids collisions and confusing information for userland.
5. One fly in the ointment, which is an issue even if no code is changed, is that the debugfs entries at /sys/kernel/debug/vchiq/clients/<pid> are added with global PIDs rather than namespaced PIDs. If we replace them with namespaced PIDs we can have collisions, if we don't namespaced userland won't know what to do with these. For the time being we've left this, as leaving it as-is sidesteps collisions. In theory it should be possible to change the layout of this directory for different PID namespaces.

With these changes, all internal kernel data structures and messages sent to the GPU reference global PID, no collisions are possible and everything on that side works as if there wasn't a namespacing restraint at all, while as far as userland is concerned it is receiving messages with valid namespaced PIDs.

Spelunking

Currently, our proof-of-concept code is dirtily spelunking into void * arrays and taking a guess at where to change to values, which I should point out is clearly not what we are proposing long-term here. Additionally there are probably some details in this code which are flakey/wrong, at the moment it's a really rough experiment (ok enough caveats :)

However, I am sure that by using what we know about the data structures passed as messages we can correctly identify what needs to be changed and when without needing to guess anything. Advice on this would be useful too if you feel the approach isn't completely insane.

We'd like your input on this before we proceed further in case we are going down the wrong path, you have major objections to the approach or you have some input on this. If you feel another route should be taken do let us know, as we are eager to enable EGL applications (and VCHIQ clients in general) to work correctly inside containers (we = resin.io, who rather love containers :)

[pelwell]

I think your general approach - only use global PIDs when communicating with the GPU - is the correct one. However, VCHIQ should never inspect or modify the content of application-defined messages, and there is no need for it to - all of the ARM-side code is open source, either in the kernel repo or the userland repo.

Your 0x4008 is probably EGLINTMAKECURRENT_ID, but there are almost certainly other places that will need changing. You may get away with just replacing all calls to getpid() with vcos_process_id_current, and giving it an implementation that returns the global PID. If that information is only available to the kernel then I would rather add another VCHIQ ioctl or dedicated driver to return it.

[lorenzo-stoakes]

Hi @pelwell, thanks very much for your feedback.

On the kernel side, the issue with not modifying these messages is that PID-namespaced userland processes do not and cannot (to my knowledge) determine their own global PID, and we are therefore unable to do the translation in any of the userland code.

I am also not certain about leaking global PID to userland, I think there might be security issues with this (the code is already doing this via debugfs and dumps of output, but it doesn't mean it's the correct behaviour), in this case we'd in effect be exposing an ioctl which returns global PID for any process that wants it.

I worry as well about changing code which purports to return the 'current process ID' but instead returns what is to the namespaced application a meaningless ID, it's pretty confusing. An associated issue with this is probably not hugely likely but some custom user code which uses VCHIQ direct could end up broken (but hey, it's already broken so :)

A suggestion - it might be worth disassociating 'client ID' from global or virtual PID altogether, and adjust userland code accordingly. If possible we could have this check (and any other like it) compare stored state of the ID against the message's ID.

This is effectively equivalent to your suggestion however rather than providing global PID we're providing an arbitrary ID. This could potentially be done without a new ioctl assuming userland correctly stores its associated ID somewhere for comparison purposes.

One issue with this is that it'd be a breaking change - but only with an old userland, new kernel since new userland, old kernel would use global PID as the internally stored ID and and comparing it against what is returned would carry on working fine.

This is all assuming that the GPU code doesn't actually need to know the actual PID of a userland process, which I think is a safe assumption as I've got things working with the GPU being told both virtual and global PIDs with no change in behaviour, it's more a case of the ids needing to match in messages on both sides.

If a breaking change is too big a deal and exposing global PID isn't an issue, then adding an ioctl and replacing any PID check (maybe with some function name alterations for clarity) with comparison of the global PID will work too.

[pelwell]

I can assure you that the GPU doesn't care what the PIDs mean - they are just client IDs.

Inventing a whole new global ID when a perfectly good one exists doesn't make sense to me. It sounds like a new VCHIQ ioctl and vchiq_lib helper function are what is needed.

[lorenzo-stoakes]

Okay great will get cracking on a solution using global PID @pelwell

[lorenzo-stoakes]

@pelwell I've been able to achieve this entirely with userland changes, the GET_CLIENT_ID ioctl seems sufficient for this purpose, so I think this issue can be closed here.