[PATCH 07/26] cachefiles: Fix excess dput() after end_removing()

[Aditya]

Why are this bulk fixes

On Thu, 26 Mar 2026 at 4:17 PM, David Howells <dhowells at redhat.com> wrote:

> When cachefiles_cull() calls cachefiles_bury_object(), the latter eats the
> former's ref on the victim dentry that it obtained from
> cachefiles_lookup_for_cull().  However, commit 7bb1eb45e43c left the dput
> of the victim in place, resulting in occasional:
>
>   WARNING: fs/dcache.c:829 at dput.part.0+0xf5/0x110, CPU#7:
> cachefilesd/11831
>   cachefiles_cull+0x8c/0xe0 [cachefiles]
>   cachefiles_daemon_cull+0xcd/0x120 [cachefiles]
>   cachefiles_daemon_write+0x14e/0x1d0 [cachefiles]
>   vfs_write+0xc3/0x480
>   ...
>
> reports.
>
> Actually, it's worse than that: cachefiles_bury_object() eats the ref it
> was
> given - and then may continue to the now-unref'd dentry it if it turns out
> to
> be a directory.  So simply removing the aberrant dput() is not sufficient.
>
> Fix this by making cachefiles_bury_object() retain the ref itself around
> end_removing() if it needs to keep it and then drop the ref before
> returning.
>
> Fixes: bd6ede8a06e8 ("VFS/nfsd/cachefiles/ovl: introduce start_removing()
> and end_removing()")
> Reported-by: Marc Dionne <marc.dionne at auristor.com>
> Signed-off-by: David Howells <dhowells at redhat.com>
> cc: NeilBrown <neil at brown.name>
> cc: Paulo Alcantara <pc at manguebit.org>
> cc: netfs at lists.linux.dev
> cc: linux-afs at lists.infradead.org
> cc: linux-fsdevel at vger.kernel.org
> ---
>  fs/cachefiles/namei.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index e5ec90dccc27..20138309733f 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -287,14 +287,14 @@ int cachefiles_bury_object(struct cachefiles_cache
> *cache,
>         if (!d_is_dir(rep)) {
>                 ret = cachefiles_unlink(cache, object, dir, rep, why);
>                 end_removing(rep);
> -
>                 _leave(" = %d", ret);
>                 return ret;
>         }
>
>         /* directories have to be moved to the graveyard */
>         _debug("move stale object to graveyard");
> -       end_removing(rep);
> +       dget(rep);
> +       end_removing(rep); /* Drops ref on rep */
>
>  try_again:
>         /* first step is to make up a grave dentry in the graveyard */
> @@ -304,8 +304,10 @@ int cachefiles_bury_object(struct cachefiles_cache
> *cache,
>
>         /* do the multiway lock magic */
>         trap = lock_rename(cache->graveyard, dir);
> -       if (IS_ERR(trap))
> -               return PTR_ERR(trap);
> +       if (IS_ERR(trap)) {
> +               ret = PTR_ERR(trap);
> +               goto out;
> +       }
>
>         /* do some checks before getting the grave dentry */
>         if (rep->d_parent != dir || IS_DEADDIR(d_inode(rep))) {
> @@ -313,25 +315,27 @@ int cachefiles_bury_object(struct cachefiles_cache
> *cache,
>                  * lock */
>                 unlock_rename(cache->graveyard, dir);
>                 _leave(" = 0 [culled?]");
> -               return 0;
> +               ret = 0;
> +               goto out;
>         }
>
> +       ret = -EIO;
>         if (!d_can_lookup(cache->graveyard)) {
>                 unlock_rename(cache->graveyard, dir);
>                 cachefiles_io_error(cache, "Graveyard no longer a
> directory");
> -               return -EIO;
> +               goto out;
>         }
>
>         if (trap == rep) {
>                 unlock_rename(cache->graveyard, dir);
>                 cachefiles_io_error(cache, "May not make directory loop");
> -               return -EIO;
> +               goto out;
>         }
>
>         if (d_mountpoint(rep)) {
>                 unlock_rename(cache->graveyard, dir);
>                 cachefiles_io_error(cache, "Mountpoint in cache");
> -               return -EIO;
> +               goto out;
>         }
>
>         grave = lookup_one(&nop_mnt_idmap, &QSTR(nbuffer),
> cache->graveyard);
> @@ -343,11 +347,12 @@ int cachefiles_bury_object(struct cachefiles_cache
> *cache,
>
>                 if (PTR_ERR(grave) == -ENOMEM) {
>                         _leave(" = -ENOMEM");
> -                       return -ENOMEM;
> +                       ret = -ENOMEM;
> +                       goto out;
>                 }
>
>                 cachefiles_io_error(cache, "Lookup error %ld",
> PTR_ERR(grave));
> -               return -EIO;
> +               goto out;
>         }
>
>         if (d_is_positive(grave)) {
> @@ -362,7 +367,7 @@ int cachefiles_bury_object(struct cachefiles_cache
> *cache,
>                 unlock_rename(cache->graveyard, dir);
>                 dput(grave);
>                 cachefiles_io_error(cache, "Mountpoint in graveyard");
> -               return -EIO;
> +               goto out;
>         }
>
>         /* target should not be an ancestor of source */
> @@ -370,7 +375,7 @@ int cachefiles_bury_object(struct cachefiles_cache
> *cache,
>                 unlock_rename(cache->graveyard, dir);
>                 dput(grave);
>                 cachefiles_io_error(cache, "May not make directory loop");
> -               return -EIO;
> +               goto out;
>         }
>
>         /* attempt the rename */
> @@ -404,8 +409,10 @@ int cachefiles_bury_object(struct cachefiles_cache
> *cache,
>         __cachefiles_unmark_inode_in_use(object, d_inode(rep));
>         unlock_rename(cache->graveyard, dir);
>         dput(grave);
> -       _leave(" = 0");
> -       return 0;
> +       _leave(" = %d", ret);
> +out:
> +       dput(rep);
> +       return ret;
>  }
>
>  /*
> @@ -812,7 +819,6 @@ int cachefiles_cull(struct cachefiles_cache *cache,
> struct dentry *dir,
>
>         ret = cachefiles_bury_object(cache, NULL, dir, victim,
>                                      FSCACHE_OBJECT_WAS_CULLED);
> -       dput(victim);
>         if (ret < 0)
>                 goto error;
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-erofs/attachments/20260326/1e7910e1/attachment.htm>