Re: [apparmor] [PATCH v2 06/15] selinux: Use simple_start_creating() / simple_done_creating()

[NeilBrown]

On Tue, 24 Feb 2026, Paul Moore wrote:
> On Mon, Feb 23, 2026 at 8:26 AM Chris Mason <c...@meta.com> wrote:
> > NeilBrown <ne...@ownmail.net> wrote:
> > > From: NeilBrown <n...@brown.name>
> > >
> > > Instead of explicitly locking the parent and performing a lookup in
> > > selinux, use simple_start_creating(), and then use
> > > simple_done_creating() to unlock.
> > >
> >
> > Hi everyone,
> >
> > Amir suggested I run these through, and this commit was flagged:
> >
> > commit bc981629767980bbaee2ca3c7348b5a648665a9d
> > Author: NeilBrown <n...@brown.name>
> >
> > selinux: Use simple_start_creating() / simple_done_creating()
> >
> > Instead of explicitly locking the parent and performing a lookup in
> > selinux, use simple_start_creating(), and then use
> > simple_done_creating() to unlock.
> >
> > This extends the region that the directory is locked for, and also
> > performs a lookup. The lock extension is of no real consequence.
> > The lookup uses simple_lookup() and so always succeeds. Thus when
> > d_make_persistent() is called the dentry will already be hashed.
> > d_make_persistent() handles this case.
> >
> > Reviewed-by: Jeff Layton <jlay...@kernel.org>
> > Acked-by: Paul Moore <p...@paul-moore.com>
> >
> > > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > > index 3245cc531555b..7d4f90e5b12a0 100644
> > > --- a/security/selinux/selinuxfs.c
> > > +++ b/security/selinux/selinuxfs.c
> > > @@ -1931,15 +1931,16 @@ static const struct inode_operations 
> > > swapover_dir_inode_operations = {
> > >  static struct dentry *sel_make_swapover_dir(struct super_block *sb,
> > >                                               unsigned long *ino)
> > >  {
> > > -     struct dentry *dentry = d_alloc_name(sb->s_root, ".swapover");
> > > +     struct dentry *dentry;
> > >       struct inode *inode;
> > >
> > > -     if (!dentry)
> > > +     inode = sel_make_inode(sb, S_IFDIR);
> > > +     if (!inode)
> > >               return ERR_PTR(-ENOMEM);
> > >
> > > -     inode = sel_make_inode(sb, S_IFDIR);
> > > -     if (!inode) {
> > > -             dput(dentry);
> > > +     dentry = simple_start_creating(sb->s_root, ".swapover");
> > > +     if (!dentry) {
> > > +             iput(inode);
> > >               return ERR_PTR(-ENOMEM);
> > >       }
> >
> > Can this cause a kernel crash when simple_start_creating() fails?
> >
> > The function simple_start_creating() returns ERR_PTR on failure, not
> > NULL. The check `if (!dentry)` only catches NULL values. When
> > simple_start_creating() fails and returns an ERR_PTR value like
> > ERR_PTR(-ENOMEM), this value is non-NULL and the error check is bypassed.
> >
> > Execution would continue to d_make_persistent(dentry, inode), which
> > attempts to lock dentry->d_lock, dereferencing the ERR_PTR value and
> > causing a kernel crash.
> >
> > The old code used d_alloc_name() which returns NULL on error. The new
> > code uses simple_start_creating() which returns ERR_PTR on error, but
> > the error checking pattern wasn't updated to match.
> >
> > Should the check be `if (IS_ERR(dentry))` instead, and return dentry
> > rather than ERR_PTR(-ENOMEM) to preserve the actual error code?
> 
> Good catch Chris, yes, please change this Neil and feel free to
> preserve my ACK.

Thanks.
I've made it

        dentry = simple_start_creating(sb->s_root, ".swapover");
        if (IS_ERR(dentry)) {
                iput(inode);
                return dentry;
        }

NeilBrown