1. Patchew
  2. linux
  3. View series

[PATCH net] bonding: only set speed/duplex to unknown, if getting speed failed

Thomas Bogendoerfer posted 1 patch 1 week ago
There is a newer version of this series
drivers/net/bonding/bond_main.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
[PATCH net] bonding: only set speed/duplex to unknown, if getting speed failed
Posted by Thomas Bogendoerfer 1 week ago 
bond_update_speed_duplex() first set speed/duplex to unknown and
then asks slave driver for current speed/duplex. Since getting
speed/duplex might take longer there is a race, where this false state
is visible by /proc/net/bonding. With commit 691b2bf14946 ("bonding:
 update port speed when getting bond speed") this race gets more visible,
if user space is calling ethtool on a regular base.

Fix this by only setting speed/duplex to unknown, if link speed is
really unknown/unusable.

Fixes: 691b2bf14946 ("bonding: update port speed when getting bond speed")
Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
---
 drivers/net/bonding/bond_main.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e7caf400a59c..4cdf89b21ca0 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -791,26 +791,29 @@ static int bond_update_speed_duplex(struct slave *slave)
 	struct ethtool_link_ksettings ecmd;
 	int res;
 
-	slave->speed = SPEED_UNKNOWN;
-	slave->duplex = DUPLEX_UNKNOWN;
-
 	res = __ethtool_get_link_ksettings(slave_dev, &ecmd);
 	if (res < 0)
-		return 1;
+		goto speed_duplex_unknown;
 	if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1))
-		return 1;
+		goto speed_duplex_unknown;
 	switch (ecmd.base.duplex) {
 	case DUPLEX_FULL:
 	case DUPLEX_HALF:
 		break;
 	default:
-		return 1;
+		goto speed_duplex_unknown;
 	}
 
 	slave->speed = ecmd.base.speed;
 	slave->duplex = ecmd.base.duplex;
 
 	return 0;
+
+speed_duplex_unknown:
+	slave->speed = SPEED_UNKNOWN;
+	slave->duplex = DUPLEX_UNKNOWN;
+
+	return 1;
 }
 
 const char *bond_slave_link_status(s8 link)
-- 
2.43.0
Re: [PATCH net] bonding: only set speed/duplex to unknown, if getting speed failed
Posted by Hangbin Liu 1 week ago 
On Fri, Jan 30, 2026 at 12:19:04PM +0100, Thomas Bogendoerfer wrote:
> bond_update_speed_duplex() first set speed/duplex to unknown and
> then asks slave driver for current speed/duplex. Since getting
> speed/duplex might take longer there is a race, where this false state
> is visible by /proc/net/bonding. With commit 691b2bf14946 ("bonding:

The patch looks good to me. But based on your description, I don't think
the fixes tag is correct.

Thanks
Hangbin
>  update port speed when getting bond speed") this race gets more visible,
> if user space is calling ethtool on a regular base.
> 
> Fix this by only setting speed/duplex to unknown, if link speed is
> really unknown/unusable.
> 
> Fixes: 691b2bf14946 ("bonding: update port speed when getting bond speed")
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> ---
>  drivers/net/bonding/bond_main.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index e7caf400a59c..4cdf89b21ca0 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -791,26 +791,29 @@ static int bond_update_speed_duplex(struct slave *slave)
>  	struct ethtool_link_ksettings ecmd;
>  	int res;
>  
> -	slave->speed = SPEED_UNKNOWN;
> -	slave->duplex = DUPLEX_UNKNOWN;
> -
>  	res = __ethtool_get_link_ksettings(slave_dev, &ecmd);
>  	if (res < 0)
> -		return 1;
> +		goto speed_duplex_unknown;
>  	if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1))
> -		return 1;
> +		goto speed_duplex_unknown;
>  	switch (ecmd.base.duplex) {
>  	case DUPLEX_FULL:
>  	case DUPLEX_HALF:
>  		break;
>  	default:
> -		return 1;
> +		goto speed_duplex_unknown;
>  	}
>  
>  	slave->speed = ecmd.base.speed;
>  	slave->duplex = ecmd.base.duplex;
>  
>  	return 0;
> +
> +speed_duplex_unknown:
> +	slave->speed = SPEED_UNKNOWN;
> +	slave->duplex = DUPLEX_UNKNOWN;
> +
> +	return 1;
>  }
>  
>  const char *bond_slave_link_status(s8 link)
> -- 
> 2.43.0
>
Re: [PATCH net] bonding: only set speed/duplex to unknown, if getting speed failed
Posted by Thomas Bogendoerfer 4 days, 17 hours ago 
On Fri, 30 Jan 2026 12:36:19 +0000
Hangbin Liu <liuhangbin@gmail.com> wrote:

> On Fri, Jan 30, 2026 at 12:19:04PM +0100, Thomas Bogendoerfer wrote:
> > bond_update_speed_duplex() first set speed/duplex to unknown and
> > then asks slave driver for current speed/duplex. Since getting
> > speed/duplex might take longer there is a race, where this false state
> > is visible by /proc/net/bonding. With commit 691b2bf14946 ("bonding:  
> 
> The patch looks good to me. But based on your description, I don't think
> the fixes tag is correct.

the race is old, but it got visible by that commit. Before 
bond_update_speed_duplex() was only called on enslaving and when bond
is brought up. Now it could also be called during normal operation and
that's what caught attention by customers.

I'm fine changing the fixes tag to whatever we agree to. So which should
I take ?

Thomas.

-- 
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Jochen Jaser, Andrew McDonald, Werner Knoblich
Re: [PATCH net] bonding: only set speed/duplex to unknown, if getting speed failed
Posted by Hangbin Liu 4 days, 4 hours ago 
On Mon, Feb 02, 2026 at 03:17:26PM +0100, Thomas Bogendoerfer wrote:
> On Fri, 30 Jan 2026 12:36:19 +0000
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> > On Fri, Jan 30, 2026 at 12:19:04PM +0100, Thomas Bogendoerfer wrote:
> > > bond_update_speed_duplex() first set speed/duplex to unknown and
> > > then asks slave driver for current speed/duplex. Since getting
> > > speed/duplex might take longer there is a race, where this false state
> > > is visible by /proc/net/bonding. With commit 691b2bf14946 ("bonding:  
> > 
> > The patch looks good to me. But based on your description, I don't think
> > the fixes tag is correct.
> 
> the race is old, but it got visible by that commit. Before 
> bond_update_speed_duplex() was only called on enslaving and when bond
> is brought up. Now it could also be called during normal operation and
> that's what caught attention by customers.
> 
> I'm fine changing the fixes tag to whatever we agree to. So which should
> I take ?

Maybe
98f41f694f46 ("bonding:update speed/duplex for NETDEV_CHANGE") and
589665f5a600 ("bonding: comparing a u8 with -1 is always false")?

The 98f41f694f46 set speed/duplex to -1 by default, which could cause the race
to show SPEED_UNKNOWN. But (slave->duplex == -1) checking is always false, so
no possible to show DUPLEX_UNKNOWN. The 589665f5a600 fixed this issue, after
that speed/duplex both could be shown as UNKNOWN.

Thanks
Hangbin
Re: [PATCH net] bonding: only set speed/duplex to unknown, if getting speed failed
Posted by Thomas Bogendoerfer 3 days, 17 hours ago 
On Tue, 3 Feb 2026 03:24:09 +0000
Hangbin Liu <liuhangbin@gmail.com> wrote:

> On Mon, Feb 02, 2026 at 03:17:26PM +0100, Thomas Bogendoerfer wrote:
> > On Fri, 30 Jan 2026 12:36:19 +0000
> > Hangbin Liu <liuhangbin@gmail.com> wrote:
> >   
> > > On Fri, Jan 30, 2026 at 12:19:04PM +0100, Thomas Bogendoerfer wrote:  
> > > > bond_update_speed_duplex() first set speed/duplex to unknown and
> > > > then asks slave driver for current speed/duplex. Since getting
> > > > speed/duplex might take longer there is a race, where this false state
> > > > is visible by /proc/net/bonding. With commit 691b2bf14946 ("bonding:    
> > > 
> > > The patch looks good to me. But based on your description, I don't think
> > > the fixes tag is correct.  
> > 
> > the race is old, but it got visible by that commit. Before 
> > bond_update_speed_duplex() was only called on enslaving and when bond
> > is brought up. Now it could also be called during normal operation and
> > that's what caught attention by customers.
> > 
> > I'm fine changing the fixes tag to whatever we agree to. So which should
> > I take ?  
> 
> Maybe
> 98f41f694f46 ("bonding:update speed/duplex for NETDEV_CHANGE") and
> 589665f5a600 ("bonding: comparing a u8 with -1 is always false")?
> 
> The 98f41f694f46 set speed/duplex to -1 by default, which could cause the
> race to show SPEED_UNKNOWN. But (slave->duplex == -1) checking is always
> false, so no possible to show DUPLEX_UNKNOWN. The 589665f5a600 fixed this
> issue, after that speed/duplex both could be shown as UNKNOWN.

589665f5a600 only replaces the -1 with defines. So it doesn't introduce
semantic changes, but 98f41f694f46 did.

Thomas.

-- 
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Jochen Jaser, Andrew McDonald, Werner Knoblich
Re: [PATCH net] bonding: only set speed/duplex to unknown, if getting speed failed
Posted by Jay Vosburgh 1 week ago 
Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Fri, Jan 30, 2026 at 12:19:04PM +0100, Thomas Bogendoerfer wrote:
>> bond_update_speed_duplex() first set speed/duplex to unknown and
>> then asks slave driver for current speed/duplex. Since getting
>> speed/duplex might take longer there is a race, where this false state
>> is visible by /proc/net/bonding. With commit 691b2bf14946 ("bonding:
>
>The patch looks good to me. But based on your description, I don't think
>the fixes tag is correct.

	Agreed on both points; I suspect the origin of the race window
is:

commit e9fe8efeeae11f19bb6fafd6153ec77deaeb4b83
Author: Nikolay Aleksandrov <razor@blackwall.org>
Date:   Tue Sep 9 23:17:01 2014 +0200

    bonding: procfs: clean bond->lock usage and use RCU

	as this patch converted some locking in the procfs logic to be
solely RCU.

	-J

>Thanks
>Hangbin
>>  update port speed when getting bond speed") this race gets more visible,
>> if user space is calling ethtool on a regular base.
>> 
>> Fix this by only setting speed/duplex to unknown, if link speed is
>> really unknown/unusable.
>> 
>> Fixes: 691b2bf14946 ("bonding: update port speed when getting bond speed")
>> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
>> ---
>>  drivers/net/bonding/bond_main.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index e7caf400a59c..4cdf89b21ca0 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -791,26 +791,29 @@ static int bond_update_speed_duplex(struct slave *slave)
>>  	struct ethtool_link_ksettings ecmd;
>>  	int res;
>>  
>> -	slave->speed = SPEED_UNKNOWN;
>> -	slave->duplex = DUPLEX_UNKNOWN;
>> -
>>  	res = __ethtool_get_link_ksettings(slave_dev, &ecmd);
>>  	if (res < 0)
>> -		return 1;
>> +		goto speed_duplex_unknown;
>>  	if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1))
>> -		return 1;
>> +		goto speed_duplex_unknown;
>>  	switch (ecmd.base.duplex) {
>>  	case DUPLEX_FULL:
>>  	case DUPLEX_HALF:
>>  		break;
>>  	default:
>> -		return 1;
>> +		goto speed_duplex_unknown;
>>  	}
>>  
>>  	slave->speed = ecmd.base.speed;
>>  	slave->duplex = ecmd.base.duplex;
>>  
>>  	return 0;
>> +
>> +speed_duplex_unknown:
>> +	slave->speed = SPEED_UNKNOWN;
>> +	slave->duplex = DUPLEX_UNKNOWN;
>> +
>> +	return 1;
>>  }
>>  
>>  const char *bond_slave_link_status(s8 link)
>> -- 
>> 2.43.0
>> 

---
	-Jay Vosburgh, jv@jvosburgh.net
Re: [PATCH net] bonding: only set speed/duplex to unknown, if getting speed failed
Posted by Nikolay Aleksandrov 1 week ago 
On 30/01/2026 17:38, Jay Vosburgh wrote:
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
>> On Fri, Jan 30, 2026 at 12:19:04PM +0100, Thomas Bogendoerfer wrote:
>>> bond_update_speed_duplex() first set speed/duplex to unknown and
>>> then asks slave driver for current speed/duplex. Since getting
>>> speed/duplex might take longer there is a race, where this false state
>>> is visible by /proc/net/bonding. With commit 691b2bf14946 ("bonding:
>>
>> The patch looks good to me. But based on your description, I don't think
>> the fixes tag is correct.
> 
> 	Agreed on both points; I suspect the origin of the race window
> is:
> 
> commit e9fe8efeeae11f19bb6fafd6153ec77deaeb4b83
> Author: Nikolay Aleksandrov <razor@blackwall.org>
> Date:   Tue Sep 9 23:17:01 2014 +0200
> 
>      bonding: procfs: clean bond->lock usage and use RCU
> 
> 	as this patch converted some locking in the procfs logic to be
> solely RCU.
> 
> 	-J
> 

I don't think so. :-)
bond_update_speed_duplex() can sleep and has never been called with the write_lock.
See for example:
  commit 876254ae2758
  Author: Veaceslav Falico <vfalico@redhat.com>
  Date:   Tue Mar 12 06:31:32 2013 +0000

     bonding: don't call update_speed_duplex() under spinlocks

It seems to me that behaviour has always been there, regardless of having a
read_lock in procfs or not.

Cheers,
  Nik

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.