Re: [PATCH net-next 3/4] net: psp: drop psp assoc clear in sk_clone()

[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] sk_clone() in net/core/sock.c (line 2499) no longer clears the
  psp_assoc pointer that sock_copy() copies raw into the child.

--

net: psp: drop psp assoc clear in sk_clone()

This patch reverts the unconditional NULL init of newsk->psp_assoc in
sk_clone() that 1d2929d0850f added. It replaces the init with a debug-only
warning. The commit message says listeners can no longer hold PSP
association state, because of 8cc3aef0cb19 and the earlier patch in this
series that limits association setup to established sockets.

> diff --git a/net/core/sock.c b/net/core/sock.c
> index 2948dffcc3e1b..d7ddd365abebb 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2496,7 +2496,7 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
>  	RCU_INIT_POINTER(newsk->sk_bpf_storage, NULL);
>  #endif
>  #if IS_ENABLED(CONFIG_INET_PSP)
> -	RCU_INIT_POINTER(newsk->psp_assoc, NULL);
> +	DEBUG_NET_WARN_ON_ONCE(rcu_access_pointer(sk->psp_assoc));

[Severity: Low]
Would it be worth keeping the RCU_INIT_POINTER() next to the new warning?

I could not find a path in the tree at the end of this series that reaches
this state. So this is about defense in depth, not a live bug.

psp_assoc sits past sk_dontcopy_end, so sock_copy() copies it raw into
newsk. With CONFIG_DEBUG_NET=n, DEBUG_NET_WARN_ON_ONCE() becomes
BUILD_BUG_ON_INVALID() and generates no code. With CONFIG_DEBUG_NET=y it
warns but still leaves the copied pointer in newsk.

If a parent ever did hold an association, each child would share the
pointer without taking a reference. Each child's destructor would then drop
a reference it never took:

inet_sock_destruct()
  psp_sk_assoc_free()
    rcu_assign_pointer(sk->psp_assoc, NULL);
    psp_assoc_put(pas);

This is the refcount underflow and use-after-free of struct psp_assoc that
1d2929d0850f fixed.

That protection now depends on two invariants in other files:

  - psp_sock_assoc_set_rx() and psp_sock_assoc_set_tx() rejecting sockets
    that are not TCP_ESTABLISHED
  - inet_hash() rejecting a non-LISTEN unhashed_state, which blocks
    established -> tcp_disconnect() -> listen()

tcp_disconnect() still leaves sk->psp_assoc set. If either check is relaxed
later, or this patch is backported without them, could the use-after-free
come back with nothing catching it on production builds? The earlier patch
in the series says the established-only restriction "can be reintroduced
when there is a use case".

sk_clone() is a slow path, so keeping the unconditional clear next to the
warning should cost very little. For example:

	DEBUG_NET_WARN_ON_ONCE(rcu_access_pointer(sk->psp_assoc));
	RCU_INIT_POINTER(newsk->psp_assoc, NULL);

Wrapping the clear in if (DEBUG_NET_WARN_ON_ONCE(...)) would not work,
because BUILD_BUG_ON_INVALID() is a void expression when
CONFIG_DEBUG_NET=n.

>  #endif
>  
>  	/* SANITY */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-psp-defeat-v1-0-9f0b430107aa%40gmail.com