Re: [PATCH v9 4/5] proc: Skip the visibility check if subset=pid is used

Christian Brauner
2026-04-22T12:46:32Z

On Tue, Apr 21, 2026 at 02:24:08PM +0200, Alexey Gladkov wrote:
> On Tue, Apr 21, 2026 at 01:51:47PM +0200, Christian Brauner wrote:
> > On Fri, Apr 17, 2026 at 01:03:46AM +1000, Aleksa Sarai wrote:
> > > On 2026-04-16, Christian Brauner <brauner@kernel.org> wrote:
> > > > On Thu, Apr 16, 2026 at 10:46:50PM +1000, Aleksa Sarai wrote:
> > > > > On 2026-04-16, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > > > > On 2026-04-13, Alexey Gladkov <legion@kernel.org> wrote:
> > > > > > > When procfs is mounted with the subset=pid option, all system files and
> > > > > > > directories from the root of the filesystem are not accessible in
> > > > > > > userspace. Only dynamic information about processes is available, which
> > > > > > > cannot be hidden with overmount.
> > > > > > >
> > > > > > > For this reason, checking for full visibility is not relevant if mounting
> > > > > > > is performed with the subset=pid option.
> > > > > > >
> > > > > > > Signed-off-by: Alexey Gladkov <legion@kernel.org>
> > > > > > > ---
> > > > > >
> > > > > > > -static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags)
> > > > > > > +static bool mount_too_revealing(struct fs_context *fc, int *new_mnt_flags)
> > > > > > > {
> > > > > > > const unsigned long required_iflags = SB_I_NOEXEC | SB_I_NODEV;
> > > > > > > struct mnt_namespace *ns = current->nsproxy->mnt_ns;
> > > > > > > + const struct super_block *sb = fc->root->d_sb;
> > > > > > > unsigned long s_iflags;
> > > > > > >
> > > > > > > if (ns->user_ns == &init_user_ns)
> > > > > > > @@ -6388,7 +6387,7 @@ static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags
> > > > > > > return true;
> > > > > > > }
> > > > > > >
> > > > > > > - return !mnt_already_visible(ns, sb, new_mnt_flags);
> > > > > > > + return (!fc->skip_visibility && !mnt_already_visible(ns, sb, new_mnt_flags));
> > > > > > > }
> > > > > >
> > > > > > Unless I'm missing something (I haven't tested this locally yet, sorry),
> > > > > > this will allow you to bypass mount_too_revealing() even for
> > > > > > non-subset=pid mounts because once you create a subset=pid mount then a
> > > > > > regular procfs mount will see the subset=pid mount and permit it.
> > > > > >
> > > > > > I think the solution is quite simple -- you can also skip super-blocks
> > > > > > that have fc->skip_visibility set in mnt_already_visible().
> > > > >
> > > > > I now see that check was present in v8 but I guess its importance wasn't
> > > > > obvious. I guess this means we will need to reintroduce
> > > > > SB_I_USERNS_ALLOW_REVEALING. :/
> > > >
> > > > I've been playing with something else. So first we should move
> > > > SB_I_USERNS_REVEALING to an fs_type flag. It's not an optional thing and
> > > > always set and never removed. That also means we can simplify
> > > > sysfs_get_tree() to just kernfs_get_tree().
> > >
> > > Seems quite reasonable to me.
> > >
> > > > And then we raise SB_I_USERNS_RESTRICTED on all procfs mounts with
> > > > pid_only and disallow using them for calculating mount permissions for
> > > > unrestricted procfs mounts.
> > >
> > > I think that's fine here and is probably all we need for now (though I
> > > think that the name is a little confusing especially given my next
> > > comment), but I think there is a bit of a deeper problem here that
> > > deserves to be mentioned if only for posterity.
> > >
> > > The core issue really is that we actually have two versions of procfs
> > > now, and they are effectively distinguished by fs_context flags instead
> > > of fs_type. Back when subset=pid was merged there was a discussion about
> > > a hypothetical proc2 -- while that got dropped, we are kind of
> > > reinventing this split in an ad-hoc way.
> > >
> > > Conceptually considering them as two distinct fs_types would be the more
> > > semantically correct thing to do, though I think it would be overkill to
> > > come up with some framework for that. (I also really hope we don't have
> > > any other filesystems where this is also the case.)
> >
> > Doing this is almost trivial. The problem really is that we'd expose
> > "procfs2" or whatever as a new mount option to userspace without
> > actually having fundamentally revamped what we would like procfs2 to do.
>
> Agree.
>
> The new procfs2 should not have the issues present in procfs.
>
> > IOW, it would elevate a security hack that got added a while ago to a
> > new filesystem type. I think that would just be sad. So I'm not so
> > excited about doing this.
> >
> > > This is why I think the name is a little weird, since it isn't an issue
> > > about one procfs being restricted in a userns, it's that they are
> > > conceptually unrelated filesystem types. A very lightweight version of
> >
> > I somewhat disagree because it's unclear where you draw the line. If you
> > use the hidepid= stuff at its most restrictive where it only shows tasks
> > your user owns you could also argue that it's very close to a separate
> > filesystem type. It's strictly subtractive and so is pidonly. There's
> > just nothing that's better or novel.
> >
> > > this would be making it something like SB_I_RESTRICTED_VARIANT that
> > > would be more a little more strict than what you have now --
> > > SB_I_RESTRICTED_VARIANT would not be treated as compatible with anything
> > > (even another SB_I_RESTRICTED_VARIANT mount) so that if we add more
> > > variants in the future we don't treat them the same either. (Again, I
> > > don't think we need this now?) Of course this would still allow
> > > subset=pid without a procfs mount because of the other special casing
> > > for it in mount_too_revealing().
> >
> > It's not a problem today. I think we can simply treat
> > SB_I_RESTRICTED_VARIANT as not allowing other restricted variants. And
> > we'd need a more elaborate mechanism anyway once we'd have "compatible"
> > subsets.
> >
> > > We are also limited in what semantics we can change here too (and I'm a
> > > little worried that even this bit for SB_I_USERNS_RESTRICTED is at risk
> > > of breaking something) so maybe that isn't needed today.
> >
> > So I think yes, SB_I_RESTRICTED_VARIANT is fine by me for now.
>
> Should I create a new version and add this to my patchset?

Yes, please!