Re: [PATCH net v2 1/2] net: Make PSP skb extension survive packet scrubbing

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] When re-adding SKB_EXT_PSP fails in skb_ext_scrub()
  (net/core/skbuff.c:6401), the code clears skb->decrypted and lets the…

--

net: Make PSP skb extension survive packet scrubbing

This commit adds a static skb_ext_scrub() helper, used by
skb_scrub_packet() in place of skb_ext_reset(). The helper keeps
SKB_EXT_PSP and drops every other extension. If re-adding the PSP
extension fails, it clears skb->decrypted.

Link: https://lore.kernel.org/r/20260910-bpf-meta-inside-skb-ext-v2-3-0b21e42180b0@xxxxxxxxxxxxxx

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 48f6764117243..6adb54674f1c9 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -6379,6 +6379,32 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
>  }
>  EXPORT_SYMBOL(skb_try_coalesce);
>  
> +static void skb_ext_scrub(struct sk_buff *skb)
> +{
> +#if IS_ENABLED(CONFIG_INET_PSP)
> +	struct psp_skb_ext *pse = skb_ext_find(skb, SKB_EXT_PSP);
> +
> +	if (pse) {
> +		struct skb_ext *ext = skb->extensions;
> +		struct psp_skb_ext tmp = *pse;
> +
> +		/* Nothing else to scrub if PSP is the only extension in ext. */
> +		if (ext->chunks == SKB_EXT_CHUNKSIZEOF(*ext) +
> +				   skb_ext_type_len[SKB_EXT_PSP])
> +			return;
> +
> +		skb_ext_reset(skb);
> +		pse = skb_ext_add(skb, SKB_EXT_PSP);
> +		if (pse)
> +			memcpy(pse, &tmp, sizeof(*pse));
> +		else
> +			skb->decrypted = 0;
                        ^^^^

[Severity: Medium]
If the GFP_ATOMIC allocation in skb_ext_add() fails, can a packet whose
payload is already decrypted lose its decrypted mark and be sent out as
plaintext?

Before this patch, skb_scrub_packet() only called skb_ext_reset(), which
leaves skb->decrypted alone. A HW-decrypted PSP RX packet, for example
from mlx5e_psp_offload_handle_rx_skb(), keeps skb->decrypted = 1. If it
is later forwarded, it is dropped in sk_validate_xmit_skb() in
net/core/dev.c, because forwarded packets have no skb->sk:

	if (sk_validate) {
		skb = sk_validate(sk, dev, skb);
	} else if (unlikely(skb_is_decrypted(skb))) {
		pr_warn_ratelimited("unencrypted skb with no associated socket - dropping\n");
		kfree_skb(skb);
		skb = NULL;
	}

This branch is the path taken when the extension buffer holds something
besides PSP, e.g. a TC_SKB_EXT or nf_bridge extension. On this path, if
__skb_ext_alloc(GFP_ATOMIC) fails, skb->decrypted is cleared but the
payload stays decrypted.

The packet could then reach an egress device by either route:

  skb_do_redirect() (BPF_F_PEER) -> skb_scrub_packet() -> forwarded in
  the peer netns

  ip_tunnel_rcv() -> skb_scrub_packet() -> forwarded

In both cases skb_is_decrypted() would return false, so the drop above
is skipped. On the success path the same packet is still dropped. The
check fails closed today, but on this failure path it would fail open.

Would it be safer to drop the packet, or keep skb->decrypted set, when
the PSP extension cannot be re-added?

> +		return;
> +	}
> +#endif
> +	skb_ext_reset(skb);
> +}
> +

[ ... ]

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