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 Signed-off-by: Jamal Hadi Salim --- include/net/xfrm.h | 2 -- net/core/filter.c | 8 -------- net/xfrm/xfrm_interface_bpf.c | 16 +++++----------- net/xfrm/xfrm_interface_core.c | 22 ++++++++++++---------- 4 files changed, 17 insertions(+), 31 deletions(-) diff --git a/include/net/xfrm.h b/include/net/xfrm.h index 445223a8dc6f..1866e5a3b911 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -2370,8 +2370,6 @@ static inline bool xfrm6_local_dontfrag(const struct sock *sk) #if (IS_BUILTIN(CONFIG_XFRM_INTERFACE) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) || \ (IS_MODULE(CONFIG_XFRM_INTERFACE) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES)) -extern struct metadata_dst __percpu *xfrm_bpf_md_dst; - int register_xfrm_interface_bpf(void); #else diff --git a/net/core/filter.c b/net/core/filter.c index f42ecfb5841f..bd98d29ed7f8 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -6354,14 +6354,6 @@ static const struct bpf_func_proto bpf_bind_proto = { #ifdef CONFIG_XFRM -#if (IS_BUILTIN(CONFIG_XFRM_INTERFACE) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) || \ - (IS_MODULE(CONFIG_XFRM_INTERFACE) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES)) - -struct metadata_dst __percpu *xfrm_bpf_md_dst; -EXPORT_SYMBOL_GPL(xfrm_bpf_md_dst); - -#endif - BPF_CALL_5(bpf_skb_get_xfrm_state, struct sk_buff *, skb, u32, index, struct bpf_xfrm_state *, to, u32, size, u64, flags) { diff --git a/net/xfrm/xfrm_interface_bpf.c b/net/xfrm/xfrm_interface_bpf.c index 5ea15037ebd1..8e83c624b774 100644 --- a/net/xfrm/xfrm_interface_bpf.c +++ b/net/xfrm/xfrm_interface_bpf.c @@ -68,16 +68,9 @@ __bpf_kfunc int bpf_skb_set_xfrm_info(struct __sk_buff *skb_ctx, const struct bp if (unlikely(skb_metadata_dst(skb))) return -EINVAL; - if (!xfrm_bpf_md_dst) { - struct metadata_dst __percpu *tmp; - - tmp = metadata_dst_alloc_percpu(0, METADATA_XFRM, GFP_ATOMIC); - if (!tmp) - return -ENOMEM; - if (cmpxchg(&xfrm_bpf_md_dst, NULL, tmp)) - metadata_dst_free_percpu(tmp); - } - md_dst = this_cpu_ptr(xfrm_bpf_md_dst); + md_dst = metadata_dst_alloc(0, METADATA_XFRM, GFP_ATOMIC); + if (unlikely(!md_dst)) + return -ENOMEM; info = &md_dst->u.xfrm_info; @@ -86,7 +79,8 @@ __bpf_kfunc int bpf_skb_set_xfrm_info(struct __sk_buff *skb_ctx, const struct bp skb_dst_force(skb); info->dst_orig = skb_dst(skb); - dst_hold((struct dst_entry *)md_dst); + /* The allocated dst owns one reference, handed to the skb here. */ + skb_dstref_steal(skb); skb_dst_set(skb, (struct dst_entry *)md_dst); return 0; } diff --git a/net/xfrm/xfrm_interface_core.c b/net/xfrm/xfrm_interface_core.c index 9835fbe63ccb..896180f8d171 100644 --- a/net/xfrm/xfrm_interface_core.c +++ b/net/xfrm/xfrm_interface_core.c @@ -423,6 +423,7 @@ static int xfrmi_rcv_cb(struct sk_buff *skb, int err) static int xfrmi_xmit2(struct sk_buff *skb, struct net_device *dev, struct flowi *fl) { + struct xfrm_md_info *md_info = skb_xfrm_md_info(skb); struct xfrm_if *xi = netdev_priv(dev); struct dst_entry *dst = skb_dst(skb); unsigned int length = skb->len; @@ -433,25 +434,26 @@ xfrmi_xmit2(struct sk_buff *skb, struct net_device *dev, struct flowi *fl) int mtu; if (xi->p.collect_md) { - struct xfrm_md_info *md_info = skb_xfrm_md_info(skb); - if (unlikely(!md_info)) return -EINVAL; if_id = md_info->if_id; fl->flowi_oif = md_info->link; - if (md_info->dst_orig) { - struct dst_entry *tmp_dst = dst; - - dst = md_info->dst_orig; - skb_dst_set(skb, dst); - md_info->dst_orig = NULL; - dst_release(tmp_dst); - } } else { if_id = xi->p.if_id; } + /* The metadata dst carries the route the packet is still to be + * transformed for, and the lookup below takes the address family from + * the dst it is given. Put that route back, taking our own reference: + * the metadata dst keeps its own, as a clone of this skb shares it. + */ + if (md_info && md_info->dst_orig) { + dst = dst_clone(md_info->dst_orig); + skb_dst_drop(skb); + skb_dst_set(skb, dst); + } + dst_hold(dst); dst = xfrm_lookup_with_ifid(xi->net, dst, fl, NULL, 0, if_id); if (IS_ERR(dst)) { -- 2.43.0