Container: Check for monitor process candidate's NSpid value (from Incus)
#16047
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
f212c52 to
b0c0669
Compare
… in devlxdFindContainerForPID This function checks the candidate monitor PID process's NSpid status variable to ensure it is not in any other PID namespaces (and thus can't be the monitor process). Inspired by lxc/incus@25cc63e Inspired-by: Stéphane Graber <stgraber@stgraber.org> Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com> License: Apache-2.0
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
There was a problem hiding this comment.
Pull Request Overview
This PR enhances process identification for /dev/lxd access by verifying that candidate monitor processes have matching PID namespaces (via NSpid) before associating them with containers.
- Replaced
loadContainerFromLXCMonitorPIDwithgetLXCMonitorContainer, which checks bothPPidandNSpidin/proc/<pid>/status, and only loads the container if the process is in the correct namespace. - Refactored
devlxdFindContainerForPIDto usegetLXCMonitorContainerduring process tree traversal and cleaned up legacy namespace-checking code.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lxd/daemon.go | Updated seccomp handler to use getLXCMonitorContainer and treat a nil container as non-error. |
| lxd/api_devlxd.go | Added getLXCMonitorContainer with namespace checks, removed old loader, and refactored PID lookup. |
Comments suppressed due to low confidence (1)
lxd/api_devlxd.go:164
- Consider adding unit tests for
getLXCMonitorContainerto cover scenarios where theNSpidmatches or does not match the PID, and when the command-line prefix is missing, ensuring the new logic behaves as expected.
// getLXCMonitorContainer returns the container instance for the candidateMonitorPID, but only if:
| } | ||
|
|
||
| // Fallback to loading all containers and checking their PID namespaces. | ||
| // This is the approach used when the process isn't actually a descendant of the container's init process, | ||
| // such as when using `lxc exec`. |
There was a problem hiding this comment.
How are the following lines related to the container's init process, isn't instance.LoadNodeAll just running a SQL query? Probably I cannot follow the comment here.
There was a problem hiding this comment.
Ah, should the comment rather be on the loop below?
There was a problem hiding this comment.
No, the comment is in the right place.
Its say that we use this fallback approach when the origin process of the devlxd request isn't actually a descendant of the container's init process (which is what happens when accessing /dev/lxd from a shell launched with lxc exec).
There was a problem hiding this comment.
Ah I see it's just a header for the entire block. Thanks.
When a process inside a container accesses
/dev/lxdLXD will now check theNSpidstatus variable of each candidate monitor process to confirm the process has no PIDs in other namespaces. If it does then it can't be the LXC monitor process, even if it has "[lxc monitor]" in its process name.Inspired by ideas from lxc/incus#2285