Re: [PATCH] smb: client: fix request buffer leak in smb2_new_read_req()

On Thu, Jul 30, 2026 at 7:05 AM Christopher Lusk <[email protected]> wrote:
>
> smb2_new_read_req() allocates the request buffer with
> smb2_plain_req_init() but only publishes it to the caller with
> *buf = req at the very end of the function. Two error returns sit in
> between:
>
>         rc = smb2_plain_req_init(SMB2_READ, io_parms->tcon, server,
>                                  (void **) &req, total_len);
>         if (rc)
>                 return rc;
>
>         if (server == NULL)
>                 return -ECONNABORTED;
>         [...]
>                 rdata->mr = smbd_register_mr(server->smbd_conn,
>                                              &rdata->subreq.io_iter,
>                                              true, need_invalidate);
>                 if (!rdata->mr)
>                         return -EAGAIN;
>
> On either of them the buffer is neither released nor handed back, so
> it is leaked. The caller cannot clean up after it: smb2_async_readv()
> does 'goto out' on a non-zero return, which skips the
> cifs_small_buf_release(buf) at async_readv_out, and buf has not been
> assigned at that point in any case.
>
> The write path has never had this problem. smb2_async_writev()
> registers the memory region inline and jumps to its release label
> instead of returning:
>
>         wdata->mr = smbd_register_mr(...);
>         if (!wdata->mr) {
>                 rc = -EAGAIN;
>                 goto async_writev_out;
>         }
>
> Commit b7972092199f ("cifs: smbd: Retry on memory registration
> failure") changed both sides from -ENOBUFS to -EAGAIN in a single
> patch, which puts the two shapes next to each other.
>
> Only the -EAGAIN return is reachable in practice, because
> smb2_plain_req_init() calls smb2_reconnect() first and that already
> fails with -EIO when server is NULL, before anything is allocated.
> Both returns are given the same treatment here rather than leaving
> one of them correct only by accident.
>
> Because -EAGAIN is a replayable error, the failure also reaches the
> retry block at the end of smb2_async_readv(), which marks the
> subrequest NETFS_SREQ_NEED_RETRY, so a failing registration can be
> retried rather than ending the I/O, and every attempt that reaches it
> leaks another buffer. smb2_should_replay() short-circuits on
> tcon->retry, so on a hard mount the attempt count is not bounded by
> the retrans setting.
>
> Only the asynchronous read path is affected. The synchronous
> SMB2_read() caller passes rdata == NULL and the memory registration
> block is guarded on rdata.
>
> The memory registration failure path was pointed out by the Sashiko
> AI reviewer while it was reviewing an unrelated patch to
> smb2_async_readv().
>
> Fixes: bd3dcc6a22a9 ("CIFS: SMBD: Upper layer performs SMB read via RDMA write through memory registration")
> Link: https://sashiko.dev/#/patchset/20260729192002.876156-1-clusk%40northecho.dev
> Link: https://lore.kernel.org/all/[email protected]/
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Christopher Lusk <[email protected]>
I will apply it to #for-next.
Thanks!