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

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

[Nikhil]

bpf_ipv4_fib_lookup() and bpf_ipv6_fib_lookup() copy the neighbour's
link layer address into params->dmac without any synchronisation, but
neigh_update() writes neigh->ha under write_seqlock(&neigh->ha_lock)
exactly because there are lockless readers.  A BPF program calling
bpf_fib_lookup() while the neighbour is being updated can therefore be
handed a torn address, and XDP/tc then forwards the packet to a bogus
L2 destination.

neigh_ha_snapshot() cannot be used here because it copies dev->addr_len
bytes while params->dmac is only ETH_ALEN long (an IPoIB egress device
has addr_len 20 and would overflow into params->smac), so open-code the
seqlock loop around the ETH_ALEN copy.

Same problem and same fix as commit 57549ab90791 ("net: bridge: arp/nd
proxy: fix reading neigh ha") and commit b824059a673b ("vxlan: fix
reading neigh ha").

Reproduced on x86_64 under qemu: a dummy device holds a permanent
neighbour whose lladdr is flipped between aa:aa:aa:aa:aa:aa and
bb:bb:bb:bb:bb:bb with RTM_NEWNEIGH, while an XDP program driven by
BPF_PROG_TEST_RUN calls bpf_fib_lookup() in a loop and checks that all
six bytes of the returned dmac are equal.  Before this patch: 211 torn
addresses out of 16300000 lookups (last one aa:aa:aa:aa:bb:bb).  After
this patch: 0 out of 38060000 lookups.

Fixes: 87f5fc7e48dd ("bpf: Provide helper to do forwarding lookups in kernel FIB table")
Cc: stable@vger.kernel.org
Signed-off-by: Nikhil <nikhilljatt@gmail.com>
---
 net/core/filter.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 61940e753552..6fdb85c9af44 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6298,6 +6298,20 @@ static const struct bpf_func_proto bpf_skb_get_xfrm_state_proto = {
 #endif
 
 #if IS_ENABLED(CONFIG_INET) || IS_ENABLED(CONFIG_IPV6)
+/* Take a stable snapshot of the neighbour's link layer address.
+ * neigh_ha_snapshot() can not be used here because it copies dev->addr_len
+ * bytes while params->dmac is only ETH_ALEN long.
+ */
+static void bpf_fib_dmac_snapshot(u8 *dmac, const struct neighbour *neigh)
+{
+	unsigned int seq;
+
+	do {
+		seq = read_seqbegin(&neigh->ha_lock);
+		memcpy(dmac, neigh->ha, ETH_ALEN);
+	} while (read_seqretry(&neigh->ha_lock, seq));
+}
+
 static int bpf_fib_set_fwd_params(struct net_device *dev,
 				  struct bpf_fib_lookup *params,
 				  u32 flags, u32 mtu, u32 in_ifindex)
@@ -6491,7 +6505,7 @@ static int bpf_ipv4_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
 
 	if (!neigh || !(READ_ONCE(neigh->nud_state) & NUD_VALID))
 		return BPF_FIB_LKUP_RET_NO_NEIGH;
-	memcpy(params->dmac, neigh->ha, ETH_ALEN);
+	bpf_fib_dmac_snapshot(params->dmac, neigh);
 	memcpy(params->smac, dev->dev_addr, ETH_ALEN);
 
 set_fwd_params:
@@ -6644,7 +6658,7 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
 	neigh = __ipv6_neigh_lookup_noref(dev, dst);
 	if (!neigh || !(READ_ONCE(neigh->nud_state) & NUD_VALID))
 		return BPF_FIB_LKUP_RET_NO_NEIGH;
-	memcpy(params->dmac, neigh->ha, ETH_ALEN);
+	bpf_fib_dmac_snapshot(params->dmac, neigh);
 	memcpy(params->smac, dev->dev_addr, ETH_ALEN);
 
 set_fwd_params:
-- 
2.43.0

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2520 bytes --]

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 8513167a858a..a7c7bbfd90e3 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -6297,6 +6297,20 @@ static const struct bpf_func_proto bpf_skb_get_xfrm_state_proto = {
>  #endif
>
>  #if IS_ENABLED(CONFIG_INET) || IS_ENABLED(CONFIG_IPV6)
> +/* Take a stable snapshot of the neighbour's link layer address.
> + * neigh_ha_snapshot() can not be used here because it copies dev->addr_len
> + * bytes while params->dmac is only ETH_ALEN long.
> + */
> +static void bpf_fib_dmac_snapshot(u8 *dmac, const struct neighbour *neigh)
> +{
> +	unsigned int seq;
> +
> +	do {
> +		seq = read_seqbegin(&neigh->ha_lock);
> +		memcpy(dmac, neigh->ha, ETH_ALEN);
> +	} while (read_seqretry(&neigh->ha_lock, seq));
> +}
> +

The commit message states:

>     neigh_ha_snapshot() cannot be used here because it copies dev->addr_len
>     bytes while params->dmac is only ETH_ALEN long (an IPoIB egress device
>     has addr_len 20 and would overflow into params->smac), so open-code the
>     seqlock loop around the ETH_ALEN copy.

Is the overflow destination accurate? Looking at struct bpf_fib_lookup in
include/uapi/linux/bpf.h, smac precedes dmac in the final union:

    union {
        struct { __u32 mark; };
        struct {
            __u8 smac[6];    /* ETH_ALEN */
            __u8 dmac[6];    /* ETH_ALEN */
        };
    };

That union is the last member of the struct, so smac sits at offset 52 and
dmac at offset 58 in the 64-byte struct.

Calling neigh_ha_snapshot(params->dmac, neigh, dev) with an IPoIB device
(addr_len == 20) would write bytes 58..77, which is 6 bytes into dmac and
14 bytes past the end of struct bpf_fib_lookup. It cannot reach params->smac
at the lower offset.

Because params points at BPF-program memory that the verifier only sized to
sizeof(struct bpf_fib_lookup), the actual consequence is an out-of-bounds
write past the program-supplied buffer, not a clobbered sibling field.

The conclusion that neigh_ha_snapshot() is unusable here is correct, and
the in-code comment is accurate. Could the changelog's claim about where the
overflow lands be corrected before this goes to stable, since the
parenthetical is the sole stated justification for open-coding the loop?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34305501194

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

[Emil Tsalapatis]

On Tue, Sep 8, 2026 at 10:40 PM Nikhil <nikhilljatt@gmail.com> wrote:
>
> bpf_ipv4_fib_lookup() and bpf_ipv6_fib_lookup() copy the neighbour's
> link layer address into params->dmac without any synchronisation, but
> neigh_update() writes neigh->ha under write_seqlock(&neigh->ha_lock)
> exactly because there are lockless readers.  A BPF program calling
> bpf_fib_lookup() while the neighbour is being updated can therefore be
> handed a torn address, and XDP/tc then forwards the packet to a bogus
> L2 destination.
>
> neigh_ha_snapshot() cannot be used here because it copies dev->addr_len
> bytes while params->dmac is only ETH_ALEN long (an IPoIB egress device
> has addr_len 20 and would overflow into params->smac), so open-code the
> seqlock loop around the ETH_ALEN copy.
>
> Same problem and same fix as commit 57549ab90791 ("net: bridge: arp/nd
> proxy: fix reading neigh ha") and commit b824059a673b ("vxlan: fix
> reading neigh ha").
>
> Reproduced on x86_64 under qemu: a dummy device holds a permanent
> neighbour whose lladdr is flipped between aa:aa:aa:aa:aa:aa and
> bb:bb:bb:bb:bb:bb with RTM_NEWNEIGH, while an XDP program driven by
> BPF_PROG_TEST_RUN calls bpf_fib_lookup() in a loop and checks that all
> six bytes of the returned dmac are equal.  Before this patch: 211 torn
> addresses out of 16300000 lookups (last one aa:aa:aa:aa:bb:bb).  After
> this patch: 0 out of 38060000 lookups.
>
> Fixes: 87f5fc7e48dd ("bpf: Provide helper to do forwarding lookups in kernel FIB table")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nikhil <nikhilljatt@gmail.com>
> ---

Bot is right wrt possible lockups, please adjust the seqlock
accordingly. Also please
add your full name in the SOB.

pw-bot: cr

>  net/core/filter.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 61940e753552..6fdb85c9af44 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -6298,6 +6298,20 @@ static const struct bpf_func_proto bpf_skb_get_xfrm_state_proto = {
>  #endif
>
>  #if IS_ENABLED(CONFIG_INET) || IS_ENABLED(CONFIG_IPV6)
> +/* Take a stable snapshot of the neighbour's link layer address.
> + * neigh_ha_snapshot() can not be used here because it copies dev->addr_len
> + * bytes while params->dmac is only ETH_ALEN long.
> + */
> +static void bpf_fib_dmac_snapshot(u8 *dmac, const struct neighbour *neigh)
> +{
> +       unsigned int seq;
> +
> +       do {
> +               seq = read_seqbegin(&neigh->ha_lock);
> +               memcpy(dmac, neigh->ha, ETH_ALEN);
> +       } while (read_seqretry(&neigh->ha_lock, seq));
> +}
> +
>  static int bpf_fib_set_fwd_params(struct net_device *dev,
>                                   struct bpf_fib_lookup *params,
>                                   u32 flags, u32 mtu, u32 in_ifindex)
> @@ -6491,7 +6505,7 @@ static int bpf_ipv4_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>
>         if (!neigh || !(READ_ONCE(neigh->nud_state) & NUD_VALID))
>                 return BPF_FIB_LKUP_RET_NO_NEIGH;
> -       memcpy(params->dmac, neigh->ha, ETH_ALEN);
> +       bpf_fib_dmac_snapshot(params->dmac, neigh);
>         memcpy(params->smac, dev->dev_addr, ETH_ALEN);
>
>  set_fwd_params:
> @@ -6644,7 +6658,7 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>         neigh = __ipv6_neigh_lookup_noref(dev, dst);
>         if (!neigh || !(READ_ONCE(neigh->nud_state) & NUD_VALID))
>                 return BPF_FIB_LKUP_RET_NO_NEIGH;
> -       memcpy(params->dmac, neigh->ha, ETH_ALEN);
> +       bpf_fib_dmac_snapshot(params->dmac, neigh);
>         memcpy(params->smac, dev->dev_addr, ETH_ALEN);
>
>  set_fwd_params:
> --
> 2.43.0
>
>

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

[Nikhil Ludder]

On Wed, Sep 9, 2026 at 1:18 PM Emil Tsalapatis <linux-lists@etsalapatis.com> wrote:
> Bot is right wrt possible lockups, please adjust the seqlock
> accordingly. Also please
> add your full name in the SOB.

Will fix the SOB in v2, thanks.

On the lockup: I don't think it can happen here.  neigh->ha_lock has
exactly one writer in the tree, __neigh_update() at
net/core/neighbour.c:1495, and it already runs with softirqs disabled:
write_lock_bh(&neigh->lock) at line 1384 covers it through
write_unlock_bh() at line 1524.  A reader in softirq context therefore
cannot preempt the writer on the same CPU and spin on the sequence
count.

The same pattern is already used by the other lockless readers of
neigh->ha on the tx path, neigh_resolve_output() and
neigh_connected_output() (neighbour.c:1610 and :1639), and by the two
recent fixes this patch follows, 57549ab90791 ("net: bridge: arp/nd
proxy: fix reading neigh ha") and b824059a673b ("vxlan: fix reading
neigh ha").

Am I missing a writer path?  If you would still prefer a different
form here, I am happy to respin.

Thanks,
Nikhil

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

[Jiayuan Chen]

On 9/10/26 4:42 AM, Nikhil Ludder wrote:
> On Wed, Sep 9, 2026 at 1:18 PM Emil Tsalapatis <linux-lists@etsalapatis.com> wrote:
>> Bot is right wrt possible lockups, please adjust the seqlock
>> accordingly. Also please
>> add your full name in the SOB.
> Will fix the SOB in v2, thanks.
>
> On the lockup: I don't think it can happen here.  neigh->ha_lock has
> exactly one writer in the tree, __neigh_update() at
> net/core/neighbour.c:1495, and it already runs with softirqs disabled:
> write_lock_bh(&neigh->lock) at line 1384 covers it through
> write_unlock_bh() at line 1524.  A reader in softirq context therefore
> cannot preempt the writer on the same CPU and spin on the sequence
> count.


Yes, no deadlock.


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.

> The same pattern is already used by the other lockless readers of
> neigh->ha on the tx path, neigh_resolve_output() and
> neigh_connected_output() (neighbour.c:1610 and :1639), and by the two
> recent fixes this patch follows, 57549ab90791 ("net: bridge: arp/nd
> proxy: fix reading neigh ha") and b824059a673b ("vxlan: fix reading
> neigh ha").
>
> Am I missing a writer path?  If you would still prefer a different
> form here, I am happy to respin.
>
> Thanks,
> Nikhil

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

[Nikhil Ludder]

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

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