Re: [PATCH bpf] bpf: fix reading neigh ha in bpf_fib_lookup()

On Thu Sep 10, 2026 at 7:31 AM EDT, Nikhil Ludder wrote:

On 9/10/26 12:59 PM, Jiayuan Chen wrote:

quoted

Yes, no deadlock.

Thanks for confirming.

The explanation makes sense, hadn't considered where this is written
from. No need to adjust.

quoted

BTW, if an IPoIB device can show up here, dmac is already truncated
today and the packet can't be forwarded anyway.
Shouldn't we just reject addr_len != ETH_ALEN instead of open-coding
the copy? Then you can use the native function instead.

You are right, and it is worse than just dmac: the line immediately
below copies dev->dev_addr into params->smac with a fixed ETH_ALEN and
no addr_len check either, so both addresses are already truncated for
such a device.  struct bpf_fib_lookup declares smac[6] and dmac[6], so
the helper is ethernet-only by contract and a non-ethernet nexthop is
already outside it.

I would rather not fold that into this patch though.  This one is a
race fix with Cc: stable and no behaviour change, whereas rejecting a
device that today returns a (garbage) success is uapi visible and does
not belong in a stable backport.  Would you be happy with the seqlock
fix as it stands, and a follow-up for bpf-next that rejects
addr_len != ETH_ALEN and covers smac as well?  I am happy to write it.

I think this split makes sense, even if there's the churn of adding the
fix then removing it to use the pre-existing helper. It's just a couple
lines of temporary duplication. @Jiayuan wdyt?

If so, which return code would you want for that?  None of the existing
BPF_FIB_LKUP_RET_* really fits: NO_NEIGH is untrue since the neighbour
is there, NOT_FWDED is vague, and adding a new BPF_FIB_LKUP_RET_* is
uapi, which is another reason to keep it out of this patch.

Thanks,
Nikhil