nfsd: fix inverted cp_ttl check in async copy reaper

[PATCH] nfsd: fix inverted cp_ttl check in async copy reaper

Posted by Jeff Layton 3 days, 6 hours ago

nfsd4_async_copy_reaper() is supposed to keep completed async copy
state around for NFSD_COPY_INITIAL_TTL (10) laundromat ticks so
that OFFLOAD_STATUS can report the result, then reap the state once
the countdown expires.

The TTL predicate is inverted: `if (--copy->cp_ttl)` is true while
ticks remain and false when the counter reaches zero.  This causes
the copy to be reaped on the very first tick (cp_ttl goes from 10
to 9, which is non-zero) instead of after all 10 ticks elapse.
Once reaped, OFFLOAD_STATUS returns NFS4ERR_BAD_STATEID because
the copy state has already been freed.

A secondary consequence: if cp_ttl ever reached zero (not possible
with the current initial value of 10 since the copy is reaped at
9), the copy would never be added to the reaplist and would leak
indefinitely on clp->async_copies.

Fix by negating the test so that cleanup runs when the TTL expires.

Fixes: 26e6e6939369 ("NFSD: Add nfsd4_copy time-to-live")
Cc: stable@vger.kernel.org
Reported-by: Chris Mason <clm@meta.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 85e94c30285a..72f410fd7a8a 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1467,7 +1467,7 @@ void nfsd4_async_copy_reaper(struct nfsd_net *nn)
 		list_for_each_safe(pos, next, &clp->async_copies) {
 			copy = list_entry(pos, struct nfsd4_copy, copies);
 			if (test_bit(NFSD4_COPY_F_OFFLOAD_DONE, &copy->cp_flags)) {
-				if (--copy->cp_ttl) {
+				if (!--copy->cp_ttl) {
 					list_del_init(&copy->copies);
 					list_add(&copy->copies, &reaplist);
 				}

---
base-commit: e91f16e59f513a1e3fc052e2b630efebeaf9a4f2
change-id: 20260521-async_copy_reaper_cp_ttl_inverted-697a0a3c850b

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: fix inverted cp_ttl check in async copy reaper

Posted by Chuck Lever 3 days, 3 hours ago

From: Chuck Lever <chuck.lever@oracle.com>

On Thu, 21 May 2026 09:25:40 -0400, Jeff Layton wrote:
> nfsd4_async_copy_reaper() is supposed to keep completed async copy
> state around for NFSD_COPY_INITIAL_TTL (10) laundromat ticks so
> that OFFLOAD_STATUS can report the result, then reap the state once
> the countdown expires.
> 
> The TTL predicate is inverted: `if (--copy->cp_ttl)` is true while
> ticks remain and false when the counter reaches zero.  This causes
> the copy to be reaped on the very first tick (cp_ttl goes from 10
> to 9, which is non-zero) instead of after all 10 ticks elapse.
> Once reaped, OFFLOAD_STATUS returns NFS4ERR_BAD_STATEID because
> the copy state has already been freed.
> 
> [...]

Applied to nfsd-testing, thanks!

[1/1] nfsd: fix inverted cp_ttl check in async copy reaper
      commit: 14ba3b4b7cb4d05a070c77226d10dfc332898421

--
Chuck Lever <chuck.lever@oracle.com>

Re: [PATCH] nfsd: fix inverted cp_ttl check in async copy reaper

Posted by Paulo Alcantara 3 days, 5 hours ago

Jeff Layton <jlayton@kernel.org> writes:

> nfsd4_async_copy_reaper() is supposed to keep completed async copy
> state around for NFSD_COPY_INITIAL_TTL (10) laundromat ticks so
> that OFFLOAD_STATUS can report the result, then reap the state once
> the countdown expires.
>
> The TTL predicate is inverted: `if (--copy->cp_ttl)` is true while
> ticks remain and false when the counter reaches zero.  This causes
> the copy to be reaped on the very first tick (cp_ttl goes from 10
> to 9, which is non-zero) instead of after all 10 ticks elapse.
> Once reaped, OFFLOAD_STATUS returns NFS4ERR_BAD_STATEID because
> the copy state has already been freed.
>
> A secondary consequence: if cp_ttl ever reached zero (not possible
> with the current initial value of 10 since the copy is reaped at
> 9), the copy would never be added to the reaplist and would leak
> indefinitely on clp->async_copies.
>
> Fix by negating the test so that cleanup runs when the TTL expires.
>
> Fixes: 26e6e6939369 ("NFSD: Add nfsd4_copy time-to-live")

Wouldn't aa0ebd21df9c be the correct commit id?

Re: [PATCH] nfsd: fix inverted cp_ttl check in async copy reaper

Posted by Jeff Layton 3 days, 5 hours ago

On Thu, 2026-05-21 at 11:28 -0300, Paulo Alcantara wrote:
> Jeff Layton <jlayton@kernel.org> writes:
> 
> > nfsd4_async_copy_reaper() is supposed to keep completed async copy
> > state around for NFSD_COPY_INITIAL_TTL (10) laundromat ticks so
> > that OFFLOAD_STATUS can report the result, then reap the state once
> > the countdown expires.
> > 
> > The TTL predicate is inverted: `if (--copy->cp_ttl)` is true while
> > ticks remain and false when the counter reaches zero.  This causes
> > the copy to be reaped on the very first tick (cp_ttl goes from 10
> > to 9, which is non-zero) instead of after all 10 ticks elapse.
> > Once reaped, OFFLOAD_STATUS returns NFS4ERR_BAD_STATEID because
> > the copy state has already been freed.
> > 
> > A secondary consequence: if cp_ttl ever reached zero (not possible
> > with the current initial value of 10 since the copy is reaped at
> > 9), the copy would never be added to the reaplist and would leak
> > indefinitely on clp->async_copies.
> > 
> > Fix by negating the test so that cleanup runs when the TTL expires.
> > 
> > Fixes: 26e6e6939369 ("NFSD: Add nfsd4_copy time-to-live")
> 
> Wouldn't aa0ebd21df9c be the correct commit id?

Thanks, yes it would. The other commit id is correct for a copy in my
tree, but aa0ebd21df9c is the correct upstream one.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: fix inverted cp_ttl check in async copy reaper

Posted by Jeff Layton 3 days, 6 hours ago

On Thu, 2026-05-21 at 09:25 -0400, Jeff Layton wrote:
> nfsd4_async_copy_reaper() is supposed to keep completed async copy
> state around for NFSD_COPY_INITIAL_TTL (10) laundromat ticks so
> that OFFLOAD_STATUS can report the result, then reap the state once
> the countdown expires.
> 
> The TTL predicate is inverted: `if (--copy->cp_ttl)` is true while
> ticks remain and false when the counter reaches zero.  This causes
> the copy to be reaped on the very first tick (cp_ttl goes from 10
> to 9, which is non-zero) instead of after all 10 ticks elapse.
> Once reaped, OFFLOAD_STATUS returns NFS4ERR_BAD_STATEID because
> the copy state has already been freed.
> 
> A secondary consequence: if cp_ttl ever reached zero (not possible
> with the current initial value of 10 since the copy is reaped at
> 9), the copy would never be added to the reaplist and would leak
> indefinitely on clp->async_copies.
> 
> Fix by negating the test so that cleanup runs when the TTL expires.
> 
> Fixes: 26e6e6939369 ("NFSD: Add nfsd4_copy time-to-live")
> Cc: stable@vger.kernel.org

Mea culpa. This doesn't need to go to stable so we can drop that tag.
It's a bug, but not one that results in a crash or data corruption or
anything. Also, this should have:

Assisted-by: kres:claude-opus-4-6

> Reported-by: Chris Mason <clm@meta.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs4proc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 85e94c30285a..72f410fd7a8a 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1467,7 +1467,7 @@ void nfsd4_async_copy_reaper(struct nfsd_net *nn)
>  		list_for_each_safe(pos, next, &clp->async_copies) {
>  			copy = list_entry(pos, struct nfsd4_copy, copies);
>  			if (test_bit(NFSD4_COPY_F_OFFLOAD_DONE, &copy->cp_flags)) {
> -				if (--copy->cp_ttl) {
> +				if (!--copy->cp_ttl) {
>  					list_del_init(&copy->copies);
>  					list_add(&copy->copies, &reaplist);
>  				}
> 
> ---
> base-commit: e91f16e59f513a1e3fc052e2b630efebeaf9a4f2
> change-id: 20260521-async_copy_reaper_cp_ttl_inverted-697a0a3c850b
> 
> Best regards,

-- 
Jeff Layton <jlayton@kernel.org>