Re: [PATCH 31/39] convert selinuxfs

Re: [PATCH 31/39] convert selinuxfs

[Al Viro]

On Sun, Sep 21, 2025 at 04:44:28PM -0400, Paul Moore wrote:
> > +       dput(dentry);
> > +       return dentry;  // borrowed
> >  }
> 
> Prefer C style comments on their own line:
> 
>   dput(dentry);
>   /* borrowed dentry */
>   return dentry;

Umm...  IMO that's more of an annotation along the lines of "fallthru"...

> > @@ -2079,15 +2088,14 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc)
> >                 goto err;
> >         }
> >
> > -       fsi->policycap_dir = sel_make_dir(sb->s_root, POLICYCAP_DIR_NAME,
> > +       dentry = sel_make_dir(sb->s_root, POLICYCAP_DIR_NAME,
> >                                           &fsi->last_ino);
> 
> I'd probably keep fsi->policycap_dir in this patch simply to limit the
> scope of this patch to just the DCACHE_PERSISTENT related changes, but
> I'm not going to make a big fuss about that.

Not hard to split that way.  Will do...


BTW, an unrelated question: does userland care about selinuxfs /null being
called that (and being on selinuxfs, for that matter)?  Same for the
apparmor's securityfs /apparmor/.null...

If nobody cares, I would rather add an internal-only filesystem with
root being a character device (1,3) and whatever markings selinux et.al.
need for it.  With open_devnull(creds) provided for selinux,
apparmor and whoever else wants to play with neutering descriptors
on exec...

Re: [PATCH 31/39] convert selinuxfs

[Paul Moore]

On Sun, Sep 21, 2025 at 5:41 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Sep 21, 2025 at 04:44:28PM -0400, Paul Moore wrote:
> > > +       dput(dentry);
> > > +       return dentry;  // borrowed
> > >  }
> >
> > Prefer C style comments on their own line:
> >
> >   dput(dentry);
> >   /* borrowed dentry */
> >   return dentry;
>
> Umm...  IMO that's more of an annotation along the lines of "fallthru"...

Maybe, I still prefer the example provided above.  The heart wants
what the heart wants I guess.

> > > @@ -2079,15 +2088,14 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc)
> > >                 goto err;
> > >         }
> > >
> > > -       fsi->policycap_dir = sel_make_dir(sb->s_root, POLICYCAP_DIR_NAME,
> > > +       dentry = sel_make_dir(sb->s_root, POLICYCAP_DIR_NAME,
> > >                                           &fsi->last_ino);
> >
> > I'd probably keep fsi->policycap_dir in this patch simply to limit the
> > scope of this patch to just the DCACHE_PERSISTENT related changes, but
> > I'm not going to make a big fuss about that.
>
> Not hard to split that way.  Will do...

Thanks.

> BTW, an unrelated question: does userland care about selinuxfs /null being
> called that (and being on selinuxfs, for that matter)?  Same for the
> apparmor's securityfs /apparmor/.null...

That's an interesting question.  The kernel really only references it
in one place after creation, and as you've already seen, that's easily
changed.  It's more important that it can be uniquely labeled such
that most any process can open it, otherwise we run into problems when
trying to replace fds when another file that the process can't open.

I'm adding the SELinux list to tap into the folks that play with
userland more than I do, but off the top of my head I can't think of
why userspace would need to do anything directly with
/sys/fs/selinux/null.  There are some comments in the userland code
about not being able to mount selinuxfs with MS_NODEV due to the null
device, but that's the only obvious thing I see while quickly
searching through the code tonight.

> If nobody cares, I would rather add an internal-only filesystem with
> root being a character device (1,3) and whatever markings selinux et.al.
> need for it.  With open_devnull(creds) provided for selinux,
> apparmor and whoever else wants to play with neutering descriptors
> on exec...

With the ongoing efforts to push towards better support for multiple
simultaneous LSMs, we would likely need to make sure there each LSM
that currently has their own null device would continue to have their
own, otherwise we potentially run into permission conflicts between
LSMs where one could end up blocking another and then we're back to
not having a file to use as a replacement.  Not sure if that is what
you had in mind with your proposal, but just wanted to make sure that
was factored into the idea.

-- 
paul-moore.com

Re: [PATCH 31/39] convert selinuxfs

[Stephen Smalley]

On Sun, Sep 21, 2025 at 10:45 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Sun, Sep 21, 2025 at 5:41 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Sun, Sep 21, 2025 at 04:44:28PM -0400, Paul Moore wrote:
> > > > +       dput(dentry);
> > > > +       return dentry;  // borrowed
> > > >  }
> > >
> > > Prefer C style comments on their own line:
> > >
> > >   dput(dentry);
> > >   /* borrowed dentry */
> > >   return dentry;
> >
> > Umm...  IMO that's more of an annotation along the lines of "fallthru"...
>
> Maybe, I still prefer the example provided above.  The heart wants
> what the heart wants I guess.
>
> > > > @@ -2079,15 +2088,14 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc)
> > > >                 goto err;
> > > >         }
> > > >
> > > > -       fsi->policycap_dir = sel_make_dir(sb->s_root, POLICYCAP_DIR_NAME,
> > > > +       dentry = sel_make_dir(sb->s_root, POLICYCAP_DIR_NAME,
> > > >                                           &fsi->last_ino);
> > >
> > > I'd probably keep fsi->policycap_dir in this patch simply to limit the
> > > scope of this patch to just the DCACHE_PERSISTENT related changes, but
> > > I'm not going to make a big fuss about that.
> >
> > Not hard to split that way.  Will do...
>
> Thanks.
>
> > BTW, an unrelated question: does userland care about selinuxfs /null being
> > called that (and being on selinuxfs, for that matter)?  Same for the
> > apparmor's securityfs /apparmor/.null...
>
> That's an interesting question.  The kernel really only references it
> in one place after creation, and as you've already seen, that's easily
> changed.  It's more important that it can be uniquely labeled such
> that most any process can open it, otherwise we run into problems when
> trying to replace fds when another file that the process can't open.
>
> I'm adding the SELinux list to tap into the folks that play with
> userland more than I do, but off the top of my head I can't think of
> why userspace would need to do anything directly with
> /sys/fs/selinux/null.  There are some comments in the userland code
> about not being able to mount selinuxfs with MS_NODEV due to the null
> device, but that's the only obvious thing I see while quickly
> searching through the code tonight.

Is there a reason why these patches weren't sent to selinux list in
the first place?
In any event, yes, Android userspace (in particular the Android init
program) relies on /sys/fs/selinux/null at a point where /dev/null
does not yet exist [1]. Hence, I don't believe we can drop it since it
would break userspace.

[1] https://cs.android.com/search?q=%2Fsys%2Ffs%2Fselinux%2Fnull&sq=&ss=android%2Fplatform%2Fsuperproject%2Fmain


>
> > If nobody cares, I would rather add an internal-only filesystem with
> > root being a character device (1,3) and whatever markings selinux et.al.
> > need for it.  With open_devnull(creds) provided for selinux,
> > apparmor and whoever else wants to play with neutering descriptors
> > on exec...
>
> With the ongoing efforts to push towards better support for multiple
> simultaneous LSMs, we would likely need to make sure there each LSM
> that currently has their own null device would continue to have their
> own, otherwise we potentially run into permission conflicts between
> LSMs where one could end up blocking another and then we're back to
> not having a file to use as a replacement.  Not sure if that is what
> you had in mind with your proposal, but just wanted to make sure that
> was factored into the idea.
>
> --
> paul-moore.com
>

Re: [PATCH 31/39] convert selinuxfs

[Al Viro]

On Mon, Sep 22, 2025 at 08:34:02AM -0400, Stephen Smalley wrote:

> Is there a reason why these patches weren't sent to selinux list in
> the first place?

Will Cc on the next posting of that series.

> In any event, yes, Android userspace (in particular the Android init
> program) relies on /sys/fs/selinux/null at a point where /dev/null
> does not yet exist [1]. Hence, I don't believe we can drop it since it
> would break userspace.

Pity.  Oh, well...

Re: [PATCH 31/39] convert selinuxfs

[John Johansen]

On 9/21/25 14:41, Al Viro wrote:
> On Sun, Sep 21, 2025 at 04:44:28PM -0400, Paul Moore wrote:
>>> +       dput(dentry);
>>> +       return dentry;  // borrowed
>>>   }
>>
>> Prefer C style comments on their own line:
>>
>>    dput(dentry);
>>    /* borrowed dentry */
>>    return dentry;
> 
> Umm...  IMO that's more of an annotation along the lines of "fallthru"...
> 
>>> @@ -2079,15 +2088,14 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc)
>>>                  goto err;
>>>          }
>>>
>>> -       fsi->policycap_dir = sel_make_dir(sb->s_root, POLICYCAP_DIR_NAME,
>>> +       dentry = sel_make_dir(sb->s_root, POLICYCAP_DIR_NAME,
>>>                                            &fsi->last_ino);
>>
>> I'd probably keep fsi->policycap_dir in this patch simply to limit the
>> scope of this patch to just the DCACHE_PERSISTENT related changes, but
>> I'm not going to make a big fuss about that.
> 
> Not hard to split that way.  Will do...
> 
> 
> BTW, an unrelated question: does userland care about selinuxfs /null being
> called that (and being on selinuxfs, for that matter)?  Same for the
> apparmor's securityfs /apparmor/.null...
> 

For apparmor the userspace doesn't care, ideally userspace wouldn't even
see it exists.

> If nobody cares, I would rather add an internal-only filesystem with
> root being a character device (1,3) and whatever markings selinux et.al.
> need for it.  With open_devnull(creds) provided for selinux,
> apparmor and whoever else wants to play with neutering descriptors
> on exec...