| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <175313904724.2234665.16342190171473611948@noble.neil.brown.name>
Date: Tue, 22 Jul 2025 09:04:07 +1000
From: "NeilBrown" <neil@...wn.name>
To: "Jeff Layton" <jlayton@...nel.org>
Cc: "Linus Torvalds" <torvalds@...ux-foundation.org>,
"Alexander Viro" <viro@...iv.linux.org.uk>,
"Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/7] VFS: introduce done_dentry_lookup()
On Mon, 21 Jul 2025, Jeff Layton wrote:
> On Mon, 2025-07-21 at 17:59 +1000, NeilBrown wrote:
> > done_dentry_lookup() is the first step in introducing a new API for
> > locked operation on names in directories - those that create or remove
> > names. Rename operations will also be part of this API but will
> > use separate interfaces.
> >
> > The plan is to lock just the dentry (or dentries), not the whole
> > directory. A "dentry_lookup()" operation will perform the locking and
> > lookup with a corresponding "done_dentry_lookup()" releasing the
> > resulting dentry and dropping any locks.
> >
> > This done_dentry_lookup() can immediately be used to complete updates
> > started with kern_path_locked() (much as done_path_create() already
> > completes operations started with kern_path_create()).
> >
> > So this patch adds done_dentry_lookup() and uses it where
> > kern_path_locked() is used. It also adds done_dentry_lookup_return()
> > which returns a reference to the dentry rather than dropping it. This
> > is a less common need in existing code, but still worth its own interface.
> >
> > Signed-off-by: NeilBrown <neil@...wn.name>
> > ---
> > drivers/base/devtmpfs.c | 7 ++-----
> > fs/bcachefs/fs-ioctl.c | 3 +--
> > fs/namei.c | 38 ++++++++++++++++++++++++++++++++++++++
> > include/linux/namei.h | 3 +++
> > kernel/audit_fsnotify.c | 9 ++++-----
> > kernel/audit_watch.c | 3 +--
> > 6 files changed, 49 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
> > index 31bfb3194b4c..47bee8166c8d 100644
> > --- a/drivers/base/devtmpfs.c
> > +++ b/drivers/base/devtmpfs.c
> > @@ -265,8 +265,7 @@ static int dev_rmdir(const char *name)
> > else
> > err = -EPERM;
> >
> > - dput(dentry);
> > - inode_unlock(d_inode(parent.dentry));
> > + done_dentry_lookup(dentry);
> > path_put(&parent);
> > return err;
> > }
> > @@ -349,9 +348,7 @@ static int handle_remove(const char *nodename, struct device *dev)
> > if (!err || err == -ENOENT)
> > deleted = 1;
> > }
> > - dput(dentry);
> > - inode_unlock(d_inode(parent.dentry));
> > -
> > + done_dentry_lookup(dentry);
> > path_put(&parent);
> > if (deleted && strchr(nodename, '/'))
> > delete_path(nodename);
> > diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c
> > index 4e72e654da96..8077ddf4ddc4 100644
> > --- a/fs/bcachefs/fs-ioctl.c
> > +++ b/fs/bcachefs/fs-ioctl.c
> > @@ -351,8 +351,7 @@ static long bch2_ioctl_subvolume_destroy(struct bch_fs *c, struct file *filp,
> > d_invalidate(victim);
> > }
> > err:
> > - inode_unlock(dir);
> > - dput(victim);
> > + done_dentry_lookup(victim);
> > path_put(&path);
> > return ret;
> > }
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 1c80445693d4..da160a01e23d 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -1714,6 +1714,44 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> > }
> > EXPORT_SYMBOL(lookup_one_qstr_excl);
> >
> > +/**
> > + * done_dentry_lookup - finish a lookup used for create/delete
> > + * @dentry: the target dentry
> > + *
> > + * After locking the directory and lookup or validating a dentry
> > + * an attempt can be made to create (including link) or remove (including
> > + * rmdir) a dentry. After this, done_dentry_lookup() can be used to both
> > + * unlock the parent directory and dput() the dentry.
> > + *
> > + * This interface allows a smooth transition from parent-dir based
> > + * locking to dentry based locking.
> > + */
> > +void done_dentry_lookup(struct dentry *dentry)
> > +{
> > + inode_unlock(dentry->d_parent->d_inode);
> > + dput(dentry);
> > +}
> > +EXPORT_SYMBOL(done_dentry_lookup);
> > +
> > +/**
> > + * done_dentry_lookup_return - finish a lookup sequence, returning the dentry
> > + * @dentry: the target dentry
> > + *
> > + * After locking the directory and lookup or validating a dentry
> > + * an attempt can be made to create (including link) or remove (including
> > + * rmdir) a dentry. After this, done_dentry_lookup_return() can be used to
> > + * unlock the parent directory. The dentry is returned for further use.
> > + *
> > + * This interface allows a smooth transition from parent-dir based
> > + * locking to dentry based locking.
> > + */
> > +struct dentry *done_dentry_lookup_return(struct dentry *dentry)
> > +{
> > + inode_unlock(dentry->d_parent->d_inode);
> > + return dentry;
> > +}
> > +EXPORT_SYMBOL(done_dentry_lookup_return);
> > +
> > /**
> > * lookup_fast - do fast lockless (but racy) lookup of a dentry
> > * @nd: current nameidata
> > diff --git a/include/linux/namei.h b/include/linux/namei.h
> > index 5d085428e471..e097f11587c9 100644
> > --- a/include/linux/namei.h
> > +++ b/include/linux/namei.h
> > @@ -81,6 +81,9 @@ struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
> > struct qstr *name,
> > struct dentry *base);
> >
> > +void done_dentry_lookup(struct dentry *dentry);
> > +struct dentry *done_dentry_lookup_return(struct dentry *dentry);
> > +
> > extern int follow_down_one(struct path *);
> > extern int follow_down(struct path *path, unsigned int flags);
> > extern int follow_up(struct path *);
> > diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> > index c565fbf66ac8..170836c3520f 100644
> > --- a/kernel/audit_fsnotify.c
> > +++ b/kernel/audit_fsnotify.c
> > @@ -85,8 +85,8 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pa
> > dentry = kern_path_locked(pathname, &path);
> > if (IS_ERR(dentry))
> > return ERR_CAST(dentry); /* returning an error */
> > - inode = path.dentry->d_inode;
> > - inode_unlock(inode);
> > + inode = igrab(dentry->d_inode);
>
> This is a little confusing. This patch changes "inode" from pointing to
> the parent inode to point to the child inode instead. That actually
> makes a bit more sense given the naming, but might best be done in a
> separate patch.
I could at least explain it properly the commit description.
This is part of my struggle with done_dentry_lookup_return(). Maybe I
should just use that and preserve the current use of dentry here....
Or maybe a separate patch as you say. I'll ponder it.
>
> > + done_dentry_lookup(dentry);
> >
> > audit_mark = kzalloc(sizeof(*audit_mark), GFP_KERNEL);
> > if (unlikely(!audit_mark)) {
> > @@ -97,17 +97,16 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pa
> > fsnotify_init_mark(&audit_mark->mark, audit_fsnotify_group);
> > audit_mark->mark.mask = AUDIT_FS_EVENTS;
> > audit_mark->path = pathname;
> > - audit_update_mark(audit_mark, dentry->d_inode);
> > + audit_update_mark(audit_mark, inode);
> > audit_mark->rule = krule;
> >
> > - ret = fsnotify_add_inode_mark(&audit_mark->mark, inode, 0);
> > + ret = fsnotify_add_inode_mark(&audit_mark->mark, path.dentry->d_inode, 0);
> > if (ret < 0) {
> > audit_mark->path = NULL;
> > fsnotify_put_mark(&audit_mark->mark);
> > audit_mark = ERR_PTR(ret);
> > }
> > out:
> > - dput(dentry);
> > path_put(&path);
> > return audit_mark;
> > }
> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > index 0ebbbe37a60f..f6ecac2109d4 100644
> > --- a/kernel/audit_watch.c
> > +++ b/kernel/audit_watch.c
> > @@ -359,8 +359,7 @@ static int audit_get_nd(struct audit_watch *watch, struct path *parent)
> > watch->ino = d_backing_inode(d)->i_ino;
> > }
> >
> > - inode_unlock(d_backing_inode(parent->dentry));
> > - dput(d);
> > + done_dentry_lookup(d);
> > return 0;
> > }
> >
>
> It looks right to me though.
>
> Reviewed-by: Jeff Layton <jlayton@...nel.org>
>
Thanks,
NeilBrown
Powered by blists - more mailing lists