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

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

Yes, no deadlock.

Thanks for confirming.

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.

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