Message-ID: <e72104c480c2c7f5c29f80b72d2a597a50ef9fae.camel@linux.ibm.com>
Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace
From:   James Bottomley <jejb@linux.ibm.com>
Reply-To: jejb@linux.ibm.com
To:     Christian Brauner <christian.brauner@ubuntu.com>
Cc:     Stefan Berger <stefanb@linux.ibm.com>,
        linux-integrity@vger.kernel.org, zohar@linux.ibm.com,
        serge@hallyn.com, containers@lists.linux.dev,
        dmitry.kasatkin@gmail.com, ebiederm@xmission.com,
        krzysztof.struczynski@huawei.com, roberto.sassu@huawei.com,
        mpeters@redhat.com, lhinds@redhat.com, lsturman@redhat.com,
        puiterwi@redhat.com, jamjoom@us.ibm.com,
        linux-kernel@vger.kernel.org, paul@paul-moore.com, rgb@redhat.com,
        linux-security-module@vger.kernel.org, jmorris@namei.org
Date:   Sun, 12 Dec 2021 09:13:12 -0500
In-Reply-To: <20211210114934.tacjnwryihrsx6ln@wittgenstein>
References: <20211208221818.1519628-1-stefanb@linux.ibm.com>
         <20211208221818.1519628-16-stefanb@linux.ibm.com>
         <20211209143428.ip6bwry5hqtee5vy@wittgenstein>
         <20211209143749.wk4agkynfqdzftbl@wittgenstein>
         <fb99af21f029b8072435e35731b919f4ec98f89d.camel@linux.ibm.com>
         <e2feaf2f6ac4bc82f328f94ca35d14cdc3ca79d1.camel@linux.ibm.com>
         <20211210114934.tacjnwryihrsx6ln@wittgenstein>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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:
[...]
> > @@ -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