Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:

On Thu, Dec 09, 2021 at 02:38:13PM -0500, James Bottomley wrote:

[...]

quoted

@@ -317,21 +315,15 @@ EXPORT_SYMBOL_GPL(securityfs_create_symlink);
 void securityfs_remove(struct dentry *dentry)
 {
 	struct user_namespace *ns = dentry->d_sb->s_user_ns;
-	struct inode *dir;
 
 	if (!dentry || IS_ERR(dentry))
 		return;
 
-	dir = d_inode(dentry->d_parent);
-	inode_lock(dir);
 	if (simple_positive(dentry)) {
-		if (d_is_dir(dentry))
-			simple_rmdir(dir, dentry);
-		else
-			simple_unlink(dir, dentry);
+		d_delete(dentry);

Not, that doesn't work. You can't just call d_delete() and dput() and
even if I wouldn't advise it. And you also can't do this without
taking the inode lock on the directory.

Agreed on that

simple_rmdir()/simple_unlink() take care to update various inode
fields in the parent dir and handle link counts. This really wants to
be sm like

	struct inode *parent_inode;

	parent_inode = d_inode(dentry->d_parent);
	inode_lock(parent_inode);
	if (simple_positive(dentry)) {
		dget(dentry);
		if (d_is_dir(dentry)
			simple_unlink(parent_inode, dentry);
		else
			simple_unlink(parent_inode, dentry);
		d_delete(dentry);
		dput(dentry);
	}
	inode_unlock(parent_inode);

It just slightly annoys me how the simple_ functions change fields in
an inode that is about to be evicted ... it seems redundant; plus we
shouldn't care if the object we're deleting is a directory or file.  I
also don't think we need the additional dget because the only consumer
is policy file removal and the opener of that file will have done a
dget.  The inode lock now prevents us racing with another remove in the
case of two simultaneous writes.

How about

        struct inode *parent_inode;

        parent_inode = d_inode(dentry->d_parent);
        inode_lock(parent_inode);
        if (simple_positive(dentry)) {
		drop_nlink(parent_inode);
                d_delete(dentry);
                dput(dentry);
        }
        inode_unlock(parent_inode);

James