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

[netdev-bot+sashiko@kernel.org]

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

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

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] skb_ext_scrub() keeps SKB_EXT_PSP for every caller of
  skb_scrub_packet().
- [Low] On the slow path, skb_ext_scrub() frees the whole extension block
  with skb_ext_reset() before it re-creates SKB_EXT_PSP with…

--

net: Make PSP skb extension survive packet scrubbing

This patch adds a static skb_ext_scrub() helper, called from
skb_scrub_packet(). It keeps SKB_EXT_PSP and drops every other skb
extension. The goal is to keep the PSP metadata on packets redirected into
a netns associated with a PSP device via PSP_CMD_DEV_ASSOC, so that
__psp_sk_rx_policy_check() stops rejecting them.

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

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 8da53d5b76fc2..3f8dc08b434fa 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -6379,6 +6379,31 @@ 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) {

[Severity: Medium]

Is SKB_EXT_PSP meant to be kept for every caller of skb_scrub_packet()?
The commit message only argues for one case: a netkit redirect into a
netns associated through PSP_CMD_DEV_ASSOC.

skb_scrub_packet() is also called from:

  - same-netns tunnel decap: ip_tunnel_rcv(), __iptunnel_pull_header()
    for vxlan/geneve, __ip6_tnl_rcv()
  - tunnel encap on TX: iptunnel_xmit(), vxlan_xmit_one(),
    geneve_build_skb()
  - dev_forward_skb(), veth and ipvlan, into any netns
  - skb_do_redirect()

Nothing here checks xnet, the direction, or whether the target netns or
device is associated with pse->dev_id.

psp_dev_rcv() copies the next header into the outer IP header without
checking it:

net/psp/psp_main.c:psp_dev_rcv() {
    ...
	iph->protocol = psph->nexthdr;
    ...
}

So the decrypted payload can be IPIP, GRE or UDP/VXLAN. Once a tunnel
driver decapsulates it, the inner packet now inherits the outer packet's
PSP metadata.

__psp_sk_rx_policy_check() decides based on whether the extension is
present:

include/net/psp/functions.h:__psp_sk_rx_policy_check() {
    ...
	if (!pas)
		return pse ? SKB_DROP_REASON_PSP_INPUT : 0;
    ...
}

Doesn't this change two outcomes?

First, inner or forwarded traffic to a non-PSP socket was accepted before.
That applies both to a non-associated netns and to traffic behind a
tunnel. It would now be dropped with SKB_DROP_REASON_PSP_INPUT.

Second, inner tunnelled TCP segments to a PSP socket were rejected before.
They would now be accepted when spi, dev_id, generation and version match.

The skb_scrub_packet() kernel-doc still says that for xnet we "have to
clear all information in the skb that could impact namespace isolation".
The new note only says the PSP skb extension is "preserved across packet
scrubbing by design".

Should preservation be limited to the associated-netns case? If not,
could the reasoning for the tunnel decap and non-associated netns cases
be documented?

> +		struct skb_ext *ext = skb->extensions;
> +		struct psp_skb_ext tmp;
> +
> +		/* 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;
> +
> +		tmp = *pse;
> +		skb_ext_reset(skb);
> +		pse = skb_ext_add(skb, SKB_EXT_PSP);
> +		if (pse)
> +			memcpy(pse, &tmp, sizeof(*pse));
> +		return;

[Severity: Low]

What happens to the PSP metadata if skb_ext_add() fails here?

skb_ext_reset() frees the whole extension block first. With
active_extensions now 0, skb_ext_add() goes through
__skb_ext_alloc(GFP_ATOMIC), which can fail under memory pressure. If it
does, the PSP extension is lost without any signal and the packet keeps
going.

That seems to contradict the new kernel-doc line "PSP skb extension is
preserved across packet scrubbing by design".

It also differs from psp_dev_rcv(), which treats the same failure as
fatal:

net/psp/psp_main.c:psp_dev_rcv() {
    ...
	pse = skb_ext_add(skb, SKB_EXT_PSP);
	if (!pse)
		return -EINVAL;
    ...
}

Its callers, mlx5e_psp_offload_handle_rx_skb() and nsim_do_psp(), then
drop the packet.

If the allocation fails here, a data segment for a PSP socket would later
be dropped in __psp_sk_rx_policy_check() with SKB_DROP_REASON_PSP_INPUT.
That is the symptom this patch is meant to fix, and the drop reason would
be misleading.

Could the replacement be allocated before the old block is released? Or
should the new comment be reworded so it does not promise unconditional
preservation?

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

[ ... ]

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