Re: [PATCH] cachefiles: Fix excess dput() after end_removing()

[NeilBrown <neilb@ownmail.net>]

On Thu, 26 Mar 2026, David Howells wrote:
> Marc Dionne <marc.c.dionne@gmail.com> wrote:
> 
> > I think it is the correct Fixes tag, but I'm not sure that this is
> > actually the right fix.  7bb1eb45e43c switched other callers of
> > cachefiles_bury_object to use start_removing_dentry, which gets an
> > additional ref, and removed the extra dget from
> > cachefiles_bury_object.  In the cachefiles_cull case however, the
> > dentry is from start_removing and has a single ref on entry to
> > cachefiles_bury_object, which is an issue as "rep" may be used there
> > after end_removing may have put the last ref.  So the correct is
> > probably for cachefiles_cull to add a dget() before the call to
> > cachefiles_bury_object.
> 
> Ugh.  You're right.
> 
> The problem is that we're calling start_removing() without knowing whether we
> can just unlink the object.  I wonder if I need to do the lookup in
> cachefiles_lookup_for_cull() and only then call start_removing_dentry() if
> it's not a directory (directories get moved to the graveyard for cachefilesd
> to tear down).
> 
> I think the right solution is actually to move start_removing_dentry() down
> into cachefiles_bury_object() and make it contingent on the dentry being a
> non-dir.
> 
> David
> 
> 

cachesfiles_bury_object() has a comment saying:

 * On entry there must be at least 2 refs on rep, one will be dropped on exit.

and this is consistent with the code in that function.

It is called from 3 places.
- cachefiles_invalidate_cookie(), cachesfiles_look_up_object(), and
  cachefiles_acquire_volume() all precede it with a
  start_removing_dentry() which results in 2 references to the dentry
  (the original and and extra which it takes) - so that fits with the
  comment.

- cachesfiles_cull() preceeds it with cachesfiles_lookup_for_cull()
  which uses start_removing() which returns with 1 reference to the
  dentry.  As the dentry didn't pre-exist, there is only one ref.
  So this is incorrect.

cachesfiles_cull() needs to take an extra reference to victim so that
when cachefiles_busy_object() calls end_removing, it still has a valid
reference.

So I think

--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -781,7 +781,7 @@ int cachefiles_cull(struct cachefiles_cache *cache, struct dentry *dir,
 	if (ret < 0)
 		goto error_unlock;
 
-	ret = cachefiles_bury_object(cache, NULL, dir, victim,
+	ret = cachefiles_bury_object(cache, NULL, dir, dget(victim),
 				     FSCACHE_OBJECT_WAS_CULLED);
 	dput(victim);
 	if (ret < 0)

would be a correct fix.

If you agree I can post a properly formated patch which explanation.

Thanks,
NeilBrown