Re: [PATCH net-next 1/2] netfilter: nft_fwd_netdev: drop the stale dst when steering to a device

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit fixes a real wire-level bug in nft_fwd_neigh_eval()
  (net/netfilter/nft_fwd_netdev.c) but has no Fixes: tag and no Cc:…
- [Low] The commit message says the rule supplies both the device and the
  nexthop "so the route dst is never consulted", which implies dropping…

--

netfilter: nft_fwd_netdev: drop the stale dst when steering to a device

This patch makes nft_fwd_neigh_eval() drop the skb's dst before it
switches skb->dev to the configured output device and calls neigh_xmit().
Without the drop, a packet parked on an incomplete neighbour's arp_queue
can be re-resolved through the stale dst when the queue drains. It then
leaves the configured device carrying another device's link-layer
addresses.

Link: https://lore.kernel.org/netdev/QDISC-6HS9.v3.r0.20261103062649-2@mojatatu.com/

> Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-6HS9.v3.r0.20261103062649-2@mojatatu.com
> Link: https://lore.kernel.org/netdev/QDISC-6HS9.v3.r0.20261103062649-2@mojatatu.com/
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

[Severity: Low]
Should this have a Fixes: tag and Cc: stable?

The skb->dev = dev; followed by neigh_xmit() in nft_fwd_neigh_eval(),
with the skb's dst still attached, came in with:

d32de98ea70f ("netfilter: nft_fwd_netdev: allow to forward packets via neighbour layer")

The bug became much easier to hit once nft_fwd_validate() allowed
NF_NETDEV_EGRESS, because egress skbs always carry the output route's
dst:

f87b9464d152 ("netfilter: nft_fwd_netdev: Support egress hook")

The reproducer in the commit message uses an egress chain. Without a
Fixes: tag, stable backporters can't tell which trees need this fix.

> diff --git a/net/netfilter/nft_fwd_netdev.c b/net/netfilter/nft_fwd_netdev.c
> index a48c2f765bbaa..ad4d61d64cdde 100644
> --- a/net/netfilter/nft_fwd_netdev.c
> +++ b/net/netfilter/nft_fwd_netdev.c
> @@ -175,6 +175,7 @@ static void nft_fwd_neigh_eval(const struct nft_expr *expr,
>  		}
>  	}
>  
> +	skb_dst_drop(skb);

[Severity: Low]
The commit message says:

    Both lookup inputs the neighbour transmission needs, the device and
    the nexthop, are supplied by the rule, so the route dst is never
    consulted.

Is that accurate? The arp_queue drain is not the only place that reads
the dst. If the target neighbour never resolves, the neighbour's
error_report hook also reads it:

neigh_xmit()
  neigh->output()
    ... resolution fails ...
    arp_error_report() / ndisc_error_report()
      dst_link_failure(skb)

include/net/dst.h:dst_link_failure() {
    ...
	if (dst && dst->ops && dst->ops->link_failure)
		dst->ops->link_failure(skb);
    ...
}

Before this patch, an egress skb handled by "fwd ip to <nh> device <dev>"
whose nexthop never resolved still had its route dst. So
ipv4_link_failure() or ip6_link_failure() sent ICMP host unreachable or
ICMPv6 address unreachable back to the sender, and expired or
invalidated the original route.

Once the dst is dropped, dst_link_failure() returns without doing
anything. __icmp_send() also needs skb_rtable(skb_in), so the packet is
now dropped silently. Local senders no longer get EHOSTUNREACH feedback,
and the route is not expired.

The new behaviour is arguably more correct. The old ICMP and route
expiry applied to a route and device the packet was no longer using,
and the new behaviour matches the ingress hook, where skbs normally have
no dst.

Could the commit message mention this change in user-visible behaviour?

The follow-up patch "bpf: drop the stale dst when redirecting to an
explicit nexthop" makes the same claim for bpf_redirect_neigh(), so the
same question may apply there.

>  	skb->dev = dev;
>  	skb_clear_tstamp(skb);
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/CORE-9NOG.v1.r0.20261111073528%40mojatatu.com