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

[netdev-bot+sashiko@kernel.org]

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