From: Jiayuan Chen <jiayuan.chen@shopee.com>
bond_rr_gen_slave_id() dereferences bond->rr_tx_counter without a NULL
check. rr_tx_counter is a per-CPU counter only allocated in bond_open()
when the bond mode is round-robin. If the bond device was never brought
up, rr_tx_counter remains NULL, causing a null-ptr-deref.
The XDP redirect path can reach this code even when the bond is not up:
bpf_master_redirect_enabled_key is a global static key, so when any bond
device has native XDP attached, the XDP_TX -> xdp_master_redirect()
interception is enabled for all bond slaves system-wide. This allows the
path xdp_master_redirect() -> bond_xdp_get_xmit_slave() ->
bond_xdp_xmit_roundrobin_slave_get() -> bond_rr_gen_slave_id() to be
reached on a bond that was never opened.
Fix this by adding a NULL check with unlikely() in bond_rr_gen_slave_id()
before dereferencing rr_tx_counter. When rr_tx_counter is NULL (bond was
never opened), fall back to get_random_u32() for slave selection. The
allocation in bond_open() is kept, with WRITE_ONCE() added to safely
publish the pointer to the XDP read side. A plain read suffices for the
!bond->rr_tx_counter guard in bond_open() itself, as bond_open() runs
under RTNL lock and is the only writer of rr_tx_counter.
Fixes: 879af96ffd72 ("net, core: Add support for XDP redirection to slave device")
Reported-by: syzbot+80e046b8da2820b6ba73@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/698f84c6.a70a0220.2c38d7.00cc.GAE@google.com/T/
Signed-off-by: Jiayuan Chen <jiayuan.chen@shopee.com>
---
drivers/net/bonding/bond_main.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 444519078da3..b8ec87625ce3 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4290,9 +4290,11 @@ static int bond_open(struct net_device *bond_dev)
struct slave *slave;
if (BOND_MODE(bond) == BOND_MODE_ROUNDROBIN && !bond->rr_tx_counter) {
- bond->rr_tx_counter = alloc_percpu(u32);
- if (!bond->rr_tx_counter)
+ u32 __percpu *rr_tx_tmp = alloc_percpu(u32);
+
+ if (!rr_tx_tmp)
return -ENOMEM;
+ WRITE_ONCE(bond->rr_tx_counter, rr_tx_tmp);
}
/* reset slave->backup and slave->inactive */
@@ -4883,6 +4885,9 @@ static u32 bond_rr_gen_slave_id(struct bonding *bond)
struct reciprocal_value reciprocal_packets_per_slave;
int packets_per_slave = bond->params.packets_per_slave;
+ if (unlikely(!READ_ONCE(bond->rr_tx_counter)))
+ return get_random_u32();
+
switch (packets_per_slave) {
case 0:
slave_id = get_random_u32();
--
2.43.0On Mon, Mar 09, 2026 at 11:06:58AM +0800, Jiayuan Chen wrote:
> From: Jiayuan Chen <jiayuan.chen@shopee.com>
>
> bond_rr_gen_slave_id() dereferences bond->rr_tx_counter without a NULL
> check. rr_tx_counter is a per-CPU counter only allocated in bond_open()
> when the bond mode is round-robin. If the bond device was never brought
> up, rr_tx_counter remains NULL, causing a null-ptr-deref.
>
> The XDP redirect path can reach this code even when the bond is not up:
> bpf_master_redirect_enabled_key is a global static key, so when any bond
> device has native XDP attached, the XDP_TX -> xdp_master_redirect()
> interception is enabled for all bond slaves system-wide. This allows the
> path xdp_master_redirect() -> bond_xdp_get_xmit_slave() ->
> bond_xdp_xmit_roundrobin_slave_get() -> bond_rr_gen_slave_id() to be
> reached on a bond that was never opened.
>
> Fix this by adding a NULL check with unlikely() in bond_rr_gen_slave_id()
> before dereferencing rr_tx_counter. When rr_tx_counter is NULL (bond was
> never opened), fall back to get_random_u32() for slave selection. The
> allocation in bond_open() is kept, with WRITE_ONCE() added to safely
> publish the pointer to the XDP read side. A plain read suffices for the
> !bond->rr_tx_counter guard in bond_open() itself, as bond_open() runs
> under RTNL lock and is the only writer of rr_tx_counter.
>
> Fixes: 879af96ffd72 ("net, core: Add support for XDP redirection to slave device")
> Reported-by: syzbot+80e046b8da2820b6ba73@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/698f84c6.a70a0220.2c38d7.00cc.GAE@google.com/T/
> Signed-off-by: Jiayuan Chen <jiayuan.chen@shopee.com>
> ---
> drivers/net/bonding/bond_main.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
This is Jay's patch + the unlikely change, looks good to me.
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
Cheers,
Nik
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 444519078da3..b8ec87625ce3 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4290,9 +4290,11 @@ static int bond_open(struct net_device *bond_dev)
> struct slave *slave;
>
> if (BOND_MODE(bond) == BOND_MODE_ROUNDROBIN && !bond->rr_tx_counter) {
> - bond->rr_tx_counter = alloc_percpu(u32);
> - if (!bond->rr_tx_counter)
> + u32 __percpu *rr_tx_tmp = alloc_percpu(u32);
> +
> + if (!rr_tx_tmp)
> return -ENOMEM;
> + WRITE_ONCE(bond->rr_tx_counter, rr_tx_tmp);
> }
>
> /* reset slave->backup and slave->inactive */
> @@ -4883,6 +4885,9 @@ static u32 bond_rr_gen_slave_id(struct bonding *bond)
> struct reciprocal_value reciprocal_packets_per_slave;
> int packets_per_slave = bond->params.packets_per_slave;
>
> + if (unlikely(!READ_ONCE(bond->rr_tx_counter)))
> + return get_random_u32();
> +
> switch (packets_per_slave) {
> case 0:
> slave_id = get_random_u32();
> --
> 2.43.0
>
On Tue, Mar 10, 2026 at 12:49 PM Nikolay Aleksandrov
<razor@blackwall.org> wrote:
>
> On Mon, Mar 09, 2026 at 11:06:58AM +0800, Jiayuan Chen wrote:
> > From: Jiayuan Chen <jiayuan.chen@shopee.com>
> >
> > bond_rr_gen_slave_id() dereferences bond->rr_tx_counter without a NULL
> > check. rr_tx_counter is a per-CPU counter only allocated in bond_open()
> > when the bond mode is round-robin. If the bond device was never brought
> > up, rr_tx_counter remains NULL, causing a null-ptr-deref.
> >
> > The XDP redirect path can reach this code even when the bond is not up:
> > bpf_master_redirect_enabled_key is a global static key, so when any bond
> > device has native XDP attached, the XDP_TX -> xdp_master_redirect()
> > interception is enabled for all bond slaves system-wide. This allows the
> > path xdp_master_redirect() -> bond_xdp_get_xmit_slave() ->
> > bond_xdp_xmit_roundrobin_slave_get() -> bond_rr_gen_slave_id() to be
> > reached on a bond that was never opened.
> >
> > Fix this by adding a NULL check with unlikely() in bond_rr_gen_slave_id()
> > before dereferencing rr_tx_counter. When rr_tx_counter is NULL (bond was
> > never opened), fall back to get_random_u32() for slave selection. The
> > allocation in bond_open() is kept, with WRITE_ONCE() added to safely
> > publish the pointer to the XDP read side. A plain read suffices for the
> > !bond->rr_tx_counter guard in bond_open() itself, as bond_open() runs
> > under RTNL lock and is the only writer of rr_tx_counter.
> >
> > Fixes: 879af96ffd72 ("net, core: Add support for XDP redirection to slave device")
> > Reported-by: syzbot+80e046b8da2820b6ba73@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/all/698f84c6.a70a0220.2c38d7.00cc.GAE@google.com/T/
> > Signed-off-by: Jiayuan Chen <jiayuan.chen@shopee.com>
> > ---
> > drivers/net/bonding/bond_main.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
>
> This is Jay's patch + the unlikely change, looks good to me.
> Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
Orthogonal to this patch :
get_random_u32() typical cost is around 10 to 20 ns, I really wonder
if this makes sense
for the packets_per_slave == 0 or 1 case to haves this kind of
randomness in the first place.
Perhaps we could use a
static DEFINE_PER_CPU(u32, rr_tx_counter)
And :
slave_id = this_cpu_inc_return(rr_tx_counter);
On Tue, Mar 10, 2026 at 1:00 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Mar 10, 2026 at 12:49 PM Nikolay Aleksandrov
> <razor@blackwall.org> wrote:
> >
> > On Mon, Mar 09, 2026 at 11:06:58AM +0800, Jiayuan Chen wrote:
> > > From: Jiayuan Chen <jiayuan.chen@shopee.com>
> > >
> > > bond_rr_gen_slave_id() dereferences bond->rr_tx_counter without a NULL
> > > check. rr_tx_counter is a per-CPU counter only allocated in bond_open()
> > > when the bond mode is round-robin. If the bond device was never brought
> > > up, rr_tx_counter remains NULL, causing a null-ptr-deref.
> > >
> > > The XDP redirect path can reach this code even when the bond is not up:
> > > bpf_master_redirect_enabled_key is a global static key, so when any bond
> > > device has native XDP attached, the XDP_TX -> xdp_master_redirect()
> > > interception is enabled for all bond slaves system-wide. This allows the
> > > path xdp_master_redirect() -> bond_xdp_get_xmit_slave() ->
> > > bond_xdp_xmit_roundrobin_slave_get() -> bond_rr_gen_slave_id() to be
> > > reached on a bond that was never opened.
> > >
> > > Fix this by adding a NULL check with unlikely() in bond_rr_gen_slave_id()
> > > before dereferencing rr_tx_counter. When rr_tx_counter is NULL (bond was
> > > never opened), fall back to get_random_u32() for slave selection. The
> > > allocation in bond_open() is kept, with WRITE_ONCE() added to safely
> > > publish the pointer to the XDP read side. A plain read suffices for the
> > > !bond->rr_tx_counter guard in bond_open() itself, as bond_open() runs
> > > under RTNL lock and is the only writer of rr_tx_counter.
> > >
> > > Fixes: 879af96ffd72 ("net, core: Add support for XDP redirection to slave device")
> > > Reported-by: syzbot+80e046b8da2820b6ba73@syzkaller.appspotmail.com
> > > Closes: https://lore.kernel.org/all/698f84c6.a70a0220.2c38d7.00cc.GAE@google.com/T/
> > > Signed-off-by: Jiayuan Chen <jiayuan.chen@shopee.com>
> > > ---
> > > drivers/net/bonding/bond_main.c | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> >
> > This is Jay's patch + the unlikely change, looks good to me.
> > Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
>
> Orthogonal to this patch :
>
> get_random_u32() typical cost is around 10 to 20 ns, I really wonder
> if this makes sense
> for the packets_per_slave == 0 or 1 case to haves this kind of
> randomness in the first place.
>
> Perhaps we could use a
>
> static DEFINE_PER_CPU(u32, rr_tx_counter)
>
> And :
> slave_id = this_cpu_inc_return(rr_tx_counter);
I also have mixed feelings about this patch.
We probably should detect that the device is not ready before hitting
something deeper in the stack.
Sure, a NULL deref is avoided, bu what happens next ?
We send a packet while the device is not UP, I am pretty sure this
violates at least some RCU rules in device dismantling.
On Tue, Mar 10, 2026 at 01:07:15PM +0100, Eric Dumazet wrote:
> On Tue, Mar 10, 2026 at 1:00 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Mar 10, 2026 at 12:49 PM Nikolay Aleksandrov
> > <razor@blackwall.org> wrote:
> > >
> > > On Mon, Mar 09, 2026 at 11:06:58AM +0800, Jiayuan Chen wrote:
> > > > From: Jiayuan Chen <jiayuan.chen@shopee.com>
> > > >
> > > > bond_rr_gen_slave_id() dereferences bond->rr_tx_counter without a NULL
> > > > check. rr_tx_counter is a per-CPU counter only allocated in bond_open()
> > > > when the bond mode is round-robin. If the bond device was never brought
> > > > up, rr_tx_counter remains NULL, causing a null-ptr-deref.
> > > >
> > > > The XDP redirect path can reach this code even when the bond is not up:
> > > > bpf_master_redirect_enabled_key is a global static key, so when any bond
> > > > device has native XDP attached, the XDP_TX -> xdp_master_redirect()
> > > > interception is enabled for all bond slaves system-wide. This allows the
> > > > path xdp_master_redirect() -> bond_xdp_get_xmit_slave() ->
> > > > bond_xdp_xmit_roundrobin_slave_get() -> bond_rr_gen_slave_id() to be
> > > > reached on a bond that was never opened.
> > > >
> > > > Fix this by adding a NULL check with unlikely() in bond_rr_gen_slave_id()
> > > > before dereferencing rr_tx_counter. When rr_tx_counter is NULL (bond was
> > > > never opened), fall back to get_random_u32() for slave selection. The
> > > > allocation in bond_open() is kept, with WRITE_ONCE() added to safely
> > > > publish the pointer to the XDP read side. A plain read suffices for the
> > > > !bond->rr_tx_counter guard in bond_open() itself, as bond_open() runs
> > > > under RTNL lock and is the only writer of rr_tx_counter.
> > > >
> > > > Fixes: 879af96ffd72 ("net, core: Add support for XDP redirection to slave device")
> > > > Reported-by: syzbot+80e046b8da2820b6ba73@syzkaller.appspotmail.com
> > > > Closes: https://lore.kernel.org/all/698f84c6.a70a0220.2c38d7.00cc.GAE@google.com/T/
> > > > Signed-off-by: Jiayuan Chen <jiayuan.chen@shopee.com>
> > > > ---
> > > > drivers/net/bonding/bond_main.c | 9 +++++++--
> > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > >
> > >
> > > This is Jay's patch + the unlikely change, looks good to me.
> > > Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
> >
> > Orthogonal to this patch :
> >
> > get_random_u32() typical cost is around 10 to 20 ns, I really wonder
> > if this makes sense
> > for the packets_per_slave == 0 or 1 case to haves this kind of
> > randomness in the first place.
> >
> > Perhaps we could use a
> >
> > static DEFINE_PER_CPU(u32, rr_tx_counter)
> >
> > And :
> > slave_id = this_cpu_inc_return(rr_tx_counter);
>
> I also have mixed feelings about this patch.
>
> We probably should detect that the device is not ready before hitting
> something deeper in the stack.
>
> Sure, a NULL deref is avoided, bu what happens next ?
>
> We send a packet while the device is not UP, I am pretty sure this
> violates at least some RCU rules in device dismantling.
IIRC when the redirect continues, the packet should get dropped if the device is
not up (checks at a few places), but that's outside of bond's jurisdiction and
after the slave id is needed in xdp master redirect's path unfortunately.
I'm not sure it can reach much further, it just has the master dev's slave id
generation in its path.
In any case we shouldn't crash in the slave id generation in the bonding,
that ndo's only job is to return a slave id.
On 3/10/26 1:39 PM, Nikolay Aleksandrov wrote:
> On Tue, Mar 10, 2026 at 01:07:15PM +0100, Eric Dumazet wrote:
>> On Tue, Mar 10, 2026 at 1:00 PM Eric Dumazet <edumazet@google.com> wrote:
>>>
>>> On Tue, Mar 10, 2026 at 12:49 PM Nikolay Aleksandrov
>>> <razor@blackwall.org> wrote:
>>>>
>>>> On Mon, Mar 09, 2026 at 11:06:58AM +0800, Jiayuan Chen wrote:
>>>>> From: Jiayuan Chen <jiayuan.chen@shopee.com>
>>>>>
>>>>> bond_rr_gen_slave_id() dereferences bond->rr_tx_counter without a NULL
>>>>> check. rr_tx_counter is a per-CPU counter only allocated in bond_open()
>>>>> when the bond mode is round-robin. If the bond device was never brought
>>>>> up, rr_tx_counter remains NULL, causing a null-ptr-deref.
>>>>>
>>>>> The XDP redirect path can reach this code even when the bond is not up:
>>>>> bpf_master_redirect_enabled_key is a global static key, so when any bond
>>>>> device has native XDP attached, the XDP_TX -> xdp_master_redirect()
>>>>> interception is enabled for all bond slaves system-wide. This allows the
>>>>> path xdp_master_redirect() -> bond_xdp_get_xmit_slave() ->
>>>>> bond_xdp_xmit_roundrobin_slave_get() -> bond_rr_gen_slave_id() to be
>>>>> reached on a bond that was never opened.
>>>>>
>>>>> Fix this by adding a NULL check with unlikely() in bond_rr_gen_slave_id()
>>>>> before dereferencing rr_tx_counter. When rr_tx_counter is NULL (bond was
>>>>> never opened), fall back to get_random_u32() for slave selection. The
>>>>> allocation in bond_open() is kept, with WRITE_ONCE() added to safely
>>>>> publish the pointer to the XDP read side. A plain read suffices for the
>>>>> !bond->rr_tx_counter guard in bond_open() itself, as bond_open() runs
>>>>> under RTNL lock and is the only writer of rr_tx_counter.
>>>>>
>>>>> Fixes: 879af96ffd72 ("net, core: Add support for XDP redirection to slave device")
>>>>> Reported-by: syzbot+80e046b8da2820b6ba73@syzkaller.appspotmail.com
>>>>> Closes: https://lore.kernel.org/all/698f84c6.a70a0220.2c38d7.00cc.GAE@google.com/T/
>>>>> Signed-off-by: Jiayuan Chen <jiayuan.chen@shopee.com>
>>>>> ---
>>>>> drivers/net/bonding/bond_main.c | 9 +++++++--
>>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>
>>>>
>>>> This is Jay's patch + the unlikely change, looks good to me.
>>>> Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
>>>
>>> Orthogonal to this patch :
>>>
>>> get_random_u32() typical cost is around 10 to 20 ns, I really wonder
>>> if this makes sense
>>> for the packets_per_slave == 0 or 1 case to haves this kind of
>>> randomness in the first place.
>>>
>>> Perhaps we could use a
>>>
>>> static DEFINE_PER_CPU(u32, rr_tx_counter)
>>>
>>> And :
>>> slave_id = this_cpu_inc_return(rr_tx_counter);
>>
>> I also have mixed feelings about this patch.
>>
>> We probably should detect that the device is not ready before hitting
>> something deeper in the stack.
>>
>> Sure, a NULL deref is avoided, bu what happens next ?
>>
>> We send a packet while the device is not UP, I am pretty sure this
>> violates at least some RCU rules in device dismantling.
>
> IIRC when the redirect continues, the packet should get dropped if the device is
> not up (checks at a few places), but that's outside of bond's jurisdiction and
> after the slave id is needed in xdp master redirect's path unfortunately.
> I'm not sure it can reach much further, it just has the master dev's slave id
> generation in its path.
>
> In any case we shouldn't crash in the slave id generation in the bonding,
> that ndo's only job is to return a slave id.
I'm sorry for the back and forth, but I share Eric's concern. I think
the approach suggested by Daniel:
https://lore.kernel.org/netdev/4d15be93-b497-4499-996d-9f3a67a2abc6@iogearbox.net/
or the initial patch form:
https://lore.kernel.org/netdev/20260224112545.37888-1-jiayuan.chen@linux.dev/T/#m7c67bb12f85bc88d583788fb6e41113c46208ae7
would be better. To respond to old concerns raised there: the check is
IMHO bond-specific, as control moves from the lower interface to the
upper bonding device, and the code is under an RCU critical section, the
device can't go away before the xmit is completed.
/P
On 3/12/26 6:36 PM, Paolo Abeni wrote:
> On 3/10/26 1:39 PM, Nikolay Aleksandrov wrote:
>> On Tue, Mar 10, 2026 at 01:07:15PM +0100, Eric Dumazet wrote:
>>> On Tue, Mar 10, 2026 at 1:00 PM Eric Dumazet <edumazet@google.com> wrote:
>>>> On Tue, Mar 10, 2026 at 12:49 PM Nikolay Aleksandrov
>>>> <razor@blackwall.org> wrote:
>>>>> On Mon, Mar 09, 2026 at 11:06:58AM +0800, Jiayuan Chen wrote:
>>>>>> From: Jiayuan Chen <jiayuan.chen@shopee.com>
>>>>>>
>>>>>> bond_rr_gen_slave_id() dereferences bond->rr_tx_counter without a NULL
>>>>>> check. rr_tx_counter is a per-CPU counter only allocated in bond_open()
>>>>>> when the bond mode is round-robin. If the bond device was never brought
>>>>>> up, rr_tx_counter remains NULL, causing a null-ptr-deref.
>>>>>>
>>>>>> The XDP redirect path can reach this code even when the bond is not up:
>>>>>> bpf_master_redirect_enabled_key is a global static key, so when any bond
>>>>>> device has native XDP attached, the XDP_TX -> xdp_master_redirect()
>>>>>> interception is enabled for all bond slaves system-wide. This allows the
>>>>>> path xdp_master_redirect() -> bond_xdp_get_xmit_slave() ->
>>>>>> bond_xdp_xmit_roundrobin_slave_get() -> bond_rr_gen_slave_id() to be
>>>>>> reached on a bond that was never opened.
>>>>>>
>>>>>> Fix this by adding a NULL check with unlikely() in bond_rr_gen_slave_id()
>>>>>> before dereferencing rr_tx_counter. When rr_tx_counter is NULL (bond was
>>>>>> never opened), fall back to get_random_u32() for slave selection. The
>>>>>> allocation in bond_open() is kept, with WRITE_ONCE() added to safely
>>>>>> publish the pointer to the XDP read side. A plain read suffices for the
>>>>>> !bond->rr_tx_counter guard in bond_open() itself, as bond_open() runs
>>>>>> under RTNL lock and is the only writer of rr_tx_counter.
>>>>>>
>>>>>> Fixes: 879af96ffd72 ("net, core: Add support for XDP redirection to slave device")
>>>>>> Reported-by: syzbot+80e046b8da2820b6ba73@syzkaller.appspotmail.com
>>>>>> Closes: https://lore.kernel.org/all/698f84c6.a70a0220.2c38d7.00cc.GAE@google.com/T/
>>>>>> Signed-off-by: Jiayuan Chen <jiayuan.chen@shopee.com>
>>>>>> ---
>>>>>> drivers/net/bonding/bond_main.c | 9 +++++++--
>>>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>
>>>>> This is Jay's patch + the unlikely change, looks good to me.
>>>>> Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
>>>> Orthogonal to this patch :
>>>>
>>>> get_random_u32() typical cost is around 10 to 20 ns, I really wonder
>>>> if this makes sense
>>>> for the packets_per_slave == 0 or 1 case to haves this kind of
>>>> randomness in the first place.
>>>>
>>>> Perhaps we could use a
>>>>
>>>> static DEFINE_PER_CPU(u32, rr_tx_counter)
>>>>
>>>> And :
>>>> slave_id = this_cpu_inc_return(rr_tx_counter);
>>> I also have mixed feelings about this patch.
>>>
>>> We probably should detect that the device is not ready before hitting
>>> something deeper in the stack.
>>>
>>> Sure, a NULL deref is avoided, bu what happens next ?
>>>
>>> We send a packet while the device is not UP, I am pretty sure this
>>> violates at least some RCU rules in device dismantling.
>> IIRC when the redirect continues, the packet should get dropped if the device is
>> not up (checks at a few places), but that's outside of bond's jurisdiction and
>> after the slave id is needed in xdp master redirect's path unfortunately.
>> I'm not sure it can reach much further, it just has the master dev's slave id
>> generation in its path.
>>
>> In any case we shouldn't crash in the slave id generation in the bonding,
>> that ndo's only job is to return a slave id.
> I'm sorry for the back and forth, but I share Eric's concern. I think
> the approach suggested by Daniel:
>
> https://lore.kernel.org/netdev/4d15be93-b497-4499-996d-9f3a67a2abc6@iogearbox.net/
>
> or the initial patch form:
>
> https://lore.kernel.org/netdev/20260224112545.37888-1-jiayuan.chen@linux.dev/T/#m7c67bb12f85bc88d583788fb6e41113c46208ae7
>
> would be better. To respond to old concerns raised there: the check is
> IMHO bond-specific, as control moves from the lower interface to the
> upper bonding device, and the code is under an RCU critical section, the
> device can't go away before the xmit is completed.
>
> /P
Looking at this issue holistically:
1. The XDP layer fix addresses the root cause of the current issue
2. Adding a defensive null check in bond_rr_gen_slave_id() protects
against buggy callers - whether from XDP or future code paths. This
aligns with the defense-in-depth principle that Nikolay and Sebastian
highlighted.
Could we include both in v1? This way, the bond layer is robust regardless
of who calls it, preventing similar crashes from other potential code paths.
Thanks
On 3/12/26 7:02 PM, Jiayuan Chen wrote: > f who calls it, preventing similar crashes from other potential code > paths. Sorry for the noise. Could we include both: 1. Add link status check in xdp 2. Adding a defensive null check in bond_rr_gen_slave_id() This way, the bond layer is robust regardless of who calls it, preventing similar crashes from other potential code paths.
On Thu, Mar 12, 2026 at 11:36:20AM +0100, Paolo Abeni wrote:
> On 3/10/26 1:39 PM, Nikolay Aleksandrov wrote:
> > On Tue, Mar 10, 2026 at 01:07:15PM +0100, Eric Dumazet wrote:
> >> On Tue, Mar 10, 2026 at 1:00 PM Eric Dumazet <edumazet@google.com> wrote:
> >>>
> >>> On Tue, Mar 10, 2026 at 12:49 PM Nikolay Aleksandrov
> >>> <razor@blackwall.org> wrote:
> >>>>
> >>>> On Mon, Mar 09, 2026 at 11:06:58AM +0800, Jiayuan Chen wrote:
> >>>>> From: Jiayuan Chen <jiayuan.chen@shopee.com>
> >>>>>
> >>>>> bond_rr_gen_slave_id() dereferences bond->rr_tx_counter without a NULL
> >>>>> check. rr_tx_counter is a per-CPU counter only allocated in bond_open()
> >>>>> when the bond mode is round-robin. If the bond device was never brought
> >>>>> up, rr_tx_counter remains NULL, causing a null-ptr-deref.
> >>>>>
> >>>>> The XDP redirect path can reach this code even when the bond is not up:
> >>>>> bpf_master_redirect_enabled_key is a global static key, so when any bond
> >>>>> device has native XDP attached, the XDP_TX -> xdp_master_redirect()
> >>>>> interception is enabled for all bond slaves system-wide. This allows the
> >>>>> path xdp_master_redirect() -> bond_xdp_get_xmit_slave() ->
> >>>>> bond_xdp_xmit_roundrobin_slave_get() -> bond_rr_gen_slave_id() to be
> >>>>> reached on a bond that was never opened.
> >>>>>
> >>>>> Fix this by adding a NULL check with unlikely() in bond_rr_gen_slave_id()
> >>>>> before dereferencing rr_tx_counter. When rr_tx_counter is NULL (bond was
> >>>>> never opened), fall back to get_random_u32() for slave selection. The
> >>>>> allocation in bond_open() is kept, with WRITE_ONCE() added to safely
> >>>>> publish the pointer to the XDP read side. A plain read suffices for the
> >>>>> !bond->rr_tx_counter guard in bond_open() itself, as bond_open() runs
> >>>>> under RTNL lock and is the only writer of rr_tx_counter.
> >>>>>
> >>>>> Fixes: 879af96ffd72 ("net, core: Add support for XDP redirection to slave device")
> >>>>> Reported-by: syzbot+80e046b8da2820b6ba73@syzkaller.appspotmail.com
> >>>>> Closes: https://lore.kernel.org/all/698f84c6.a70a0220.2c38d7.00cc.GAE@google.com/T/
> >>>>> Signed-off-by: Jiayuan Chen <jiayuan.chen@shopee.com>
> >>>>> ---
> >>>>> drivers/net/bonding/bond_main.c | 9 +++++++--
> >>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
> >>>>>
> >>>>
> >>>> This is Jay's patch + the unlikely change, looks good to me.
> >>>> Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
> >>>
> >>> Orthogonal to this patch :
> >>>
> >>> get_random_u32() typical cost is around 10 to 20 ns, I really wonder
> >>> if this makes sense
> >>> for the packets_per_slave == 0 or 1 case to haves this kind of
> >>> randomness in the first place.
> >>>
> >>> Perhaps we could use a
> >>>
> >>> static DEFINE_PER_CPU(u32, rr_tx_counter)
> >>>
> >>> And :
> >>> slave_id = this_cpu_inc_return(rr_tx_counter);
> >>
> >> I also have mixed feelings about this patch.
> >>
> >> We probably should detect that the device is not ready before hitting
> >> something deeper in the stack.
> >>
> >> Sure, a NULL deref is avoided, bu what happens next ?
> >>
> >> We send a packet while the device is not UP, I am pretty sure this
> >> violates at least some RCU rules in device dismantling.
> >
> > IIRC when the redirect continues, the packet should get dropped if the device is
> > not up (checks at a few places), but that's outside of bond's jurisdiction and
> > after the slave id is needed in xdp master redirect's path unfortunately.
> > I'm not sure it can reach much further, it just has the master dev's slave id
> > generation in its path.
> >
> > In any case we shouldn't crash in the slave id generation in the bonding,
> > that ndo's only job is to return a slave id.
>
> I'm sorry for the back and forth, but I share Eric's concern. I think
> the approach suggested by Daniel:
>
> https://lore.kernel.org/netdev/4d15be93-b497-4499-996d-9f3a67a2abc6@iogearbox.net/
>
That will work, I like Daniel's patch as well. It will add a test for all redirects
for master devices, but I guess that is ok. For bonding it will work because the bond
has the problem only while it was never opened (before first up).
IMO this patch still has value, because currently the code implicitly relies on
a specific sequence of events, who knows tomorrow someone may find another way
and again call that ndo while the bond is down.
> or the initial patch form:
>
> https://lore.kernel.org/netdev/20260224112545.37888-1-jiayuan.chen@linux.dev/T/#m7c67bb12f85bc88d583788fb6e41113c46208ae7
>
> would be better. To respond to old concerns raised there: the check is
> IMHO bond-specific, as control moves from the lower interface to the
> upper bonding device, and the code is under an RCU critical section, the
> device can't go away before the xmit is completed.
This one I don't like, it is not about going away, it is more about adding 2 new
tests for everyone and potentially 1 more cache line.
>
> /P
>
Cheers,
Nik
© 2016 - 2026 Red Hat, Inc.