[PATCH] smb: client: set replay flag on the read send-error retry path

[PATCH] smb: client: set replay flag on the read send-error retry path

[Christopher Lusk]

smb2_async_readv() and smb2_async_writev() end with the same
send-error block: if the error is replayable and smb2_should_replay()
agrees, tell netfs to retry the subrequest. The write path also sets
wdata->replay. The read path does not set rdata->replay.

smb2_should_replay() is not a pure predicate. It consumes the retry
budget and computes the exponential back-off, doubling cur_sleep up to
CIFS_MAX_SLEEP. That back-off is only applied where the replay flag is
tested at the top of the reissued request:

	if (rdata->replay) {
		/* Back-off before retry */
		if (rdata->cur_sleep)
			msleep(rdata->cur_sleep);
		smb2_set_replay(server, &rqst);
	}

So on the read path the back-off is recomputed on every send-error
retry and then discarded, and SMB2_FLAGS_REPLAY_OPERATION is not set
on the reissued request.

netfs does not pace the retry either. netfs_reissue_read() calls
->issue_read() directly, and fs/netfs/read_retry.c contains no delay
of its own, so read send-error retries reissue immediately while the
equivalent write retries back off.

The read response callback already sets rdata->replay under the same
conditions, so the read path does use the replay mechanism. Only this
send-error path omits it.

Where the back-off belongs was settled while the commit below was
under review. David Howells asked whether netfslib should be doing the
back-off [1], and objected to sleeping inside the response callback
because that runs in the cifsd thread and would stall the socket [2].
The sleep was therefore taken out of smb2_should_replay() and moved to
just before the replay in smb2_async_readv() and smb2_async_writev()
[3]. Setting the flag here preserves that arrangement: the sleep still
happens at the top of the reissued request, not in a callback.

Set rdata->replay here, matching smb2_async_writev().

Fixes: 2c1238a7477a ("cifs: make retry logic in read/write path consistent with other paths")
Link: https://lore.kernel.org/all/1652858.1769038134@warthog.procyon.org.uk/ [1]
Link: https://lore.kernel.org/all/1653031.1769038583@warthog.procyon.org.uk/ [2]
Link: https://lore.kernel.org/all/CANT5p=pXP3+CywpmK-on2uTvxO3S=31_B85_UDR7RoK1dQVtMA@mail.gmail.com/ [3]
Assisted-by: Codex:gpt-5.5
Assisted-by: Claude:claude-opus-5
Signed-off-by: Christopher Lusk <clusk@northecho.dev>
---

Tooling and testing, per Documentation/process/generated-content.rst:

- The site was surfaced by a static audit sweeping recent merge windows
  for state and contract defects, then resolved by reading the two
  paths and smb2_should_replay() directly. The audit had left it
  undecided because it framed the question as whether a synchronous
  send failure leaves transmission ambiguous. That question governs
  only the smb2_set_replay() half; the discarded back-off does not
  depend on it.
- The patch and changelog were drafted with LLM assistance, see the
  Assisted-by trailers, and reviewed line by line by me. I am
  responsible for all of it.
- The behavioural description above is derived from reading the code,
  not from measurement. I have not observed the retry timing against a
  live server.
- Deliberately not marked for stable. The change looks correct to me on
  a reading of the two paths, but I have no user report and no measured
  impact, and that seemed too thin a basis to ask for a backport. If
  you think it warrants one, please add the tag.
- Testing: compile-tested only. x86_64, CONFIG_CIFS=m, gcc 15.2.1,
  W=1, clean. Base is cifs-2.6 for-next fa724e235cfd ("cifs: add
  fscache_resize_cookie() to cifs_setsize()"). I have no SMB server
  test rig here, so the retry pacing change is not verified at runtime
  and a test from someone with one would be welcome.

One question I could not settle, raised separately because I did not
want to put it in the changelog without evidence:

smb2_should_replay() short-circuits as

	if (tcon->retry || (*pretries)++ < tcon->ses->server->retrans)

so on a hard mount the retry counter is never incremented and the
function always returns true. netfs does not bound the loop either;
subreq->retry_count is incremented in fs/netfs/read_retry.c but never
compared against anything. That suggests a persistent replayable send
error on a hard mount could retry without a bound, which this patch
would at least pace rather than fix. I may well be missing a
terminating condition elsewhere, for example adjust_credits() blocking
in cifs_issue_read() or the reconnect path breaking the loop, so I have
not made any claim about it above. For what it is worth, the ->retries
counter was described on-list as tracking client retransmissions "when
soft mounts are used", which is at least consistent with the hard-mount
path being unbounded by design:
https://lore.kernel.org/all/CANT5p=oL+tP5_SFNRabROCqDMjriXj5osnyyAjrMeq6BiJcr1Q@mail.gmail.com/

I read the full review history of 2c1238a7477a (v1 through v4) before
sending. The read/write asymmetry in the send-error blocks was not
raised by any reviewer or bot at the time, so as far as I can tell this
is an oversight rather than a deliberate choice.
 fs/smb/client/smb2pdu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 4ce165e40657..06ab6eeb4f13 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -4885,6 +4885,7 @@ smb2_async_readv(struct cifs_io_subrequest *rdata)
 	    smb2_should_replay(tcon,
 			       &rdata->retries,
 			       &rdata->cur_sleep)) {
+		rdata->replay = true;
 		trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_retry_needed);
 		__set_bit(NETFS_SREQ_NEED_RETRY, &rdata->subreq.flags);
 	}
-- 
2.54.0

Re: [PATCH] smb: client: set replay flag on the read send-error retry path

[Christopher Lusk]

On Wed, Jul 29, 2026, Steve French <smfrench@gmail.com> wrote:
> Any thoughts on the AI review comments from Sashiko?

Yes. Both comments are about pre-existing code rather than the line
this patch adds, and Sashiko says so itself in both cases. One of
them is a real bug, and I have sent a fix for it as a separate patch.
The other is a real property of the code, but it is symmetric with
the write path and this patch does not create it.

Details below. The code reading is against cifs-2.6/for-next at
fa724e23. I have no SMB Direct hardware, so the one thing I could
measure I measured by forcing the failure; the numbers are under
point 1.

1. The request buffer leak in smb2_new_read_req()

This one is real, and it is narrower than the comment suggests, and
it turns out to be another instance of exactly the read/write
asymmetry this patch is about.

smb2_new_read_req() allocates via smb2_plain_req_init() and then has
two early returns before it assigns *buf:

	rc = smb2_plain_req_init(SMB2_READ, ..., (void **)&req, total_len);
	if (rc)
		return rc;
	if (server == NULL)
		return -ECONNABORTED;
	...
	rdata->mr = smbd_register_mr(...);
	if (!rdata->mr)
		return -EAGAIN;

Only the -EAGAIN one is reachable. smb2_plain_req_init() calls
smb2_reconnect() first, and that already returns -EIO when !server,
before anything is allocated, so the -ECONNABORTED return is dead
code for any caller with a tcon.

On the -EAGAIN path the caller cannot clean up, because *buf was
never assigned and smb2_async_readv() does "goto out", which skips
cifs_small_buf_release(buf). So one small buf is leaked per attempt.

The write path already gets this right: smb2_async_writev() does the
MR registration itself and on failure does

	rc = -EAGAIN;
	goto async_writev_out;

which lands on cifs_small_buf_release(req). The read path leaks only
because the allocation is hidden inside the helper.

It is also not necessarily a one-shot leak. -EAGAIN is a replayable
error, so the failure reaches the retry block at the bottom of
smb2_async_readv(), which marks the subrequest
NETFS_SREQ_NEED_RETRY, and smb2_should_replay() starts with

	if (tcon->retry || (*pretries)++ < ...->retrans)

so on a hard mount the attempt count is not bounded by retrans. Every
attempt that reaches the failed registration leaks another buffer.

I want to be careful about how far I push that, because the testing
below only partly bears it out. Under forced failure the write side
was reissued and completed, but the read in my configuration was
unbuffered and netfs returned EAGAIN to userspace rather than
reissuing, so I got my per-attempt figure from five separate reads
rather than five retries of one. The per-attempt leak is measured;
how many attempts a real failure produces is not.

Scope: CONFIG_CIFS_SMB_DIRECT, and smb3_use_rdma_offload() true for
the I/O. The synchronous SMB2_read() caller passes rdata == NULL, and
the RDMA block is guarded on rdata, so it is the async read path
only.

None of this is new in my patch. The __set_bit(NETFS_SREQ_NEED_RETRY)
that closes the loop has been there since 2c1238a7477a. If anything
this patch slows the leak down, because the reissue now honours the
back-off that smb2_should_replay() had already computed.

The fix is on the list already, sent just before this mail:

  https://lore.kernel.org/linux-cifs/20260729220017.944651-1-clusk@northecho.dev/

It gives the function a common error label so both returns release
the buffer, rather than fixing only the one that can be reached
today:

	if (!server) {
		rc = -ECONNABORTED;
		goto free_req;
	}
	[...]
		if (!rdata->mr) {
			rc = -EAGAIN;
			goto free_req;
		}
	[...]
	*buf = req;
	return rc;

free_req:
	cifs_small_buf_release(req);
	return rc;

Fixes: bd3dcc6a22a9 ("CIFS: SMBD: Upper layer performs SMB read via
RDMA write through memory registration").

I could not test it on real hardware, so I forced the registration
failure with a throwaway debug patch, on both the read and the write
side, over an ordinary SMB2 TCP mount, and measured
small_buf_alloc_count from /proc/fs/cifs/Stats. Same kernel, same
test, without and with the fix:

                                        unpatched   patched
  clean read, no injection                    +0        +0
  5 reads, one forced read MR failure each    +5         0
  5 writes, one forced write MR failure each  +0        +0
  buffers still allocated after umount         5         0
  kmemleak objects under smb2_new_read_req     1         0

The write column is the control: the same forced failure on the path
that already has the release does not leak. kmemleak on the unpatched
run points at the 448 byte object with contents starting \xfeSMB and
a backtrace through cifs_small_buf_get, __smb2_plain_req_init,
smb2_new_read_req, smb2_async_readv, cifs_issue_read.

Full numbers, the injector diff, and the two caveats I want on the
record are in the patch's below-fold.

2. rdata->replay is never cleared

The mechanism is right. netfs reuses the same subrequest object for
retries and short-read continuations (netfs_reissue_read() and
netfs_retry_read_subrequests() adjust start/len and reissue the same
netfs_io_subrequest), so the enclosing cifs_io_subrequest, and with
it ->replay and ->cur_sleep, survives. Nothing in fs/smb/client ever
clears either field. So after one genuine replayable error on a
subrequest, a later continuation for the remaining bytes will sleep
up to CIFS_MAX_SLEEP and set SMB2_FLAGS_REPLAY_OPERATION on a request
that is not a replay.

But this is symmetric, and it predates this patch on both sides.
wdata->replay is set in smb2_writev_callback() and in
smb2_async_writev()'s out: block, and is likewise never cleared, so
short-write continuations behave the same way today. On the read
side, smb2_readv_callback() already sets rdata->replay. What this
patch changes is that the read send-error path now behaves like the
read response path and like both write paths. It widens an existing
condition rather than introducing one.

Worth noting that the async read and write paths are the only replay
users in the file that carry the flag in a structure. Every other
caller uses the replay_again: loop and recomputes .replay = !!(retries)
from a local counter on each attempt, so the flag cannot outlive the
attempt that set it.

If you and Shyam agree that the stickiness is wrong, the fix is to
clear ->replay and ->cur_sleep on both paths when a reissue is not a
retry after a replayable error. That is a behaviour change to the
read and write paths both, so it belongs in its own patch, and I did
not want to assume: a subrequest that has already failed once staying
in back-off mode is a defensible design, and I would rather hear
whether it was the intent.

So: the leak fix is out as a standalone patch, and I will write the
replay-clearing one only if you want it. If you would rather have all
of this as one series together with the patch this is a reply to, say
so and I will respin.

Analysis and test harness assisted by Claude (claude-opus-5). The
numbers under point 1 are measured; everything else above is from
reading the code at fa724e23.