userns: detect initial namespace by inode - #240
Conversation
3a4e7f4 to
372f6db
Compare
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
-
I think that
TestInodeInUserNS(and a separatefunc inodeInUserNS) makes little sense now when the code is very simple and clean. I mean, we just do stat(2) and comparest.Ino-- this needs no unit tests. -
The problem is, this (predefined inode number for
PROC_USER_INIT_INO) only appears in kernel v3.8 (it comes from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=98f842e675f9), and since Go itself (since Go 1.24) requires kernel v3.2 (see https://go.dev/wiki/MinimumRequirements#linuxlinux), we do need a fallback to old code. It will still work since most people run linux >= v3.8, and no one sane will run systemd v260+ on an older kernel.
Note this package (github.com/moby/sys/userns) only requires Go 1.18 which itself has a lower linux kernel version requirement. Meaning, we definitely need the fallback. |
372f6db to
41c4209
Compare
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
See my comments above
41c4209 to
54934d6
Compare
cool... I went with "if /proc/self/ns/user exists use that one, if not, continue the old path" ... this should make the code more robust on new kernels, and , like you said, on on old kernels it matter less .. |
OK, the Two notes though:
|
54934d6 to
a8eaedb
Compare
| if err := syscall.Stat("/proc/self/ns/user", &st); err == nil { | ||
| return st.Ino != procUserInitIno | ||
| } else if !os.IsNotExist(err) { | ||
| return false |
There was a problem hiding this comment.
This should still fall back to the legacy method?
There was a problem hiding this comment.
This should still fall back to the legacy method?
I can do this... but i think this might be the wrong way of doing it... consider thisL If the file "/proc/self/ns/user" is there, it means a modern kernel, so if stat fails for any other reason than "file does not exists" it mens that you are still on a modern kernel but something unexpected is wrong with /proc, permissions, mount state, or namespace access, its safer to assume that you are not in the init namespace than to fallback in the old criteria (specially if you plan to remove this line once older kernel are not supported) ...
There was a problem hiding this comment.
I agree with this logic; the only thing i'm wondering is; do we need a comment here to describe the logic?
Not a blocker, just putting it on the table; a future visitor of the code may think we either accidentally fall through, or return too early.
|
@AkihiroSuda @kolyshkin may i bug you with one more review. ppllleeeeeeasseee? 😃 |
There was a problem hiding this comment.
LGTM
left a comment #240 (comment), but not a blocker; @kolyshkin can you PTAL?
The current uid_map heuristic treats a private user namespace with an identity uid_map as the initial user namespace. systemd 260 can create that shape with PrivateUsers=full, causing callers to believe they can perform initial-namespace-only operations such as cgroup device BPF setup. Use /proc/self/ns/user's inode instead and compare it with the kernel's PROC_USER_INIT_INO value. This detects the initial user namespace directly instead of inferring it from the uid_map layout. Related: containers/crun#2150 Signed-off-by: Alvaro Leiva Geisse <aleivag@gmail.com>
a8eaedb to
023b764
Compare
@thaJeztah updated with the comment... @kolyshkin its up to you now! 😄 |
4 participants
The current uid_map heuristic treats a private user namespace with an identity uid_map as the initial user namespace. systemd 260 can create that shape with PrivateUsers=full, causing callers to believe they can perform initial-namespace-only operations such as cgroup device BPF setup.
Use /proc/self/ns/user's inode instead and compare it with the kernel's PROC_USER_INIT_INO value. This detects the initial user namespace directly instead of inferring it from the uid_map layout.
Related: #239