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

[Emil Tsalapatis]

On Thu Sep 10, 2026 at 7:31 AM EDT, Nikhil Ludder wrote:
> On 9/10/26 12:59 PM, Jiayuan Chen wrote:
>> Yes, no deadlock.
>
> Thanks for confirming.

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

>
>> 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