Re: [PATCH net-next v3 1/2] tls: annotate lockless access to sk->sk_err

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit body describes a user-visible bug: tls_rx_rec_wait()
  "can return 0 after another thread consumes the error".

Pre-existing issues:
- [Medium] This problem was not introduced by this patch, but the patch
  edits the line involved.

--

tls: annotate lockless access to sk->sk_err

In the kTLS TX paths, this patch replaces repeated unmarked loads of
sk->sk_err with a single READ_ONCE() snapshot. On the no-data path of
tls_rx_rec_wait(), it drops the "if (sk->sk_err)" pre-check and calls
sock_error() just once, so another thread that consumes sk_err can no
longer turn the error into a 0 return.

Link: https://lore.kernel.org/netdev/3d9d442f-f168-43da-87b0-010ad5a78365@redhat.com/

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 312e51270f293..a2be1283ca930 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -704,10 +707,14 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
>  	int err;
>  
>  	err = tls_push_record(sk, flags, record_type);
> -	if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
> -		*copied -= sk_msg_free(sk, msg);
> -		tls_free_open_rec(sk);
> -		err = -sk->sk_err;
> +	if (err && err != -EINPROGRESS) {
> +		int skerr = READ_ONCE(sk->sk_err);
> +
> +		if (skerr == EBADMSG) {
> +			*copied -= sk_msg_free(sk, msg);
> +			tls_free_open_rec(sk);
> +			err = -skerr;
> +		}
>  	}
>  	return err;
>  }

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but the
patch touches this line. Can the EBADMSG test in bpf_exec_tx_verdict() miss
a synchronous encryption failure?

tls_push_record() reports the failure by writing sk_err, then returns the
raw crypto rc:

net/tls/tls_sw.c:tls_push_record() {
    ...
	rc = tls_do_encryption(sk, tls_ctx, ctx, req,
			       msg_pl->sg.size + prot->tail_size, i);
	if (rc < 0) {
		if (rc != -EINPROGRESS)
			tls_err_abort(sk, -EBADMSG);
		ctx->async_capable = 1;
		return rc;
	}
    ...
}

bpf_exec_tx_verdict() then reads sk_err back to decide whether to clean up.
Some writers that don't hold lock_sock can change sk_err between
tls_err_abort() and the READ_ONCE() here:

  getsockopt(SO_ERROR): sk_getsockopt()->sock_error() does
  xchg(&sk->sk_err, 0)

  do_recvmmsg() or MSG_ERRQUEUE, via sock_dequeue_err_skb()

  tls_encrypt_done() for an earlier async record, which can overwrite
  sk_err with its own error via tls_err_abort()

If one of these lands in the window, sk_msg_free() and tls_free_open_rec()
are skipped. *copied then still counts bytes that were never encrypted or
sent.

ctx->open_rec also keeps pointing at the failed record. On this error path
tls_do_encryption() has already done list_del(&rec->list) and returned
without clearing ctx->open_rec.

If sk_err was cleared, the next sendmsg passes the loop-top sk_err check in
tls_sw_sendmsg_locked() and reuses that record. Its scatterlists were
already end-marked and chained by the earlier tls_push_record().

The single READ_ONCE() snapshot removes the old double load, but it doesn't
close this window between the write and the read-back. Would it be more
robust for tls_push_record() to report the encryption failure to its caller
directly, instead of passing it through sk_err?

[ ... ]

> @@ -1132,10 +1144,16 @@ tls_rx_rec_wait(struct sock *sk, bool nonblock, bool released, bool has_copied)
>  		 * sk_err here so a connection abort surfaces as the
>  		 * actual error rather than a clean EOF.
>  		 */
> -		if (sk->sk_err) {
> -			if (has_copied)
> -				return -READ_ONCE(sk->sk_err);
> -			return sock_error(sk);
> +		if (has_copied) {
> +			int err = READ_ONCE(sk->sk_err);
> +
> +			if (err)
> +				return -err;
> +		} else {
> +			int err = sock_error(sk);
> +
> +			if (err)
> +				return err;
>  		}
>  		if (sk->sk_shutdown & RCV_SHUTDOWN)
>  			return 0;

[Severity: Low]
The commit message describes a fix that users can see:

    On the no-data path that check-then-sock_error() window can return 0
    after another thread consumes the error.

All three callers of tls_rx_rec_wait() end the read on a 0 return. In
tls_sw_recvmsg(), for example:

	err = tls_rx_rec_wait(sk, flags & MSG_DONTWAIT,
			      released, !!(decrypted + copied));
	if (err <= 0)
		goto recv_end;

tls_sw_splice_read() and tls_sw_read_sock() do the same. So before this
patch, with nothing copied, userspace could see a spurious EOF while
RCV_SHUTDOWN was not set. One example is when sk_err came from
tls_err_abort().

Should this patch carry Fixes: tags so the fix reaches stable? The recheck
before the RCV_SHUTDOWN test was added by:

Fixes: edcf32b8a48f ("tls: Flush backlog before waiting for a new record")

That commit's stated goal was to report the error "rather than a clean
EOF". The top-of-loop check was last restructured by:

f508262ae9f2 ("tls: Preserve sk_err across recvmsg() when data has been copied")

although its check-then-sock_error() shape is older than that commit.

Also, the commit message says "the outer if (sk_err) is an unmarked load"
in the singular, but the patch rewrites both such blocks in
tls_rx_rec_wait(). Could the message mention both sites?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010-tls-fix-sk-kcsan-err-v3-0-e28aadeb31ad%40proton.me