Re: [PATCH v1 net] tcp: Don't call skb_clone_and_charge_r() for close()d listener in tcp_v6_do_rcv().

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Incomplete fix / residual TOCTOU race in tcp_v6_do_rcv(): the
  new guard is a state *blacklist* (`!((1 << sk->sk_state) &…

--

tcp: Don't call skb_clone_and_charge_r() for close()d listener in tcp_v6_do_rcv().

The patch extends the guard in tcp_v6_do_rcv() so that
skb_clone_and_charge_r() is skipped for TCP_CLOSE in addition to
TCP_LISTEN, closing the window where a concurrent close() turns a listener
into TCP_CLOSE while the lockless listener path is running.  The commit
message notes this is safe for non-listeners because tcp_rcv_state_process()
drops the skb for TCP_CLOSE.

No Link: tags are present in the commit header.

> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index df9c29eb5c1f4..7fa4ed2fd4f16 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1604,7 +1604,8 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
>  	   by tcp. Feel free to propose better solution.
>  					       --ANK (980728)
>  	 */
> -	if (np->rxopt.all && sk->sk_state != TCP_LISTEN)
> +	if (np->rxopt.all &&
> +	    !((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)))
>  		opt_skb = skb_clone_and_charge_r(skb, sk);

[Severity: High]
Is TCP_CLOSE the only other state the lockless listener path can observe
here?

tcp_v6_rcv() calls tcp_v6_do_rcv() without taking the socket lock when it
samples TCP_LISTEN:

net/ipv6/tcp_ipv6.c:tcp_v6_rcv() {
	...
	if (sk->sk_state == TCP_LISTEN) {
		ret = tcp_v6_do_rcv(sk, skb);
		goto put_and_return;
	}

	sk_incoming_cpu_update(sk);

	bh_lock_sock_nested(sk);
	...
}

Only a socket reference is held across that call, so nothing pins
sk->sk_state while tcp_v6_do_rcv() re-reads it.  Can user space move the
same socket further than TCP_CLOSE inside that window?  inet_shutdown() on
a listener falls through to sk->sk_prot->disconnect(), i.e. tcp_disconnect()
-> tcp_set_state(sk, TCP_CLOSE), and a following connect() on the still-open
fd sets TCP_SYN_SENT.

If TCP_SYN_SENT (or later TCP_ESTABLISHED) is what gets loaded at the new
check, the blacklist passes and skb_clone_and_charge_r() runs with no socket
lock again:

include/net/sock.h:skb_clone_and_charge_r() {
	skb = skb_clone(skb, sk_gfp_mask(sk, GFP_ATOMIC));
	if (skb) {
		if (sk_rmem_schedule(sk, skb, skb->truesize)) {
			skb_set_owner_r(skb, sk);
	...
}

sk_rmem_schedule() and skb_set_owner_r() -> sk_mem_charge() ->
sk_forward_alloc_add() perform a non-atomic read-modify-write of
sk->sk_forward_alloc, which is the same accounting corruption behind the
inet_sock_destruct() "sk->sk_forward_alloc" warning quoted in the commit
message.

Would a positive condition be more robust than extending the blacklist,
for example bailing out of tcp_v6_do_rcv() when the lockless listener path
observes sk->sk_state != TCP_LISTEN?  That would also match the comment at
the top of the function:

	/* The socket must have it's spinlock held when we get
	 * here, unless it is a TCP_LISTEN socket.
	 ...
	 */

On the same window, does the TCP_ESTABLISHED fast path just below become
reachable without the lock as well?

	if (sk->sk_state == TCP_ESTABLISHED) { /* Fast path */
		struct dst_entry *dst;

		dst = rcu_dereference_protected(sk->sk_rx_dst,
						lockdep_sock_is_held(sk));

The lockdep_sock_is_held() assertion, the RCU_INIT_POINTER()/dst_release()
of sk->sk_rx_dst and tcp_rcv_established() all assume the socket lock is
held.

Should the state reads on this path also use READ_ONCE() (or
inet_sk_state_load()) instead of plain loads, given the socket lock is not
held?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914011420.115556-1-kuniyu%40google.com