[PATCH net-next v3 0/2] net: annotate remaining lockless sk->sk_err accesses

[PATCH net-next v3 0/2] net: annotate remaining lockless sk->sk_err accesses

[Quanye Yang via B4 Relay]

The TCP/MPTCP series that annotated lockless sk_err peeks and
consumes is in net-next. Paolo asked for the same treatment on
kTLS and on the unmarked writers that still race those readers.

do_recvmmsg() and getsockopt(SO_ERROR) still call sock_error()
without the socket lock. kTLS is a ULP on that same struct sock,
so tls_rx_rec_wait() has the same peek-versus-consume split, and
the send path still does a double unmarked load.

sock_dequeue_err_skb() can store sk_err from MSG_ERRQUEUE before
lock_sock(). strp_abort_strp() and sk_psock_report_error() write
the same field on the TCP/TLS and sockmap paths.

Patch 1 annotates the TLS readers and consumes sk_err once on the
no-data path. Patch 2 pairs the remaining writers with WRITE_ONCE().
No extra ordering is added; ICMP error-queue overwrite semantics
are unchanged.

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

Signed-off-by: Quanye Yang <quanyeyang@proton.me>
---
Changes in v3:
- rebase on net-next after the net merge
- refresh WRITE_ONCE() in strp_abort_strp() after the strparser
  refactor (Jakub Kicinski)
- Patch 2: carry Reviewed-by from Eric Dumazet
- Patch 1: unchanged
- Link to v2: https://patch.msgid.link/20261003-tls-fix-sk-kcsan-err-v2-0-e939a2ebdd56@proton.me

Changes in v2:
- tls_encrypt_done(): fold the three unmarked sk_err loads into one
  READ_ONCE()
- Link to v1: https://patch.msgid.link/20261002-tls-fix-sk-kcsan-err-v1-0-baa0ba056323@proton.me

---
Quanye Yang (2):
      tls: annotate lockless access to sk->sk_err
      net: annotate lockless writes to sk->sk_err

 include/linux/skmsg.h     |  2 +-
 net/core/skbuff.c         |  5 +++--
 net/strparser/strparser.c |  2 +-
 net/tls/tls_device.c      |  5 +++--
 net/tls/tls_sw.c          | 54 +++++++++++++++++++++++++++++++----------------
 5 files changed, 44 insertions(+), 24 deletions(-)
---
base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
change-id: 20261002-tls-fix-sk-kcsan-err-6fef32744f15

Best regards,
--  
Quanye Yang <quanyeyang@proton.me>

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

[Quanye Yang via B4 Relay]

From: Quanye Yang <quanyeyang@proton.me>

kTLS sits on the same struct sock as TCP. do_recvmmsg() and
getsockopt(SO_ERROR) still call sock_error() without the socket lock
and clear sk_err with xchg().

tls_rx_rec_wait() already peeks when data has been copied and consumes
otherwise, but the outer if (sk_err) is an unmarked load. On the
no-data path that check-then-sock_error() window can return 0 after
another thread consumes the error. Call sock_error() once and only
return when it is non-zero; keep READ_ONCE() on the peek path.

tls_sw_sendmsg_locked(), tls_push_data(), bpf_exec_tx_verdict() and
tls_encrypt_done() read sk_err more than once. Fold those unmarked loads
into one READ_ONCE() and use that value as the returned errno. The field is
still not consumed there.

Link: https://lore.kernel.org/netdev/3d9d442f-f168-43da-87b0-010ad5a78365@redhat.com/
Signed-off-by: Quanye Yang <quanyeyang@proton.me>
---
 net/tls/tls_device.c |  5 +++--
 net/tls/tls_sw.c     | 54 ++++++++++++++++++++++++++++++++++------------------
 2 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index f11d0528fc43..03ce83a9d4e9 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -444,8 +444,9 @@ static int tls_push_data(struct sock *sk,
 	if ((flags & (MSG_MORE | MSG_EOR)) == (MSG_MORE | MSG_EOR))
 		return -EINVAL;
 
-	if (unlikely(sk->sk_err))
-		return -sk->sk_err;
+	rc = -READ_ONCE(sk->sk_err);
+	if (unlikely(rc))
+		return rc;
 
 	flags |= MSG_SENDPAGE_DECRYPTED;
 	tls_push_record_flags = flags | MSG_MORE;
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 312e51270f29..a2be1283ca93 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -473,6 +473,7 @@ static void tls_encrypt_done(void *data, int err)
 	struct scatterlist *sge;
 	struct sk_msg *msg_en;
 	struct sock *sk;
+	int skerr;
 
 	if (err == -EINPROGRESS) /* see the comment in tls_decrypt_done() */
 		return;
@@ -488,13 +489,15 @@ static void tls_encrypt_done(void *data, int err)
 	sge->offset -= prot->prepend_size;
 	sge->length += prot->prepend_size;
 
+	skerr = READ_ONCE(sk->sk_err);
+
 	/* Check if error is previously set on socket */
-	if (err || sk->sk_err) {
+	if (err || skerr) {
 		rec = NULL;
 
 		/* If err is already set on socket, return the same code */
-		if (sk->sk_err) {
-			ctx->async_wait.err = -sk->sk_err;
+		if (skerr) {
+			ctx->async_wait.err = -skerr;
 		} else {
 			ctx->async_wait.err = err;
 			tls_err_abort(sk, err);
@@ -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;
 }
@@ -800,10 +807,9 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
 	}
 
 	while (msg_data_left(msg)) {
-		if (sk->sk_err) {
-			ret = -sk->sk_err;
+		ret = -READ_ONCE(sk->sk_err);
+		if (ret)
 			goto send_end;
-		}
 
 		if (ctx->open_rec)
 			rec = ctx->open_rec;
@@ -1107,10 +1113,16 @@ tls_rx_rec_wait(struct sock *sk, bool nonblock, bool released, bool has_copied)
 	timeo = sock_rcvtimeo(sk, nonblock);
 
 	while (!tls_strp_msg_ready(ctx)) {
-		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 (ret < 0)
@@ -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;

-- 
2.55.0

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

[netdev-bot+sashiko]

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

[PATCH net-next v3 2/2] net: annotate lockless writes to sk->sk_err

[Quanye Yang via B4 Relay]

From: Quanye Yang <quanyeyang@proton.me>

do_recvmmsg() and getsockopt(SO_ERROR) clear sk_err with xchg()
without the socket lock. TCP, MPTCP and kTLS already peek the same
field with READ_ONCE() or consume it via sock_error().

sock_dequeue_err_skb() still uses plain stores. tcp_recvmsg() can
call it via MSG_ERRQUEUE before lock_sock(), so those writes race
with the annotated readers and with sock_error(). The same unmarked
stores exist in strp_abort_strp() and sk_psock_report_error(), which
run on the TCP/TLS socket.

Annotate those writers with WRITE_ONCE(). No extra ordering is
needed; this does not change who wins when ICMP error-queue entries
overwrite sk_err.

Link: https://lore.kernel.org/netdev/3d9d442f-f168-43da-87b0-010ad5a78365@redhat.com/
Signed-off-by: Quanye Yang <quanyeyang@proton.me>
Reviewed-by: Eric Dumazet <edumazet@kernel.org>
---
 include/linux/skmsg.h     | 2 +-
 net/core/skbuff.c         | 5 +++--
 net/strparser/strparser.c | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index d5e35f24738d..52ce45f25f5a 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -429,7 +429,7 @@ static inline void sk_psock_report_error(struct sk_psock *psock, int err)
 {
 	struct sock *sk = psock->sk;
 
-	sk->sk_err = err;
+	WRITE_ONCE(sk->sk_err, err);
 	sk_error_report(sk);
 }
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d56f4f0102f7..a6c9cf57bc3b 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5539,12 +5539,13 @@ struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
 	if (skb && (skb_next = skb_peek(q))) {
 		icmp_next = is_icmp_err_skb(skb_next);
 		if (icmp_next)
-			sk->sk_err = SKB_EXT_ERR(skb_next)->ee.ee_errno;
+			WRITE_ONCE(sk->sk_err,
+				   SKB_EXT_ERR(skb_next)->ee.ee_errno);
 	}
 	spin_unlock_irqrestore(&q->lock, flags);
 
 	if (is_icmp_err_skb(skb) && !icmp_next)
-		sk->sk_err = 0;
+		WRITE_ONCE(sk->sk_err, 0);
 
 	if (skb_next)
 		sk_error_report(sk);
diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index 9c1559d7923f..5f1da32b152f 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -56,7 +56,7 @@ static void strp_abort_strp(struct strparser *strp, int err)
 	strp->need_bytes = 0;
 
 	/* Report an error on the lower socket */
-	sk->sk_err = -err;
+	WRITE_ONCE(sk->sk_err, -err);
 	sk_error_report(sk);
 }
 

-- 
2.55.0

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch does not annotate every sk_err writer on the TCP
  socket that strparser uses.

Pre-existing issues:
- [Medium] In net/kcm/kcmsock.c, report_csk_error(struct sock *csk, int
  err) ignores its `err` argument and always stores EPIPE (line 54).
- [Low] The inet_sk_error_report tracepoint
  (include/trace/events/sock.h:215) copies sk->sk_err with a plain load:…

--

net: annotate lockless writes to sk->sk_err

This commit changes the plain stores to sk->sk_err in
sock_dequeue_err_skb(), strp_abort_strp() and sk_psock_report_error() to
WRITE_ONCE(). sock_error() and getsockopt(SO_ERROR) clear the field with
xchg() without the socket lock, and TCP, MPTCP and kTLS read it with
READ_ONCE().

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

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index d56f4f0102f75..a6c9cf57bc3bf 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5539,12 +5539,13 @@ struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
>  	if (skb && (skb_next = skb_peek(q))) {
>  		icmp_next = is_icmp_err_skb(skb_next);
>  		if (icmp_next)
> -			sk->sk_err = SKB_EXT_ERR(skb_next)->ee.ee_errno;
> +			WRITE_ONCE(sk->sk_err,
> +				   SKB_EXT_ERR(skb_next)->ee.ee_errno);
>  	}
>  	spin_unlock_irqrestore(&q->lock, flags);
>  
>  	if (is_icmp_err_skb(skb) && !icmp_next)
> -		sk->sk_err = 0;
> +		WRITE_ONCE(sk->sk_err, 0);
>  
>  	if (skb_next)
>  		sk_error_report(sk);

[Severity: Low]
This is a pre-existing issue, but sk_error_report() here runs after q->lock
has been dropped. For AF_INET/AF_INET6 sockets it calls the
inet_sk_error_report tracepoint, and that tracepoint reads the field with a
plain load:

include/trace/events/sock.h:inet_sk_error_report {
    ...
	__entry->error = sk->sk_err;
    ...
}

When tracing is enabled, can this race with sock_error()'s xchg() or with
the WRITE_ONCE() writers on another CPU? The worst result looks like a
stale errno in the trace event. Should this read also be a READ_ONCE()?

[ ... ]

> diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
> index 9c1559d7923f2..5f1da32b152fc 100644
> --- a/net/strparser/strparser.c
> +++ b/net/strparser/strparser.c
> @@ -56,7 +56,7 @@ static void strp_abort_strp(struct strparser *strp, int err)
>  	strp->need_bytes = 0;
>  
>  	/* Report an error on the lower socket */
> -	sk->sk_err = -err;
> +	WRITE_ONCE(sk->sk_err, -err);
>  	sk_error_report(sk);
>  }

[Severity: Medium]
Does this cover every writer on the TCP sockets that strparser runs on?

KCM attaches the strparser to the same lower TCP socket:

net/kcm/kcmsock.c:kcm_attach() {
    ...
	err = strp_init(&psock->strp, csk, &cb);
    ...
	csk->sk_state_change = psock_state_change;
    ...
}

KCM still reports errors on that socket with a plain store:

net/kcm/kcmsock.c:report_csk_error() {
	csk->sk_err = EPIPE;
	sk_error_report(csk);
}

Two paths reach this store:

  kcm_abort_tx_psock(), after mux->lock is dropped (callers are
  kcm_write_msgs(), kcm_unattach() and kcm_release())

  psock_state_change(), which is the csk->sk_state_change callback

None of these paths holds the lock of the lower TCP socket. Userspace still
holds the TCP fd, so getsockopt(SO_ERROR) or recvmmsg() can xchg() the
field at the same time, while tcp_poll() reads it with READ_ONCE().

Should report_csk_error() get the same WRITE_ONCE() treatment?

smc_clc_wait_msg() has similar plain stores to the TCP clcsock's sk_err.
They run after sock_recvmsg() returns, without the socket lock:

net/smc/smc_clc.c:smc_clc_wait_msg() {
    ...
		clc_sk->sk_err = EINTR;
    ...
			clc_sk->sk_err = 0; /* reset for fallback usage */
    ...
}

UDP, raw and ping sockets also use sock_dequeue_err_skb(), through
ip_recv_error()/ipv6_recv_error(). On those sockets the other writers are
still plain stores. Examples are __udp4_lib_err(), udp_abort(),
__udp6_lib_err(), raw_err(), raw_abort(), the IPv6 raw error path in
net/ipv6/raw.c, and ping_err().

The subject line reads as a general annotation of lockless sk_err writes,
but the body doesn't say that the conversion is partial. Could the commit
message say what is left for follow-ups?

[Severity: Medium]
This isn't a bug introduced by this patch, but report_csk_error() above
never uses its err argument and always stores EPIPE.

kcm_tx_work() logs the real error and passes it in:

net/kcm/kcmsock.c:kcm_tx_work() {
    ...
		pr_warn("KCM: Hard failure on kcm_write_msgs %d\n", err);
		report_csk_error(&kcm->sk, -err);
    ...
}

kcm_sendmsg() and kcm_abort_tx_psock() pass their error values the same
way, and those values are dropped too.

Is it intended that userspace always sees EPIPE through SO_ERROR here,
rather than the actual transmit error? This behaviour goes back to the
original KCM code.

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