Re: [PATCH 4/4] smb: prevent TIF_NOTIFY_SIGNAL from interrupting

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Partial send of the RFC1002 marker is not accounted for in `total_len` on error, leading to missed connection teardown and TCP stream corruption.
--

--- Patch [4]: [PATCH 4/4] smb: prevent TIF_NOTIFY_SIGNAL from interrupting ---
commit 0a99d5c0436dadc5ec220717067bbc5bd7313e1f
Author: Christian Brauner <[email protected]>
smb: prevent TIF_NOTIFY_SIGNAL from interrupting

This commit replaces a hand-rolled signal masking solution in
smb_send_kvec() with the new no_notify_signal_save() critical section
to prevent io_uring from cancelling a send in flight.

> diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
> index fdf4e50c27ceb..05dc20916c276 100644
> --- a/fs/smb/client/transport.c
> +++ b/fs/smb/client/transport.c

[ ... ]

> @@ -172,15 +172,12 @@ smb_send_kvec(struct TCP_Server_Info *server, struct msghdr *smb_msg,
>  		 * after the retries we will kill the socket and
>  		 * reconnect which may clear the network problem.
>  		 *
> -		 * Even if regular signals are masked, EINTR might be
> -		 * propagated from sk_stream_wait_memory() to here when
> -		 * TIF_NOTIFY_SIGNAL is used for task work. For example,
> -		 * certain io_uring completions will use that. Treat
> -		 * having EINTR with pending task work the same as EAGAIN
> -		 * to avoid unnecessary reconnects.
> +		 * Task work must not abort the send, see signal_pending().
>  		 */
> +		notify_flags = no_notify_signal_save();
>  		rc = sock_sendmsg(ssocket, smb_msg);
> -		if (rc == -EAGAIN || unlikely(rc == -EINTR && task_work_pending(current))) {
> +		no_notify_signal_restore(notify_flags);
> +		if (rc == -EAGAIN) {

[Severity: High]
This is a pre-existing issue, but does returning -EAGAIN here cause a
protocol desync if we partially sent the RFC1002 marker?

Looking at __smb_send_rqst() which calls smb_send_kvec() to send the
4-byte marker:

fs/smb/client/transport.c:__smb_send_rqst() {
        ...
        rc = smb_send_kvec(server, &smb_msg, &sent);
        if (rc < 0)
                goto unmask;

        total_len += sent;
        ...
}

If smb_send_kvec() successfully sends 1 to 3 bytes of the marker, but
then blocks and eventually returns -EAGAIN due to the 15-second retry
timeout, the code jumps to unmask without updating total_len.

Later in the function, the reconnect logic checks total_len:

fs/smb/client/transport.c:__smb_send_rqst() {
        ...
        if ((total_len > 0) && (total_len != send_length)) {
                cifs_dbg(FYI, "partial send (wanted=%u sent=%zu): terminating session\n",
                         send_length, total_len);
                cifs_signal_cifsd_for_reconnect(server, false);
                ...
        }
}

Since total_len remains 0, this check fails and the connection is not
terminated. Will this leave stray bytes in the TCP stream and cause the
server to misinterpret all subsequent requests?

>  			retries++;
>  			if (retries >= 14 ||
>  			    (!server->noblocksnd && (retries > 2))) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4