Re: [PATCH net-next v1 08/15] psp: only give time-wait sockets of psp connections a validator

[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

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).

- [High] psp_twsk_init() (net/psp/psp_sock.c) now sets
  tw->tw_validate_xmit_skb = psp_validate_xmit whenever the full socket…

--

psp: only give time-wait sockets of psp connections a validator

This commit changes psp_twsk_init() so that tw_validate_xmit_skb is set
only when the full socket has a psp association, not on every tcp
time-wait socket. It prepares for the upcoming tc psp action, which
treats a socket that has a validator as having its own encryption
offload.

> diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
> index 609b06c7af65b..664955bfb4fdb 100644
> --- a/net/psp/psp_sock.c
> +++ b/net/psp/psp_sock.c
> @@ -330,10 +330,14 @@ void psp_twsk_init(struct inet_timewait_sock *tw, const struct sock *sk)
>  {
>  	struct psp_assoc *pas = psp_sk_assoc(sk);
>  
> -	if (pas)
> +	if (pas) {
>  		refcount_inc(&pas->refcnt);
> +		/* Only for PSP connections: a validator makes the socket
> +		 * look like it has an encryption offload of its own
> +		 */
> +		tw->tw_validate_xmit_skb = psp_validate_xmit;

[Severity: High]
Should this also check pas->tx.spi? As written, an Rx-only association
(pas->tx.spi == 0) also gets the validator here.

On a full socket, the validator is installed only after a Tx key is set,
in psp_sock_set_tx_key():

    WRITE_ONCE(sk->sk_validate_xmit_skb, psp_validate_xmit);

psp_sock_assoc_set_rx() publishes sk->psp_assoc but never sets
sk->sk_validate_xmit_skb. A connection that did rx-assoc but never
tx-assoc therefore has no validator while it is a full socket. It gains
one when it enters TIME_WAIT.

Other places that decide whether a connection is PSP-encrypted test
pas && pas->tx.spi. These include psp_reply_set_decrypted() in this same
file:

    if (pas && pas->tx.spi)
        skb->decrypted = 1;

and also psp_enqueue_set_decrypted() and psp_sk_overhead() in
include/net/psp/functions.h.

With the act_psp patch later in the series, tcf_psp_sk_offload() returns
true for a TIME_WAIT socket that has a tw_validate_xmit_skb:

    return sk_is_inet(sk) && sk->sk_state == TCP_TIME_WAIT &&
           inet_twsk(sk)->tw_validate_xmit_skb;

tcf_psp_act() then drops the packet with SKB_DROP_REASON_PSP_OUTPUT. In
addition, sk_validate_xmit_skb() in net/core/dev.c drops skbs carrying a
PSP metadata dst when the time-wait socket has a validator.

TIME_WAIT replies get skb->sk = tw through skb_set_owner_edemux() from
ip_send_unicast_reply() and tcp_v6_send_response(). The resulting
sequence looks like this:

  psp_sock_assoc_set_rx()       sk->psp_assoc set, tx.spi == 0, no validator
  connection closes
  psp_twsk_init()               tw_validate_xmit_skb set since pas != NULL
  TW ACK/RST reply, skb->sk = tw
  tcf_psp_act() ENCAP
    tcf_psp_sk_offload()        true -> TC_ACT_SHOT

So an egress psp encap rule that covers such a connection encapsulates
its packets while the socket is full. Once the socket is in TIME_WAIT,
the same rule drops its ACK and RST replies. Is this the inconsistency
the commit message describes? It seems to remain for Rx-only
associations.

An Rx-only socket has no Tx offload of its own.
Documentation/networking/psp.rst lists Tx as plain text in the Rx PSP
state.

Would gating on pas && pas->tx.spi, as psp_reply_set_decrypted() does,
match the full-socket rule? Copying the validator only when
sk->sk_validate_xmit_skb == psp_validate_xmit would be another option.

> +	}
>  	rcu_assign_pointer(tw->psp_assoc, pas);
> -	tw->tw_validate_xmit_skb = psp_validate_xmit;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111005533.6436-1-toddfreed%40meta.com