| 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: <fb2cf7c5-cced-4ea3-bf5a-a442a0e64bda@oracle.com>
Date: Wed, 13 Dec 2023 15:40:33 +0100
From: Vegard Nossum <vegard.nossum@...cle.com>
To: Alexander Viro <viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-doc@...r.kernel.org, Nicholas Piggin <npiggin@...il.com>,
Waiman Long <longman@...hat.com>
Subject: Re: [PATCH v2] dcache: remove unnecessary NULL check in dget_dlock()
[Fixed up a couple of bad addresses in Cc]
Hi,
I didn't get a response to this v2 of the patch below and I don't see it
in vfs.git.
Was there something wrong or is it just awaiting review? Is there
anything I can do or help with? I would be happy to try to review other
patches if there is anything outstanding.
Thanks,
Vegard
On 06/11/2023 14:44, Vegard Nossum wrote:
> dget_dlock() requires dentry->d_lock to be held when called, yet
> contains a NULL check for dentry.
>
> An audit of all calls to dget_dlock() shows that it is never called
> with a NULL pointer (as spin_lock()/spin_unlock() would crash in these
> cases):
>
> $ git grep -W '\<dget_dlock\>'
>
> arch/powerpc/platforms/cell/spufs/inode.c- spin_lock(&dentry->d_lock);
> arch/powerpc/platforms/cell/spufs/inode.c- if (simple_positive(dentry)) {
> arch/powerpc/platforms/cell/spufs/inode.c: dget_dlock(dentry);
>
> fs/autofs/expire.c- spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
> fs/autofs/expire.c- if (simple_positive(child)) {
> fs/autofs/expire.c: dget_dlock(child);
>
> fs/autofs/root.c: dget_dlock(active);
> fs/autofs/root.c- spin_unlock(&active->d_lock);
>
> fs/autofs/root.c: dget_dlock(expiring);
> fs/autofs/root.c- spin_unlock(&expiring->d_lock);
>
> fs/ceph/dir.c- if (!spin_trylock(&dentry->d_lock))
> fs/ceph/dir.c- continue;
> [...]
> fs/ceph/dir.c: dget_dlock(dentry);
>
> fs/ceph/mds_client.c- spin_lock(&alias->d_lock);
> [...]
> fs/ceph/mds_client.c: dn = dget_dlock(alias);
>
> fs/configfs/inode.c- spin_lock(&dentry->d_lock);
> fs/configfs/inode.c- if (simple_positive(dentry)) {
> fs/configfs/inode.c: dget_dlock(dentry);
>
> fs/libfs.c: found = dget_dlock(d);
> fs/libfs.c- spin_unlock(&d->d_lock);
>
> fs/libfs.c: found = dget_dlock(child);
> fs/libfs.c- spin_unlock(&child->d_lock);
>
> fs/libfs.c: child = dget_dlock(d);
> fs/libfs.c- spin_unlock(&d->d_lock);
>
> fs/ocfs2/dcache.c: dget_dlock(dentry);
> fs/ocfs2/dcache.c- spin_unlock(&dentry->d_lock);
>
> include/linux/dcache.h:static inline struct dentry *dget_dlock(struct dentry *dentry)
>
> After taking out the NULL check, dget_dlock() becomes almost identical
> to __dget_dlock(); the only difference is that dget_dlock() returns the
> dentry that was passed in. These are static inline helpers, so we can
> rely on the compiler to discard unused return values. We can therefore
> also remove __dget_dlock() and replace calls to it by dget_dlock().
>
> Also fix up and improve the kerneldoc comments while we're at it.
>
> Al Viro pointed out that we can also clean up some of the callers to
> make use of the returned value and provided a bit more info for the
> kerneldoc.
>
> While preparing v2 I also noticed that the tabs used in the kerneldoc
> comments were causing the kerneldoc to get parsed incorrectly so I also
> fixed this up (including for d_unhashed, which is otherwise unrelated).
>
> Testing: x86 defconfig build + boot; make htmldocs for the kerneldoc
> warning. objdump shows there are code generation changes.
>
> Link: https://lore.kernel.org/all/20231022164520.915013-1-vegard.nossum@oracle.com/
> Cc: Alexander Viro <viro@...iv.linux.org.uk>
> Cc: Christian Brauner <brauner@...nel.org>
> Cc: linux-fsdevel@...r.kernel.org
> Cc: Nick Piggin <npiggin@...nel.dk>
> Cc: Waiman Long <Waiman.Long@...com>
> Cc: linux-doc@...r.kernel.org
> Signed-off-by: Vegard Nossum <vegard.nossum@...cle.com>
> ---
> fs/dcache.c | 16 ++++------------
> include/linux/dcache.h | 29 +++++++++++++++++++----------
> 2 files changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index c82ae731df9a..4bf33ba588d8 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -942,12 +942,6 @@ void dput_to_list(struct dentry *dentry, struct list_head *list)
> spin_unlock(&dentry->d_lock);
> }
>
> -/* This must be called with d_lock held */
> -static inline void __dget_dlock(struct dentry *dentry)
> -{
> - dentry->d_lockref.count++;
> -}
> -
> static inline void __dget(struct dentry *dentry)
> {
> lockref_get(&dentry->d_lockref);
> @@ -1034,7 +1028,7 @@ static struct dentry *__d_find_alias(struct inode *inode)
> hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
> spin_lock(&alias->d_lock);
> if (!d_unhashed(alias)) {
> - __dget_dlock(alias);
> + dget_dlock(alias);
> spin_unlock(&alias->d_lock);
> return alias;
> }
> @@ -1707,8 +1701,7 @@ static enum d_walk_ret find_submount(void *_data, struct dentry *dentry)
> {
> struct dentry **victim = _data;
> if (d_mountpoint(dentry)) {
> - __dget_dlock(dentry);
> - *victim = dentry;
> + *victim = dget_dlock(dentry);
> return D_WALK_QUIT;
> }
> return D_WALK_CONTINUE;
> @@ -1853,8 +1846,7 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name)
> * don't need child lock because it is not subject
> * to concurrency here
> */
> - __dget_dlock(parent);
> - dentry->d_parent = parent;
> + dentry->d_parent = dget_dlock(parent);
> list_add(&dentry->d_child, &parent->d_subdirs);
> spin_unlock(&parent->d_lock);
>
> @@ -2851,7 +2843,7 @@ struct dentry *d_exact_alias(struct dentry *entry, struct inode *inode)
> spin_unlock(&alias->d_lock);
> alias = NULL;
> } else {
> - __dget_dlock(alias);
> + dget_dlock(alias);
> __d_rehash(alias);
> spin_unlock(&alias->d_lock);
> }
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 3da2f0545d5d..82127cf10992 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -301,20 +301,29 @@ extern char *dentry_path(const struct dentry *, char *, int);
> /* Allocation counts.. */
>
> /**
> - * dget, dget_dlock - get a reference to a dentry
> - * @dentry: dentry to get a reference to
> + * dget_dlock - get a reference to a dentry
> + * @dentry: dentry to get a reference to
> *
> - * Given a dentry or %NULL pointer increment the reference count
> - * if appropriate and return the dentry. A dentry will not be
> - * destroyed when it has references.
> + * Given a live dentry, increment the reference count and return
> + * the dentry. For a dentry to be live, it can be hashed, positive,
> + * or have a non-negative &dentry->d_lockref.count
> + *
> + * Context: @dentry->d_lock must be held.
> */
> static inline struct dentry *dget_dlock(struct dentry *dentry)
> {
> - if (dentry)
> - dentry->d_lockref.count++;
> + dentry->d_lockref.count++;
> return dentry;
> }
>
> +/**
> + * dget - get a reference to a dentry
> + * @dentry: dentry to get a reference to
> + *
> + * Given a dentry or %NULL pointer increment the reference count
> + * if appropriate and return the dentry. A dentry will not be
> + * destroyed when it has references.
> + */
> static inline struct dentry *dget(struct dentry *dentry)
> {
> if (dentry)
> @@ -325,10 +334,10 @@ static inline struct dentry *dget(struct dentry *dentry)
> extern struct dentry *dget_parent(struct dentry *dentry);
>
> /**
> - * d_unhashed - is dentry hashed
> - * @dentry: entry to check
> + * d_unhashed - is dentry hashed
> + * @dentry: entry to check
> *
> - * Returns true if the dentry passed is not currently hashed.
> + * Returns true if the dentry passed is not currently hashed.
> */
>
> static inline int d_unhashed(const struct dentry *dentry)
Powered by blists - more mailing lists