| 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: <CAOQ4uxgbv8as-m7P9Az41nZiOBPLN0znW4xu1HSqpuCN1=Tp4Q@mail.gmail.com>
Date: Fri, 22 Aug 2025 11:44:59 +0200
From: Amir Goldstein <amir73il@...il.com>
To: NeilBrown <neil@...wn.name>
Cc: 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 v2 15/16] VFS: introduce start_removing_dentry()
On Fri, Aug 22, 2025 at 2:11 AM NeilBrown <neil@...wn.name> wrote:
>
> start_removing_dentry() is similar to start_removing() but instead of
> providing a name for lookup, the target dentry is given.
>
> start_removing_dentry() checks that the dentry is still hashed and in
> the parent, and if so it locks and increases the refcount so that
> end_dirop() can be used to finish the operation.
>
> This is used in cachefiles, overlayfs, smb/server and apparmor.
>
> There will be other users including ecryptfs.
>
> Signed-off-by: NeilBrown <neil@...wn.name>
> ---
> fs/cachefiles/interface.c | 14 +++++++++-----
> fs/cachefiles/namei.c | 22 ++++++++++++----------
> fs/cachefiles/volume.c | 10 +++++++---
> fs/namei.c | 29 +++++++++++++++++++++++++++++
> fs/overlayfs/dir.c | 10 ++++------
> fs/overlayfs/readdir.c | 8 ++++----
> fs/smb/server/vfs.c | 27 ++++-----------------------
> include/linux/namei.h | 2 ++
> security/apparmor/apparmorfs.c | 8 ++++----
> 9 files changed, 75 insertions(+), 55 deletions(-)
>
> diff --git a/fs/cachefiles/interface.c b/fs/cachefiles/interface.c
> index 3e63cfe15874..763d7d55b1f9 100644
> --- a/fs/cachefiles/interface.c
> +++ b/fs/cachefiles/interface.c
> @@ -9,6 +9,7 @@
> #include <linux/mount.h>
> #include <linux/xattr.h>
> #include <linux/file.h>
> +#include <linux/namei.h>
> #include <linux/falloc.h>
> #include <trace/events/fscache.h>
> #include "internal.h"
> @@ -428,11 +429,14 @@ static bool cachefiles_invalidate_cookie(struct fscache_cookie *cookie)
> if (!old_tmpfile) {
> struct cachefiles_volume *volume = object->volume;
> struct dentry *fan = volume->fanout[(u8)cookie->key_hash];
> -
> - inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);
> - cachefiles_bury_object(volume->cache, object, fan,
> - old_file->f_path.dentry,
> - FSCACHE_OBJECT_INVALIDATED);
> + struct dentry *obj;
> +
> + obj = start_removing_dentry(fan, old_file->f_path.dentry);
> + if (!IS_ERR(obj))
> + cachefiles_bury_object(volume->cache, object,
> + fan, obj,
> + FSCACHE_OBJECT_INVALIDATED);
> + end_dirop(obj);
> }
> fput(old_file);
> }
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index ddced50afb66..cc6dccd606ea 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -424,13 +424,12 @@ int cachefiles_delete_object(struct cachefiles_object *object,
>
> _enter(",OBJ%x{%pD}", object->debug_id, object->file);
>
> - /* Stop the dentry being negated if it's only pinned by a file struct. */
> - dget(dentry);
> -
> - inode_lock_nested(d_backing_inode(fan), I_MUTEX_PARENT);
> - ret = cachefiles_unlink(volume->cache, object, fan, dentry, why);
> - inode_unlock(d_backing_inode(fan));
> - dput(dentry);
> + dentry = start_removing_dentry(fan, dentry);
> + if (IS_ERR(dentry))
> + ret = PTR_ERR(dentry);
> + else
> + ret = cachefiles_unlink(volume->cache, object, fan, dentry, why);
> + end_dirop(dentry);
> return ret;
> }
>
> @@ -643,9 +642,12 @@ bool cachefiles_look_up_object(struct cachefiles_object *object)
>
> if (!d_is_reg(dentry)) {
> pr_err("%pd is not a file\n", dentry);
> - inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);
> - ret = cachefiles_bury_object(volume->cache, object, fan, dentry,
> - FSCACHE_OBJECT_IS_WEIRD);
> + struct dentry *de = start_removing_dentry(fan, dentry);
> + if (!IS_ERR(de))
> + ret = cachefiles_bury_object(volume->cache, object,
> + fan, de,
> + FSCACHE_OBJECT_IS_WEIRD);
> + end_dirop(de);
> dput(dentry);
> if (ret < 0)
> return false;
> diff --git a/fs/cachefiles/volume.c b/fs/cachefiles/volume.c
> index 781aac4ef274..8c29f3db3fae 100644
> --- a/fs/cachefiles/volume.c
> +++ b/fs/cachefiles/volume.c
> @@ -7,6 +7,7 @@
>
> #include <linux/fs.h>
> #include <linux/slab.h>
> +#include <linux/namei.h>
> #include "internal.h"
> #include <trace/events/fscache.h>
>
> @@ -58,9 +59,12 @@ void cachefiles_acquire_volume(struct fscache_volume *vcookie)
> if (ret < 0) {
> if (ret != -ESTALE)
> goto error_dir;
> - inode_lock_nested(d_inode(cache->store), I_MUTEX_PARENT);
> - cachefiles_bury_object(cache, NULL, cache->store, vdentry,
> - FSCACHE_VOLUME_IS_WEIRD);
> + vdentry = start_removing_dentry(cache->store, vdentry);
> + if (!IS_ERR(vdentry))
> + cachefiles_bury_object(cache, NULL, cache->store,
> + vdentry,
> + FSCACHE_VOLUME_IS_WEIRD);
> + end_dirop(vdentry);
> cachefiles_put_directory(volume->dentry);
> cond_resched();
> goto retry;
> diff --git a/fs/namei.c b/fs/namei.c
> index 34895487045e..af56bc39c4d5 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3261,6 +3261,35 @@ struct dentry *start_removing_noperm(struct dentry *parent,
> }
> EXPORT_SYMBOL(start_removing_noperm);
>
> +/**
> + * start_removing_dentry - prepare to remove a given dentry
> + * @parent - directory from which dentry should be removed
> + * @child - the dentry to be removed
> + *
> + * A lock is taken to protect the dentry again other dirops and
> + * the validity of the dentry is checked: correct parent and still hashed.
> + *
> + * If the dentry is valid a reference is taken and returned. If not
> + * an error is returned.
> + *
> + * end_dirop() should be called when removal is complete, or aborted.
> + *
> + * Returns: the valid dentry, or an error.
> + */
> +struct dentry *start_removing_dentry(struct dentry *parent,
> + struct dentry *child)
> +{
> + inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> + if (unlikely(IS_DEADDIR(parent->d_inode) ||
> + child->d_parent != parent ||
> + d_unhashed(child))) {
> + inode_unlock(parent->d_inode);
> + return ERR_PTR(-EINVAL);
> + }
> + return dget(child);
> +}
> +EXPORT_SYMBOL(start_removing_dentry);
> +
> #ifdef CONFIG_UNIX98_PTYS
> int path_pts(struct path *path)
> {
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 70b8687dc45e..b8f0d409e841 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -47,14 +47,12 @@ static int ovl_cleanup_locked(struct ovl_fs *ofs, struct inode *wdir,
> int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir,
> struct dentry *wdentry)
> {
> - int err;
> -
> - err = ovl_parent_lock(workdir, wdentry);
> - if (err)
> - return err;
> + wdentry = start_removing_dentry(workdir, wdentry);
> + if (IS_ERR(wdentry))
> + return PTR_ERR(wdentry);
>
> ovl_cleanup_locked(ofs, workdir->d_inode, wdentry);
> - ovl_parent_unlock(workdir);
> + end_dirop(wdentry);
>
> return 0;
> }
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index b65cdfce31ce..20348be4b98f 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1158,11 +1158,11 @@ int ovl_workdir_cleanup(struct ovl_fs *ofs, struct dentry *parent,
> if (!d_is_dir(dentry) || level > 1)
> return ovl_cleanup(ofs, parent, dentry);
>
> - err = ovl_parent_lock(parent, dentry);
> - if (err)
> - return err;
> + dentry = start_removing_dentry(parent, dentry);
> + if (IS_ERR(dentry))
> + return PTR_ERR(dentry);
> err = ovl_do_rmdir(ofs, parent->d_inode, dentry);
> - ovl_parent_unlock(parent);
> + end_dirop(dentry);
> if (err) {
> struct path path = { .mnt = mnt, .dentry = dentry };
>
I'm sorry I keep nagging about this semantic point, but when I request
that code remains "balanced", I mean "balanced to a human eye".
If we are going to delegate reviews to LLM, then we can feed LLM
the documentation that says which start_ pairs with which end_
and go on to our retirement plans, but as long as I need to review
code, I need a human readable signal about what pairs with what.
Therefore, IMO it is better to have semantic wrappers
end_removing(), end_creating(), than having to rely on humans
to understand that start_removing_dentry() pairs with end_dirop().
Alternatively, use start_dirop_remove*, start_dirop_create*,
so it is clear that they can all match end_dirop() (can they???).
And repeating my request again: I will insist for overlayfs patches
but I think it is a good practice for all of your patches -
Please keep the code balance by introducing start_ together
with end_ in the same patch, so that it is clear from context of
a single patch review, that the callers were converted correctly.
Otherwise, the only way to verify that is to review the end result
and that is not the idea of a patch series.
Thanks,
Amir.
Powered by blists - more mailing lists