On Wed, Jan 25, 2023 at 03:36:28PM -0800, Andrew Morton wrote: > On Wed, 25 Jan 2023 16:28:47 +0100 Alexey Gladkov <legion@xxxxxxxxxx> wrote: > > > The patch expands subset= option. If the proc is mounted with the > > subset=allowlist option, the /proc/allowlist file will appear. This file > > contains the filenames and directories that are allowed for this > > mountpoint. By default, /proc/allowlist contains only its own name. > > Changing the allowlist is possible as long as it is present in the > > allowlist itself. > > > > This allowlist is applied in lookup/readdir so files that will create > > modules after mounting will not be visible. > > > > Compared to the previous patches [1][2], I switched to a special virtual > > file from listing filenames in the mount options. > > > > Changlog doesn't explain why you think Linux needs this feature. The > [2/6] changelog hints that containers might be involved. IOW, please > fully describe the requirement and use-case(s). > > Also, please describe why /proc/allowlist is made available via a mount > option, rather than being permanently present. > > And why add to subset=, instead of a separate mount option. > > Does /proc/allowlist work in subdirectories? Like, permit presence of > /proc/sys/vm/compact_memory? > > I think the whole thing is misnamed, really. "allowlist" implies > access permissions. Some of the test here uses "visibility" and other > places use "presence", which are better. "presentlist" and > /proc/presentlist might be better. But why not simply /proc/contents? Currently, a lot of container runtimes - even if they mount a new procfs instance - overmount various procfs files and directories to ensure that they're hidden from the container workload. (The motivations for this are mixed and usually it's only needed for containers that run with the same privilege level as the host.) The consequence of overmounting is that we need to refuse mounting procfs again somewhere else otherwise the procfs instance might reveal files and directories that were supposed to be hidden. So this patchset moves the ability to hide entries into the kernel through an allowlist. This way you can hide files and directories while being able to mount procfs again because it will inherit the same allowlist. I get the motivation. The question is whether this belongs into the kernel at all. I'm unfortunately not convinced. This adds a lot of string parsing to procfs and I think we would also need to decide what a reasonable maximum limit for such allowlists would be. The data structure likely shouldn't be a linked list but at least an rbtree especially if the size isn't limited. But fundamentally I think it moves something that should be and currently is a userspace policy into the kernel which I think is wrong. Sure you can't predict what files show up in procfs over time but then subset=pid is already your friend - even if not as fine-grained. If this where another simple subset style mount option that allowlists a bunch of well-known global proc files then sure. But making this dynamically configurable from userspace doesn't make sense to me. I mean, users could write /gobble/dy/gook into /proc/allowlist or use it to stash secrets or hashes or whatever as we have no way of figuring out whether the entry they allowlist does or will actually ever exist. In general, such flexibility belongs into userspace imho. Frankly, if that is really required it would almost make more sense to be able to attach a new bpf program type to procfs that would allow to filter procfs entries. Then the filter could be done purely in userspace. If signed bpf lands one could then even ship signed programs that are attachable by userns root.
