bonding: don't use stale speed and duplex information

There is presently a race condition between the bonding periodic
link monitor and the updating of a slave's speed and duplex. The former occurs on a periodic basis, and the latter in response to a driver's calling of netif_carrier_on.

It is possible for the periodic monitor to run between the
driver call of netif_carrier_on and the receipt of the NETDEV_CHANGE event that causes bonding to update the slave's speed and duplex. This manifests most notably as a report that a slave is up and "0 Mbps full duplex" after enslavement, but in principle could report an incorrect speed and duplex after any link up event if the device comes up with a different speed or duplex. This affects the 802.3ad aggregator selection, as the speed and duplex are selection criteria.

This is fixed by updating the speed and duplex in the periodic
monitor, prior to using that information.

This was done historically in bonding, but the call to
bond_update_speed_duplex was removed in commit 876254ae2758 ("bonding:
don't call update_speed_duplex() under spinlocks"), as it might sleep under lock. Later, the locking was changed to only hold RTNL, and so after commit 876254ae2758 ("bonding: don't call update_speed_duplex() under spinlocks") this call is again safe.

Tested-by "Tantilov, Emil S" <[email protected]>

Cc Veaceslav Falico <[email protected]>

Cc dingtianhong <[email protected]>

Fixes 876254ae2758 ("bonding: don't call update_speed_duplex() under spinlocks")

Signed-off-by Jay Vosburgh <[email protected]>

---

v2: Correct Veaceslav's email address

Note: The "Fixes" commit is the commit that makes this operation safe again, not the commit that originally introduced the race. I don't see any simple way to resolve this bug between these two commits.

 drivers/net/bonding/bond_main.c | 1 +
 1 file changed, 1 insertion(+)

drivers/net/bonding/bond_main.c +1 -0

index 56b560558884..cabaeb61333d 100644
@@ -2127,6 +2127,7 @@ static void bond_miimon_commit(struct bonding *bond)
 			continue;
 
 		case BOND_LINK_UP:
+			bond_update_speed_duplex(slave);
 			bond_set_slave_link_state(slave, BOND_LINK_UP,
 						  BOND_SLAVE_NOTIFY_NOW);
 			slave->last_link_up = jiffies;

There is presently a race condition between the bonding periodic
link monitor and the updating of a slave's speed and duplex. The former occurs on a periodic basis, and the latter in response to a driver's calling of netif_carrier_on.

It is possible for the periodic monitor to run between the
driver call of netif_carrier_on and the receipt of the NETDEV_CHANGE event that causes bonding to update the slave's speed and duplex. This manifests most notably as a report that a slave is up and "0 Mbps full duplex" after enslavement, but in principle could report an incorrect speed and duplex after any link up event if the device comes up with a different speed or duplex. This affects the 802.3ad aggregator selection, as the speed and duplex are selection criteria.

This is fixed by updating the speed and duplex in the periodic
monitor, prior to using that information.

This was done historically in bonding, but the call to
bond_update_speed_duplex was removed in commit 876254ae2758 ("bonding:
don't call update_speed_duplex() under spinlocks"), as it might sleep under lock. Later, the locking was changed to only hold RTNL, and so after commit 876254ae2758 ("bonding: don't call update_speed_duplex() under spinlocks") this call is again safe.

Tested-by "Tantilov, Emil S" <[email protected]>

Cc Veaceslav Falico <[email protected]>

Cc dingtianhong <[email protected]>

Fixes 876254ae2758 ("bonding: don't call update_speed_duplex() under spinlocks")

Signed-off-by Jay Vosburgh <[email protected]>

Quoted 7 lines

This was done historically in bonding, but the call to
bond_update_speed_duplex was removed in commit 876254ae2758 ("bonding:
don't call update_speed_duplex() under spinlocks"), as it might sleep under lock. Later, the locking was changed to only hold RTNL, and so after commit 876254ae2758 ("bonding: don't call update_speed_duplex() under spinlocks") this call is again safe.

Quoted 5 lines

Tested-by "Tantilov, Emil S" <[email protected]>

Cc Veaceslav Falico <[email protected]>

Cc dingtianhong <[email protected]>

Fixes 876254ae2758 ("bonding: don't call update_speed_duplex() under spinlocks")

Signed-off-by Jay Vosburgh <[email protected]>

Applied, thanks Jay.

David Miller <[email protected]> wrote:
[...]

Quoted 7 lines

Quoted 7 lines

This was done historically in bonding, but the call to
bond_update_speed_duplex was removed in commit 876254ae2758 ("bonding:
don't call update_speed_duplex() under spinlocks"), as it might sleep under lock. Later, the locking was changed to only hold RTNL, and so after commit 876254ae2758 ("bonding: don't call update_speed_duplex() under spinlocks") this call is again safe.

Quoted 7 lines

Quoted 5 lines

Tested-by "Tantilov, Emil S" <[email protected]>

Cc Veaceslav Falico <[email protected]>

Cc dingtianhong <[email protected]>

Fixes 876254ae2758 ("bonding: don't call update_speed_duplex() under spinlocks")

Signed-off-by Jay Vosburgh <[email protected]>

Applied, thanks Jay.

Rereading the above, I just noticed that I put the wrong commit
into the fixes tag (and the "Later, the locking was changed" text); the correct fixes tag should be:

Fixes 4cb4f97b7e36 ("bonding: rebuild the lock use for bond_mii_monitor()")

	Kernels between 876254ae2758 and 4cb4f97b7e36 should not have

this patch applied, as it might sleep under lock.

Sorry for the error,

There is presently a race condition between the bonding periodic
link monitor and the updating of a slave's speed and duplex. The former occurs on a periodic basis, and the latter in response to a driver's calling of netif_carrier_on.

It is possible for the periodic monitor to run between the
driver call of netif_carrier_on and the receipt of the NETDEV_CHANGE event that causes bonding to update the slave's speed and duplex. This manifests most notably as a report that a slave is up and "0 Mbps full duplex" after enslavement, but in principle could report an incorrect speed and duplex after any link up event if the device comes up with a different speed or duplex. This affects the 802.3ad aggregator selection, as the speed and duplex are selection criteria.

This is fixed by updating the speed and duplex in the periodic
monitor, prior to using that information.

This was done historically in bonding, but the call to
bond_update_speed_duplex was removed in commit 876254ae2758 ("bonding:
don't call update_speed_duplex() under spinlocks"), as it might sleep under lock. Later, the locking was changed to only hold RTNL, and so after commit 876254ae2758 ("bonding: don't call update_speed_duplex() under spinlocks") this call is again safe.

Tested-by "Tantilov, Emil S" <[email protected]>

Cc Veaceslav Falico <[email protected]>

Cc dingtianhong <[email protected]>

Fixes 876254ae2758 ("bonding: don't call update_speed_duplex() under spinlocks")

Signed-off-by Jay Vosburgh <[email protected]>

---

v2: Correct Veaceslav's email address

Note: The "Fixes" commit is the commit that makes this operation safe again, not the commit that originally introduced the race. I don't see any simple way to resolve this bug between these two commits.

  drivers/net/bonding/bond_main.c | 1 +
  1 file changed, 1 insertion(+)

drivers/net/bonding/bond_main.c +0 -0

index 56b560558884..cabaeb61333d 100644

I delved into the source code and Emil's tests. I think that the problem that this patch expects to fix occurs very unusually.

Do you agree with me?

If so, maybe the following patch can reduce the performance loss. Please comment on it. Thanks a lot.

drivers/net/bonding/bond_main.c +0 -0

index b7f1a99..c4c511a 100644

zhuyj <[email protected]> wrote:
[...]

Quoted 17 lines

I delved into the source code and Emil's tests. I think that the problem that this patch expects to fix occurs very unusually.

Do you agree with me?

If so, maybe the following patch can reduce the performance loss. Please comment on it. Thanks a lot.

drivers/net/bonding/bond_main.c +0 -0

index b7f1a99..c4c511a 100644
@@ -2129,7 +2129,9 @@ static void bond_miimon_commit(struct bonding *bond)
                        continue;
 

drivers/net/bonding/bond_main.c static void bond_miimon_commit(struct bonding *bond)

Quoted 8 lines

drivers/net/bonding/bond_main.c +3 -1

                case BOND_LINK_UP:
-                       bond_update_speed_duplex(slave);
+                       if (slave->speed == SPEED_UNKNOWN)
+                               bond_update_speed_duplex(slave);
+
                        bond_set_slave_link_state(slave, BOND_LINK_UP,
 BOND_SLAVE_NOTIFY_NOW);
                        slave->last_link_up = jiffies;

I don't believe the speed is necessarily SPEED_UNKNOWN coming in
here. If the race occurs at a time later than the initial enslavement, speed may already be set (and the race manifests if the new speed changes, i.e., the link changes from 1 Gb/sec to 10 Gb/sec), so I don't think this is functionally correct.

Also, the call to bond_miimon_commit itself is already gated by
bond_miimon_inspect finding a link state change. The performance impact here should be minimal.

	-J

---
-Jay Vosburgh, [email protected]

Quoted 19 lines · Jay Vosburgh

zhuyj <[email protected]> wrote:
[...]

Quoted 17 lines

I delved into the source code and Emil's tests. I think that the problem that this patch expects to fix occurs very unusually.

Do you agree with me?

If so, maybe the following patch can reduce the performance loss. Please comment on it. Thanks a lot.

drivers/net/bonding/bond_main.c +0 -0

index b7f1a99..c4c511a 100644
@@ -2129,7 +2129,9 @@ static void bond_miimon_commit(struct bonding *bond)
                        continue;
 

Quoted 13 lines

drivers/net/bonding/bond_main.c static void bond_miimon_commit(struct bonding *bond)

Quoted 8 lines

drivers/net/bonding/bond_main.c +3 -1

                case BOND_LINK_UP:
-                       bond_update_speed_duplex(slave);
+                       if (slave->speed == SPEED_UNKNOWN)
+                               bond_update_speed_duplex(slave);
+
                        bond_set_slave_link_state(slave, BOND_LINK_UP,
 BOND_SLAVE_NOTIFY_NOW);
                        slave->last_link_up = jiffies;

I don't believe the speed is necessarily SPEED_UNKNOWN coming in
here. If the race occurs at a time later than the initial enslavement, speed may already be set (and the race manifests if the new speed changes, i.e., the link changes from 1 Gb/sec to 10 Gb/sec), so I don't think this is functionally correct.

Hi, Jay

Thanks for your reply.

IMHO, "If the race occurs at a time later than the initial enslavement, speed may already be set (and the race manifests if the new speed changes, i.e., the link changes from 1 Gb/sec to 10 Gb/sec)", from my test, this will not happen because the previous source code make the speed correct.

This "bond_update_speed_duplex" repeats to get the correct speed.

That is, this patch is to fix the error in initial enslavement. The mentioned scenario will not occur.

Even though the performance impact is minimal, if we can avoid this performance
impact, why not ?

Best Regards!
Zhu Yanjun

Quoted 6 lines

Also, the call to bond_miimon_commit itself is already gated by
bond_miimon_inspect finding a link state change. The performance impact here should be minimal.

	-J

Quoted 35 lines · zhuyj

Quoted 19 lines · Jay Vosburgh

zhuyj <[email protected]> wrote:
[...]

Quoted 17 lines

I delved into the source code and Emil's tests. I think that the problem that this patch expects to fix occurs very unusually.

Do you agree with me?

If so, maybe the following patch can reduce the performance loss. Please comment on it. Thanks a lot.

drivers/net/bonding/bond_main.c +0 -0

index b7f1a99..c4c511a 100644
@@ -2129,7 +2129,9 @@ static void bond_miimon_commit(struct bonding *bond)
                         continue;
 

Quoted 13 lines

drivers/net/bonding/bond_main.c static void bond_miimon_commit(struct bonding *bond)

Quoted 8 lines

drivers/net/bonding/bond_main.c +3 -1

                 case BOND_LINK_UP:
-                       bond_update_speed_duplex(slave);
+                       if (slave->speed == SPEED_UNKNOWN)
+                               bond_update_speed_duplex(slave);
+
                         bond_set_slave_link_state(slave, BOND_LINK_UP,
 BOND_SLAVE_NOTIFY_NOW);
                         slave->last_link_up = jiffies;

I don't believe the speed is necessarily SPEED_UNKNOWN coming in
here. If the race occurs at a time later than the initial enslavement, speed may already be set (and the race manifests if the new speed changes, i.e., the link changes from 1 Gb/sec to 10 Gb/sec), so I don't think this is functionally correct.

Hi, Jay

Quoted 7 lines

Thanks for your reply.

IMHO, "If the race occurs at a time later than the initial enslavement, speed may already be set (and the race manifests if the new speed changes, i.e., the link changes from 1 Gb/sec to 10 Gb/sec)", from my test, this will not happen because the previous source code make the speed correct.

How, exactly, will "the previous source code make the speed
correct"?

Quoted 4 lines

This "bond_update_speed_duplex" repeats to get the correct speed.

That is, this patch is to fix the error in initial enslavement. The mentioned scenario will not occur.

I see nothing in the code that limits the race to happening only
at enslavement time.

If the bond_mii_monitor call executes between the device going
link up and the arrival of the NETDEV_CHANGE or NETDEV_UP callback, the stored speed and duplex are stale. The stale speed value is not guaranteed to be SPEED_UNKNOWN, so your patch is not functionally correct.

	-J

Quoted 4 lines

Even though the performance impact is minimal, if we can avoid this performance
impact, why not ?

Quoted 8 lines

Best Regards!
Zhu Yanjun

Quoted 5 lines

Also, the call to bond_miimon_commit itself is already gated by
bond_miimon_inspect finding a link state change. The performance impact here should be minimal.

	-J

---
-Jay Vosburgh, [email protected]