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