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

[Christopher Lusk]

On Wed, Jul 29, 2026, Steve French <smfrench@xxxxxxxxx> 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@xxxxxxxxxxxxx/

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.