Re: [PATCH net v2 1/2] net: psp: avoid conflicts with skb->decrypted and sk_validate_xmit_skb()

["Daniel Zahka" <daniel.zahka@gmail.com>]

On Wed Sep 16, 2026 at 7:13 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 0 · Low: 1
>
> - [Low] The patch adds a new, user-visible restriction to two already-
>   shipped interfaces without updating Documentation/networking/psp.rst:…
>
> --
>
> net: psp: avoid conflicts with skb->decrypted and sk_validate_xmit_skb()
>
> This makes PSP and TCP ULPs mutually exclusive, since PSP and TLS both use
> skb->decrypted and sk->sk_validate_xmit_skb(). A new helper
> sk_has_decrypt_user() is added and checked from both __tcp_set_ulp() and
> psp_sock_assoc_set_rx().
>
>> diff --git a/net/core/sock.c b/net/core/sock.c
>> index fa60b7494c586..9489d9c479493 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>
> [ ... ]
>
>> @@ -2670,6 +2671,12 @@ void sk_setup_caps(struct sock *sk, struct dst_entry *dst)
>>  }
>>  EXPORT_SYMBOL_GPL(sk_setup_caps);
>>  
>> +bool sk_has_decrypt_user(const struct sock *sk)
>> +{
>> +	return psp_sk_assoc(sk) ||
>> +	       (sk_is_inet(sk) && inet_csk_has_ulp(sk)); /* for tls */
>> +}
>> +
>>  /*
>>   *	Simple resource managers for sockets.
>>   */
>> diff --git a/net/ipv4/tcp_ulp.c b/net/ipv4/tcp_ulp.c
>> index 2aa442128630e..b58045df101e5 100644
>> --- a/net/ipv4/tcp_ulp.c
>> +++ b/net/ipv4/tcp_ulp.c
>> @@ -136,6 +136,10 @@ static int __tcp_set_ulp(struct sock *sk, const struct tcp_ulp_ops *ulp_ops)
>>  	if (icsk->icsk_ulp_ops)
>>  		goto out_err;
>>  
>> +	err = -EINVAL;
>> +	if (sk_has_decrypt_user(sk))
>> +		goto out_err;
>> +
>>  	if (sk->sk_socket)
>>  		clear_bit(SOCK_SUPPORT_ZC, &sk->sk_socket->flags);
>>  
>> diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
>> index 1a2a6b7516b0f..e9b53eedf8dba 100644
>> --- a/net/psp/psp_sock.c
>> +++ b/net/psp/psp_sock.c
>> @@ -143,6 +143,10 @@ int psp_sock_assoc_set_rx(struct sock *sk, struct psp_assoc *pas,
>>  		NL_SET_ERR_MSG(extack, "Socket already has PSP state");
>>  		err = -EBUSY;
>>  		goto exit_unlock;
>> +	} else if (sk_has_decrypt_user(sk)) {
>> +		NL_SET_ERR_MSG(extack, "Socket has incompatible state");
>> +		err = -EINVAL;
>> +		goto exit_unlock;
>>  	}
>
> [Severity: Low]
> Should Documentation/networking/psp.rst be updated along with this?
>
> Two already-shipped interfaces grow a new failure mode here:
> psp_sock_assoc_set_rx() now returns -EINVAL for the rx-assoc netlink
> command, and __tcp_set_ulp() now returns -EINVAL for
> setsockopt(TCP_ULP).
>
> The "Securing a connection" section enumerates the restrictions of the
> rx-assoc/tx-assoc sequence:
>
> 	PSP encryption is currently only supported for TCP connections.
> 	Rx and Tx keys are allocated separately. First the ``rx-assoc``
> 	Netlink command needs to be issued, specifying a target TCP socket.
>
> but does not mention that a socket with a ULP attached is now rejected.
>
> The "User facing API" section also suggests a TLS-then-PSP bootstrap:
>
> 	The expectation is that higher layer protocols will take care of
> 	protocol and key negotiation. For example one may use TLS key exchange,
> 	announce the PSP capability, and switch to PSP if both endpoints
> 	are PSP-capable.
>
> and "Kernel implementation notes" says PSP "follows the TLS offload more
> closely ... and the use of skb->decrypted", with nothing stating that kTLS
> and PSP can no longer share a socket.
>
> One more thing worth spelling out in the docs: the check is not restricted
> to TLS. inet_csk_has_ulp() only tests icsk_ulp_ops, so any registered TCP
> ULP blocks rx-assoc, which is broader than the subject line suggests. The
> commit message explains this is intentional, but nothing in-tree tells
> userspace about it.
>
> The follow-up patch in the series ("selftests: drv-net: psp: test PSP and
> TCP ULP mutual exclusion") adds tests for both directions but does not
> touch Documentation/, so the gap remains at the end of the series.

I can incorporate some changes to psp.rst into a v3 if others think
that improves the quality of this patch and the PSP code. Personally, I
don't think this suggestion is worth while. What this code prevents is
something I would be surprised if a user were to try without heavily
inspecting both the implementations of PSP and TLS ULP first, at which
point I don't know if using up words in this doc is worth it.