On Mon, Feb 23, 2026 at 8:26 AM Chris Mason <[email protected]> wrote: > NeilBrown <[email protected]> wrote: > > From: NeilBrown <[email protected]> > > > > 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 <[email protected]> > > 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 <[email protected]> > Acked-by: Paul Moore <[email protected]> > > > 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. -- paul-moore.com
