Re: [PATCH net] bridge: skip generic XDP on locally re-injected packets

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

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

- [High] net/bridge/br_input.c:35 — br_netif_receive_skb()
  unconditionally calls skb_set_redirected_noclear(skb, false) on every…
- [Medium] The changelog's causal chain is not supported by the code it
  points at.
- [Low] The commit fixes a user-visible KASAN slab-use-after-free
  reported by syzbot (Reported-by:…
- [Low] The new call abuses the documented meaning of two skb bits.

--

bridge: skip generic XDP on locally re-injected packets

This marks locally delivered bridge frames as redirected in
br_netif_receive_skb() so that netif_receive_generic_xdp() bails out early
and does not run generic XDP on the bridge device.  The stated goal is to
avoid the pskb_expand_head() head reallocation inside
netif_skb_check_for_xdp() that syzbot reported as a use-after-free in
br_handle_frame().

Link: https://lore.kernel.org/all/6a6d4406.2d659fcc.1d46f5.01ad.GAE@google.com/T/ (local)

A few questions about the changelog first.

    If the
    bridge device has an XDP program attached, __netif_receive_skb_core()
    runs do_xdp_generic() a second time on such packets.

[Severity: High]
Is "a second time" accurate here?  do_xdp_generic() is called with the
program of the current skb->dev:

net/core/dev.c:__netif_receive_skb_core() {
	...
	ret2 = do_xdp_generic(rcu_dereference(skb->dev->xdp_prog),
			      &skb);
	...
}

br_pass_frame_up() already switched skb->dev to the bridge master:

net/bridge/br_input.c:br_pass_frame_up() {
	...
	indev = skb->dev;
	skb->dev = brdev;
	...
}

So the program being run in the second round is the one attached to br0,
not the one that ran on the ingress port.  When no XDP program is attached
to the port (the common case) this is the first and only invocation of the
bridge's program.

    netif_receive_generic_xdp() already refuses to run generic XDP on
    reinjected packets by checking skb_is_redirected().  Reuse that marker:
    set it right before the bridge re-injects the packet, so generic XDP is
    skipped and the head buffer is left intact.

[Severity: Medium]
Does the described causal chain hold?  The changelog says the realloc
"frees the head that the bridge rx path (br_handle_frame() /
br_handle_frame_finish()) is still using", but in br_input.c every header
dereference in those functions happens before the handoff.

br_handle_frame() caches the destination up front and last uses it right
before handing the skb off:

net/bridge/br_input.c:br_handle_frame() {
	...
	if (ether_addr_equal(p->br->dev->dev_addr, dest))
		skb->pkt_type = PACKET_HOST;

	return nf_hook_bridge_pre(skb, pskb);
	...
}

br_handle_frame_finish() does all its eth_hdr()/br_fdb_find_rcu() work
before "return br_pass_frame_up(...)", and br_pass_frame_up() only returns
the NF_HOOK() result without touching the skb again.

The reported fault site is also at the very top of br_handle_frame():

	if (!is_valid_ether_addr(eth_hdr(skb)->h_source))

which suggests the head was already freed before that invocation started,
rather than by a realloc happening further down the same call chain.  Could
the stale reference belong to some other holder, with this patch only
quieting the reproducer?  Was a "#syz test" run done to confirm?

    Reported-by: syzbot+128e9f5a0f85a51215b1@syzkaller.appspotmail.com
    Link: https://lore.kernel.org/all/6a6d4406.2d659fcc.1d46f5.01ad.GAE@google.com/T/ (local)
    Signed-off-by: Zhao ShiRong [off-list ref]

[Severity: Low]
Should this carry a Fixes: tag and Cc: stable@vger.kernel.org?  This is a
memory-safety fix for a syzbot-reported use-after-free, but only
Reported-by:/Link: are present, so there is no mechanical way for
backporters to determine the affected range.  A candidate for Fixes: would
be whichever commit made generic XDP reachable on this reinjection path
(the generic-XDP-on-virtual-devices support, or the later
netif_skb_check_for_xdp() head expansion in the receive core).

[Severity: High]
Does this disable generic XDP on bridge devices entirely?  The bit is set
unconditionally for every locally delivered frame, and
netif_receive_generic_xdp() returns before running the program:

net/core/dev.c:netif_receive_generic_xdp() {
	/* Reinjected packets coming from act_mirred or similar should
	 * not get XDP generic processing.
	 */
	if (skb_is_redirected(skb))
		return XDP_PASS;
	...
}

Forwarded frames leave the bridge through br_forward()/dev_queue_xmit() and
never enter netif_receive_skb() on the bridge device, so locally delivered
frames look like the only traffic a generic XDP program on br0 can see.
Before this change the bit was 0 at that point, because the outer round's
skb_reset_redirect() in __netif_receive_skb_core() runs before the bridge
rx_handler, so the program did run.

Would an XDP_DROP policy attached to br0 stop being enforced after this
patch, for ordinary remote unicast/broadcast/multicast traffic entering a
bridge port?  If the underlying problem is missing headroom on TX-allocated
skbs and a stale head reference, would fixing that (giving the reinjected
skb the headroom generic XDP requires, or dropping the stale reference) be
preferable to suppressing the program?

[Severity: Low]
Separately, do the values written here match the documented meaning of the
two bits?

include/linux/skbuff.h:
 *	@redirected: packet was redirected by packet classifier
 *	@from_ingress: packet was redirected from the ingress path

No packet classifier redirected this frame, and from_ingress is passed as
false while the frame is being pushed into the ingress path.  Other in-tree
setters pass skb_at_tc_ingress(skb) (net/core/filter.c) or true for
MLX5E_TC_INT_PORT_INGRESS (drivers/net/ethernet/mellanox/mlx5/core/en/tc/int_port.c).
skb_set_redirected_noclear() does store the value under CONFIG_NET_REDIRECT,
and drivers/net/ifb.c branches on it.

Also, is there a reason to pick the _noclear variant?  The two helpers
differ only in the "if (skb->from_ingress) skb_clear_tstamp(skb);" step,
which is skipped anyway when from_ingress is false, so the choice does not
express anything different from skb_set_redirected().

 	return netif_receive_skb(skb);
 }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831113051.13072-1-shxzhaosr%40163.com