Re: [PATCH net-next v2 1/4] net: xfrm: interface: restore dst_orig on every xfrm interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] xfrm: interface: missing Fixes trailer in bug-fix commit message
--

commit 07b565180a0d355b65e18ab188e00cf30014d9c1
Author: Jamal Hadi Salim <jhs@mojatatu.com>

> net: xfrm: interface: restore dst_orig on every xfrm interface
>
> Found while testing the dst release for the two producers that steer a
> packet to another device, fixed later in this series. One of the consumers
> of the metadata such a release installs is an xfrm interface, which
> restores the original route before the xfrm lookup; the swap warned on the
> metadata dst still installed in the skb, and only the collect_md arm
> restored the route at all.
>
> The issue:
> An xfrm interface takes the if_id and the underlying device either from its
> configuration or, in collect_md mode, from the xfrm metadata on the skb.
> When that metadata also carries the route the packet arrived through,
> xfrmi_xmit2() puts the route back on the skb, because the xfrm lookup takes
> the address family from it. skb_dst_set() expects the dst slot to be empty
> for a reference counted dst and warns when it is not, so the route landed
> over a metadata dst that was still there; a kernel built with
> CONFIG_DEBUG_NET=y panics on the first packet that takes this path when
> panic_on_warn is set.
>
> Two more defects sit in the same place. An interface with a configured
> if_id never restores the route: it takes the metadata dst as it is, whose
> address family is AF_UNSPEC, so the lookup finds no policy and the packet
> is dropped as a link failure instead of being transformed. And the restore
> moved the route out of the metadata dst, which a clone of the skb shares,
> so the clone that reached the interface first left the original packet with
> nothing to restore.
>
> The same slot overwrite is in the unstable helper a tc-bpf program uses to
> attach that metadata: bpf_skb_set_xfrm_info() records the route as dst_orig
> and installs a per-cpu metadata dst over it. The per-cpu dst is shared by
> every invocation of the program, so the restore cannot keep anything in it.
>
> The Fix:
> Restore dst_orig in both arms of xfrmi_xmit2(), taking a reference with
> dst_clone() so the metadata dst keeps its own, and empty the counted slot
> with skb_dst_drop() before the route is installed. The helper allocates a
> metadata dst per skb instead of a per-cpu one, so the route it records
> belongs to that skb and is released with it; the per-cpu allocation is no
> longer used and goes away.
>
> Conditions to recreate the bug:
> A tc-bpf program that records the route as dst_orig with
> bpf_skb_set_xfrm_info() and redirects the packet to an xfrm interface is
> the shortest way to hit the overwrite. A netdev egress fwd rule that steers
> a packet routed with an xfrm encapsulation into an interface reaches the
> same place once the dst release earlier in this series keeps the route:
>
>     ip link add xf0 type xfrm if_id 42
>     ip link set xf0 up
>     ip neigh replace 10.20.0.1 lladdr 02:aa:00:00:00:ab \
>             dev va nud permanent
>     ip route add 10.20.0.1/32 dev va encap xfrm if_id 42
>     nft add rule netdev t c fwd ip to 10.1.0.2 device xf0
>
> Send one packet to 10.20.0.1. With CONFIG_DEBUG_NET=y and panic_on_warn=1
> the kernel panics in xfrmi_xmit() with the metadata dst still installed
> while the route is put back. The interface with the configured if_id drops
> the packet as a link failure without consulting the outbound policy, and a
> clone made after the metadata was attached consumes the route before the
> original packet arrives.
>
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

[Severity: Low]
Since this fixes the dst_orig handling and per-cpu metadata dst added in
bpf_skb_set_xfrm_info() and xfrmi_xmit2(), should this include:

  Fixes: 94151f5aa966 ("xfrm: interface: Add unstable helpers for setting/getting XFRM metadata from TC-BPF")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/CORE-9NOG.v2.r0.20261112151659@mojatatu.com?part=1