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? > > @@ -1947,11 +1948,9 @@ static struct dentry *sel_make_swapover_dir(struct > super_block *sb, > inode->i_ino = ++(*ino); > /* directory inodes start off with i_nlink == 2 (for "." entry) */ > inc_nlink(inode); > - inode_lock(sb->s_root->d_inode); > d_make_persistent(dentry, inode); > inc_nlink(sb->s_root->d_inode); > - inode_unlock(sb->s_root->d_inode); > - dput(dentry); > + simple_done_creating(dentry); > return dentry; // borrowed > }
