Conversation

PID namespacing breaks VCHIQ as it uses PIDs as unique identifiers for VCHIQ services, meaning messages sent from userland will contain a different ID than the one expected by the GPU.

This patch addresses the problem uses the existing GET_CLIENT_ID ioctl (via vchiq_get_client_id()) to retrieve the global PID as seen by the kernel rather than using the process's reported PID.

The main situation that causes this problem to occur is when trying to run code that interfaces with the GPU inside of a container (e.g. via EGL.)

I'm not certain this is the best means of achieving this aim, however I definitely think the fix belongs in the userland tools, as the kernel alternative is to either use the namespaced PID, which opens the door to conflicts (see raspberrypi/linux#1279) or to actively intercept messages and manually determine which part of the raw data contains the ID and replace it, which is potentially unreliable and adds overhead (see raspberrypi/linux#1382 - yes we've looked at this multiple different ways :)

I've tested this change against EGL applications and it works correctly with no noticeable issues, though of course it's hard to test all cases :)

I'd really love to get some feedback on this in case there is something I've missed or got wrong here. My main concerns in the approach I've taken here are:

Correctness - Am I missing something here that renders this an invalid approach?

Completeness - There are other parts of the code that reference PID, but khronos_platform_get_process_id() seems to be the main source of the PID namespacing issue. Perhaps more code needs adjustment?

Performance - this patch causes an ioctl to be issued to determine the client ID when previously a getpid() was used, potentially introducing performance degradation. However, it does seem to only be needed at points where a message would be sent anyway so perhaps relatively this isn't that much overhead. This can potentially be mitigated by vchiq_get_client_id() using a cached client ID value instead of issuing the ioctl itself.

PID namespacing breaks VCHIQ as it uses PIDs as unique identifiers for VCHIQ
services, meaning messages sent from userland will contain a different ID than
the one expected by the GPU.
This patch addresses the problem uses the existing GET_CLIENT_ID ioctl to
retrieve the global PID as seen by the kernel rather than using the process's
reported PID.

This comment has been minimized.

This comment has been minimized.

I'm happy with the motivation behind the patch, and I think the implementation is correct. My hesitation has been on the grounds of a potential performance hit, but that doesn't seem to be a problem; if anything it seems marginally quicker:

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.