[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 12 Dec 2021 09:13:12 -0500
From: James Bottomley <jejb@...ux.ibm.com>
To: Christian Brauner <christian.brauner@...ntu.com>
Cc: Stefan Berger <stefanb@...ux.ibm.com>,
linux-integrity@...r.kernel.org, zohar@...ux.ibm.com,
serge@...lyn.com, containers@...ts.linux.dev,
dmitry.kasatkin@...il.com, ebiederm@...ssion.com,
krzysztof.struczynski@...wei.com, roberto.sassu@...wei.com,
mpeters@...hat.com, lhinds@...hat.com, lsturman@...hat.com,
puiterwi@...hat.com, jamjoom@...ibm.com,
linux-kernel@...r.kernel.org, paul@...l-moore.com, rgb@...hat.com,
linux-security-module@...r.kernel.org, jmorris@...ei.org
Subject: 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:
[...]
> > @@ -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
Powered by blists - more mailing lists