This is caused by the global variable ad_ticks_per_sec being zero as
demonstrated by the reproducer script discussed below. This causes
all timer values in __ad_timer_to_ticks to be zero, resulting
in the periodic timer to never fire.
To reproduce:
Run the script in
`tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh` which
puts bonding into a state where it never transmits LACPDUs.
line 44: ip link add fbond type bond mode 4 miimon 200 \
xmit_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast
setting bond param: ad_actor_sys_prio
given:
params.ad_actor_system = 0
call stack:
bond_option_ad_actor_sys_prio()
-> bond_3ad_update_ad_actor_settings()
-> set ad.system.sys_priority = bond->params.ad_actor_sys_prio
-> ad.system.sys_mac_addr = bond->dev->dev_addr; because
params.ad_actor_system == 0
results:
ad.system.sys_mac_addr = bond->dev->dev_addr
line 48: ip link set fbond address 52:54:00:3B:7C:A6
setting bond MAC addr
call stack:
bond->dev->dev_addr = new_mac
line 52: ip link set fbond type bond ad_actor_sys_prio 65535
setting bond param: ad_actor_sys_prio
given:
params.ad_actor_system = 0
call stack:
bond_option_ad_actor_sys_prio()
-> bond_3ad_update_ad_actor_settings()
-> set ad.system.sys_priority = bond->params.ad_actor_sys_prio
-> ad.system.sys_mac_addr = bond->dev->dev_addr; because
params.ad_actor_system == 0
results:
ad.system.sys_mac_addr = bond->dev->dev_addr
line 60: ip link set veth1-bond down master fbond
given:
params.ad_actor_system = 0
params.mode = BOND_MODE_8023AD
ad.system.sys_mac_addr == bond->dev->dev_addr
call stack:
bond_enslave
-> bond_3ad_initialize(); because first slave
-> if ad.system.sys_mac_addr != bond->dev->dev_addr
return
results:
Nothing is run in bond_3ad_initialize() because dev_add equals
sys_mac_addr leaving the global ad_ticks_per_sec zero as it is
never initialized anywhere else.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Jonathan Toppins <[email protected]>
---
Notes:
v2:
* split this fix from the reproducer
v3:
* rebased to latest net/master
drivers/net/bonding/bond_3ad.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index d7fb33c078e8..957d30db6f95 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -84,7 +84,8 @@ enum ad_link_speed_type {
static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = {
0, 0, 0, 0, 0, 0
};
-static u16 ad_ticks_per_sec;
+
+static u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL;
static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000;
static const u8 lacpdu_mcast_addr[ETH_ALEN + 2] __long_aligned =
--
2.31.1
On Mon, Aug 15, 2022 at 11:08:35AM -0400, Jonathan Toppins wrote:
> This is caused by the global variable ad_ticks_per_sec being zero as
> demonstrated by the reproducer script discussed below. This causes
> all timer values in __ad_timer_to_ticks to be zero, resulting
> in the periodic timer to never fire.
>
> To reproduce:
> Run the script in
> `tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh` which
> puts bonding into a state where it never transmits LACPDUs.
>
> line 44: ip link add fbond type bond mode 4 miimon 200 \
> xmit_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast
> setting bond param: ad_actor_sys_prio
> given:
> params.ad_actor_system = 0
> call stack:
> bond_option_ad_actor_sys_prio()
> -> bond_3ad_update_ad_actor_settings()
> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio
> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because
> params.ad_actor_system == 0
> results:
> ad.system.sys_mac_addr = bond->dev->dev_addr
>
> line 48: ip link set fbond address 52:54:00:3B:7C:A6
> setting bond MAC addr
> call stack:
> bond->dev->dev_addr = new_mac
>
> line 52: ip link set fbond type bond ad_actor_sys_prio 65535
> setting bond param: ad_actor_sys_prio
> given:
> params.ad_actor_system = 0
> call stack:
> bond_option_ad_actor_sys_prio()
> -> bond_3ad_update_ad_actor_settings()
> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio
> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because
> params.ad_actor_system == 0
> results:
> ad.system.sys_mac_addr = bond->dev->dev_addr
>
> line 60: ip link set veth1-bond down master fbond
> given:
> params.ad_actor_system = 0
> params.mode = BOND_MODE_8023AD
> ad.system.sys_mac_addr == bond->dev->dev_addr
> call stack:
> bond_enslave
> -> bond_3ad_initialize(); because first slave
> -> if ad.system.sys_mac_addr != bond->dev->dev_addr
> return
> results:
> Nothing is run in bond_3ad_initialize() because dev_add equals
> sys_mac_addr leaving the global ad_ticks_per_sec zero as it is
> never initialized anywhere else.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jonathan Toppins <[email protected]>
> ---
>
> Notes:
> v2:
> * split this fix from the reproducer
> v3:
> * rebased to latest net/master
>
> drivers/net/bonding/bond_3ad.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index d7fb33c078e8..957d30db6f95 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -84,7 +84,8 @@ enum ad_link_speed_type {
> static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = {
> 0, 0, 0, 0, 0, 0
> };
> -static u16 ad_ticks_per_sec;
> +
> +static u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL;
> static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000;
>
> static const u8 lacpdu_mcast_addr[ETH_ALEN + 2] __long_aligned =
> --
> 2.31.1
>
Acked-by: Hangbin Liu <[email protected]>
Jonathan Toppins <[email protected]> wrote:
>This is caused by the global variable ad_ticks_per_sec being zero as
>demonstrated by the reproducer script discussed below. This causes
>all timer values in __ad_timer_to_ticks to be zero, resulting
>in the periodic timer to never fire.
>
>To reproduce:
>Run the script in
>`tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh` which
>puts bonding into a state where it never transmits LACPDUs.
>
>line 44: ip link add fbond type bond mode 4 miimon 200 \
> xmit_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast
>setting bond param: ad_actor_sys_prio
>given:
> params.ad_actor_system = 0
>call stack:
> bond_option_ad_actor_sys_prio()
> -> bond_3ad_update_ad_actor_settings()
> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio
> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because
> params.ad_actor_system == 0
>results:
> ad.system.sys_mac_addr = bond->dev->dev_addr
>
>line 48: ip link set fbond address 52:54:00:3B:7C:A6
>setting bond MAC addr
>call stack:
> bond->dev->dev_addr = new_mac
>
>line 52: ip link set fbond type bond ad_actor_sys_prio 65535
>setting bond param: ad_actor_sys_prio
>given:
> params.ad_actor_system = 0
>call stack:
> bond_option_ad_actor_sys_prio()
> -> bond_3ad_update_ad_actor_settings()
> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio
> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because
> params.ad_actor_system == 0
>results:
> ad.system.sys_mac_addr = bond->dev->dev_addr
>
>line 60: ip link set veth1-bond down master fbond
>given:
> params.ad_actor_system = 0
> params.mode = BOND_MODE_8023AD
> ad.system.sys_mac_addr == bond->dev->dev_addr
>call stack:
> bond_enslave
> -> bond_3ad_initialize(); because first slave
> -> if ad.system.sys_mac_addr != bond->dev->dev_addr
> return
>results:
> Nothing is run in bond_3ad_initialize() because dev_add equals
> sys_mac_addr leaving the global ad_ticks_per_sec zero as it is
> never initialized anywhere else.
>
>Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>Signed-off-by: Jonathan Toppins <[email protected]>
>---
>
>Notes:
> v2:
> * split this fix from the reproducer
> v3:
> * rebased to latest net/master
>
> drivers/net/bonding/bond_3ad.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index d7fb33c078e8..957d30db6f95 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -84,7 +84,8 @@ enum ad_link_speed_type {
> static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = {
> 0, 0, 0, 0, 0, 0
> };
>-static u16 ad_ticks_per_sec;
>+
>+static u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL;
> static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000;
I still feel like this is kind of a hack, as it's not really
fixing bond_3ad_initialize to actually work (which is the real problem
as I understand it). If it's ok to skip all that for this case, then
why do we ever need to call bond_3ad_initialize?
-J
> static const u8 lacpdu_mcast_addr[ETH_ALEN + 2] __long_aligned =
>--
>2.31.1
>
---
-Jay Vosburgh, [email protected]
On 8/16/22 09:11, Jay Vosburgh wrote:
> Jonathan Toppins <[email protected]> wrote:
>
>> This is caused by the global variable ad_ticks_per_sec being zero as
>> demonstrated by the reproducer script discussed below. This causes
>> all timer values in __ad_timer_to_ticks to be zero, resulting
>> in the periodic timer to never fire.
>>
>> To reproduce:
>> Run the script in
>> `tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh` which
>> puts bonding into a state where it never transmits LACPDUs.
>>
>> line 44: ip link add fbond type bond mode 4 miimon 200 \
>> xmit_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast
>> setting bond param: ad_actor_sys_prio
>> given:
>> params.ad_actor_system = 0
>> call stack:
>> bond_option_ad_actor_sys_prio()
>> -> bond_3ad_update_ad_actor_settings()
>> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio
>> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because
>> params.ad_actor_system == 0
>> results:
>> ad.system.sys_mac_addr = bond->dev->dev_addr
>>
>> line 48: ip link set fbond address 52:54:00:3B:7C:A6
>> setting bond MAC addr
>> call stack:
>> bond->dev->dev_addr = new_mac
>>
>> line 52: ip link set fbond type bond ad_actor_sys_prio 65535
>> setting bond param: ad_actor_sys_prio
>> given:
>> params.ad_actor_system = 0
>> call stack:
>> bond_option_ad_actor_sys_prio()
>> -> bond_3ad_update_ad_actor_settings()
>> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio
>> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because
>> params.ad_actor_system == 0
>> results:
>> ad.system.sys_mac_addr = bond->dev->dev_addr
>>
>> line 60: ip link set veth1-bond down master fbond
>> given:
>> params.ad_actor_system = 0
>> params.mode = BOND_MODE_8023AD
>> ad.system.sys_mac_addr == bond->dev->dev_addr
>> call stack:
>> bond_enslave
>> -> bond_3ad_initialize(); because first slave
>> -> if ad.system.sys_mac_addr != bond->dev->dev_addr
>> return
>> results:
>> Nothing is run in bond_3ad_initialize() because dev_add equals
>> sys_mac_addr leaving the global ad_ticks_per_sec zero as it is
>> never initialized anywhere else.
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Signed-off-by: Jonathan Toppins <[email protected]>
>> ---
>>
>> Notes:
>> v2:
>> * split this fix from the reproducer
>> v3:
>> * rebased to latest net/master
>>
>> drivers/net/bonding/bond_3ad.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index d7fb33c078e8..957d30db6f95 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -84,7 +84,8 @@ enum ad_link_speed_type {
>> static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = {
>> 0, 0, 0, 0, 0, 0
>> };
>> -static u16 ad_ticks_per_sec;
>> +
>> +static u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL;
>> static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000;
>
> I still feel like this is kind of a hack, as it's not really
> fixing bond_3ad_initialize to actually work (which is the real problem
> as I understand it). If it's ok to skip all that for this case, then
> why do we ever need to call bond_3ad_initialize?
>
The way it is currently written you still need to call
bond_3ad_initialize() just not for setting the tick resolution. The
issue here is ad_ticks_per_sec is used in several places to calculate
timer periods, __ad_timer_to_ticks(), for various timers in the 802.3ad
protocol. And if this variable, ad_ticks_per_sec, is left uninitialized
all of these timer periods go to zero. Since the value passed in
bond_3ad_initialize() is an immediate value I simply moved it off of the
call stack and set the static global variable instead.
To fix bond_3ad_initialize(), probably something like the below is
needed, but I do not understand why the guard if check was placed in
bond_3ad_initialize().
diff --git i/drivers/net/bonding/bond_3ad.c w/drivers/net/bonding/bond_3ad.c
index d7fb33c078e8..5b5146f5c4ea 100644
--- i/drivers/net/bonding/bond_3ad.c
+++ w/drivers/net/bonding/bond_3ad.c
@@ -2005,32 +2005,21 @@ void bond_3ad_initiate_agg_selection(struct
bonding *bond, int timeout)
*
* Can be called only after the mac address of the bond is set.
*/
-void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution)
+void bond_3ad_initialize(struct bonding *bond)
{
- /* check that the bond is not initialized yet */
- if (!MAC_ADDRESS_EQUAL(&(BOND_AD_INFO(bond).system.sys_mac_addr),
- bond->dev->dev_addr)) {
-
- BOND_AD_INFO(bond).aggregator_identifier = 0;
-
- BOND_AD_INFO(bond).system.sys_priority =
- bond->params.ad_actor_sys_prio;
- if (is_zero_ether_addr(bond->params.ad_actor_system))
- BOND_AD_INFO(bond).system.sys_mac_addr =
- *((struct mac_addr *)bond->dev->dev_addr);
- else
- BOND_AD_INFO(bond).system.sys_mac_addr =
- *((struct mac_addr *)bond->params.ad_actor_system);
-
- /* initialize how many times this module is called in one
- * second (should be about every 100ms)
- */
- ad_ticks_per_sec = tick_resolution;
+ BOND_AD_INFO(bond).aggregator_identifier = 0;
+ BOND_AD_INFO(bond).system.sys_priority =
+ bond->params.ad_actor_sys_prio;
+ if (is_zero_ether_addr(bond->params.ad_actor_system))
+ BOND_AD_INFO(bond).system.sys_mac_addr =
+ *((struct mac_addr *)bond->dev->dev_addr);
+ else
+ BOND_AD_INFO(bond).system.sys_mac_addr =
+ *((struct mac_addr *)bond->params.ad_actor_system);
- bond_3ad_initiate_agg_selection(bond,
- AD_AGGREGATOR_SELECTION_TIMER *
- ad_ticks_per_sec);
- }
+ bond_3ad_initiate_agg_selection(bond,
+ AD_AGGREGATOR_SELECTION_TIMER *
+ ad_ticks_per_sec);
}
/**
diff --git i/drivers/net/bonding/bond_main.c
w/drivers/net/bonding/bond_main.c
index 50e60843020c..5f56af9dc3ba 100644
--- i/drivers/net/bonding/bond_main.c
+++ w/drivers/net/bonding/bond_main.c
@@ -2078,10 +2078,10 @@ int bond_enslave(struct net_device *bond_dev,
struct net_device *slave_dev,
/* if this is the first slave */
if (!prev_slave) {
SLAVE_AD_INFO(new_slave)->id = 1;
- /* Initialize AD with the number of times that the AD timer is
called in 1 second
- * can be called only after the mac address of the bond is set
+ /* can be called only after the mac address of the
+ * bond is set
*/
- bond_3ad_initialize(bond, 1000/AD_TIMER_INTERVAL);
+ bond_3ad_initialize(bond);
} else {
SLAVE_AD_INFO(new_slave)->id =
SLAVE_AD_INFO(prev_slave)->id + 1;
diff --git i/include/net/bond_3ad.h w/include/net/bond_3ad.h
index 184105d68294..be2992e6de5d 100644
--- i/include/net/bond_3ad.h
+++ w/include/net/bond_3ad.h
@@ -290,7 +290,7 @@ static inline const char
*bond_3ad_churn_desc(churn_state_t state)
}
/* ========== AD Exported functions to the main bonding code ========== */
-void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution);
+void bond_3ad_initialize(struct bonding *bond);
void bond_3ad_bind_slave(struct slave *slave);
void bond_3ad_unbind_slave(struct slave *slave);
void bond_3ad_state_machine_handler(struct work_struct *);
-Jon
On 8/16/22 09:41, Jonathan Toppins wrote:
> On 8/16/22 09:11, Jay Vosburgh wrote:
>> Jonathan Toppins <[email protected]> wrote:
>>
>>> This is caused by the global variable ad_ticks_per_sec being zero as
>>> demonstrated by the reproducer script discussed below. This causes
>>> all timer values in __ad_timer_to_ticks to be zero, resulting
>>> in the periodic timer to never fire.
>>>
>>> To reproduce:
>>> Run the script in
>>> `tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh`
>>> which
>>> puts bonding into a state where it never transmits LACPDUs.
>>>
>>> line 44: ip link add fbond type bond mode 4 miimon 200 \
>>> xmit_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast
>>> setting bond param: ad_actor_sys_prio
>>> given:
>>> params.ad_actor_system = 0
>>> call stack:
>>> bond_option_ad_actor_sys_prio()
>>> -> bond_3ad_update_ad_actor_settings()
>>> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio
>>> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because
>>> params.ad_actor_system == 0
>>> results:
>>> ad.system.sys_mac_addr = bond->dev->dev_addr
>>>
>>> line 48: ip link set fbond address 52:54:00:3B:7C:A6
>>> setting bond MAC addr
>>> call stack:
>>> bond->dev->dev_addr = new_mac
>>>
>>> line 52: ip link set fbond type bond ad_actor_sys_prio 65535
>>> setting bond param: ad_actor_sys_prio
>>> given:
>>> params.ad_actor_system = 0
>>> call stack:
>>> bond_option_ad_actor_sys_prio()
>>> -> bond_3ad_update_ad_actor_settings()
>>> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio
>>> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because
>>> params.ad_actor_system == 0
>>> results:
>>> ad.system.sys_mac_addr = bond->dev->dev_addr
>>>
>>> line 60: ip link set veth1-bond down master fbond
>>> given:
>>> params.ad_actor_system = 0
>>> params.mode = BOND_MODE_8023AD
>>> ad.system.sys_mac_addr == bond->dev->dev_addr
>>> call stack:
>>> bond_enslave
>>> -> bond_3ad_initialize(); because first slave
>>> -> if ad.system.sys_mac_addr != bond->dev->dev_addr
>>> return
>>> results:
>>> Nothing is run in bond_3ad_initialize() because dev_add equals
>>> sys_mac_addr leaving the global ad_ticks_per_sec zero as it is
>>> never initialized anywhere else.
>>>
>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>> Signed-off-by: Jonathan Toppins <[email protected]>
>>> ---
>>>
>>> Notes:
>>> v2:
>>> * split this fix from the reproducer
>>> v3:
>>> * rebased to latest net/master
>>>
>>> drivers/net/bonding/bond_3ad.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_3ad.c
>>> b/drivers/net/bonding/bond_3ad.c
>>> index d7fb33c078e8..957d30db6f95 100644
>>> --- a/drivers/net/bonding/bond_3ad.c
>>> +++ b/drivers/net/bonding/bond_3ad.c
>>> @@ -84,7 +84,8 @@ enum ad_link_speed_type {
>>> static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = {
>>> 0, 0, 0, 0, 0, 0
>>> };
>>> -static u16 ad_ticks_per_sec;
>>> +
>>> +static u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL;
>>> static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000;
>>
>> I still feel like this is kind of a hack, as it's not really
>> fixing bond_3ad_initialize to actually work (which is the real problem
>> as I understand it). If it's ok to skip all that for this case, then
>> why do we ever need to call bond_3ad_initialize?
>>
>
> The way it is currently written you still need to call
> bond_3ad_initialize() just not for setting the tick resolution. The
> issue here is ad_ticks_per_sec is used in several places to calculate
> timer periods, __ad_timer_to_ticks(), for various timers in the 802.3ad
> protocol. And if this variable, ad_ticks_per_sec, is left uninitialized
> all of these timer periods go to zero. Since the value passed in
> bond_3ad_initialize() is an immediate value I simply moved it off of the
> call stack and set the static global variable instead.
>
> To fix bond_3ad_initialize(), probably something like the below is
> needed, but I do not understand why the guard if check was placed in
> bond_3ad_initialize().
I looked at the history of the if guard in bond_3ad_initialize and it
has existed since the creation of the git tree. It appears since commit
5ee14e6d336f ("bonding: 3ad: apply ad_actor settings changes
immediately") the if guard is no longer needed and removing the if guard
would also fix the problem, I have not tested yet.
I think this patch series can be accepted as-is because, it does fix the
issue as demonstrated by the kselftest accompanying the series and is
the smallest change to fix the issue.
Further, I don't see why we want to set the file-scoped variable,
ad_ticks_per_sec, inside bond_3ad_initialize() as ad_ticks_per_sec is
utilized across all bonds. It seems like ad_ticks_per_sec should be
changed to a const and set at the top of the file. I see no value in
passing the value as an unnamed constant on the stack when
bond_3ad_initialize is called. These changes however could be done in
the net-next tree as follow-on cleanup patches.
Jay, how would you like to proceed?
[...]
Thanks,
-Jon
Jonathan Toppins <[email protected]> wrote:
>On 8/16/22 09:41, Jonathan Toppins wrote:
>> On 8/16/22 09:11, Jay Vosburgh wrote:
>>> Jonathan Toppins <[email protected]> wrote:
>>>
>>>> This is caused by the global variable ad_ticks_per_sec being zero as
>>>> demonstrated by the reproducer script discussed below. This causes
>>>> all timer values in __ad_timer_to_ticks to be zero, resulting
>>>> in the periodic timer to never fire.
>>>>
>>>> To reproduce:
>>>> Run the script in
>>>> `tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh`
>>>> which
>>>> puts bonding into a state where it never transmits LACPDUs.
>>>>
>>>> line 44: ip link add fbond type bond mode 4 miimon 200 \
>>>> xmit_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast
>>>> setting bond param: ad_actor_sys_prio
>>>> given:
>>>> params.ad_actor_system = 0
>>>> call stack:
>>>> bond_option_ad_actor_sys_prio()
>>>> -> bond_3ad_update_ad_actor_settings()
>>>> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio
>>>> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because
>>>> params.ad_actor_system == 0
>>>> results:
>>>> ad.system.sys_mac_addr = bond->dev->dev_addr
>>>>
>>>> line 48: ip link set fbond address 52:54:00:3B:7C:A6
>>>> setting bond MAC addr
>>>> call stack:
>>>> bond->dev->dev_addr = new_mac
>>>>
>>>> line 52: ip link set fbond type bond ad_actor_sys_prio 65535
>>>> setting bond param: ad_actor_sys_prio
>>>> given:
>>>> params.ad_actor_system = 0
>>>> call stack:
>>>> bond_option_ad_actor_sys_prio()
>>>> -> bond_3ad_update_ad_actor_settings()
>>>> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio
>>>> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because
>>>> params.ad_actor_system == 0
>>>> results:
>>>> ad.system.sys_mac_addr = bond->dev->dev_addr
>>>>
>>>> line 60: ip link set veth1-bond down master fbond
>>>> given:
>>>> params.ad_actor_system = 0
>>>> params.mode = BOND_MODE_8023AD
>>>> ad.system.sys_mac_addr == bond->dev->dev_addr
>>>> call stack:
>>>> bond_enslave
>>>> -> bond_3ad_initialize(); because first slave
>>>> -> if ad.system.sys_mac_addr != bond->dev->dev_addr
>>>> return
>>>> results:
>>>> Nothing is run in bond_3ad_initialize() because dev_add equals
>>>> sys_mac_addr leaving the global ad_ticks_per_sec zero as it is
>>>> never initialized anywhere else.
>>>>
>>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>>> Signed-off-by: Jonathan Toppins <[email protected]>
>>>> ---
>>>>
>>>> Notes:
>>>> v2:
>>>> * split this fix from the reproducer
>>>> v3:
>>>> * rebased to latest net/master
>>>>
>>>> drivers/net/bonding/bond_3ad.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/bonding/bond_3ad.c
>>>> b/drivers/net/bonding/bond_3ad.c
>>>> index d7fb33c078e8..957d30db6f95 100644
>>>> --- a/drivers/net/bonding/bond_3ad.c
>>>> +++ b/drivers/net/bonding/bond_3ad.c
>>>> @@ -84,7 +84,8 @@ enum ad_link_speed_type {
>>>> static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = {
>>>> 0, 0, 0, 0, 0, 0
>>>> };
>>>> -static u16 ad_ticks_per_sec;
>>>> +
>>>> +static u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL;
>>>> static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000;
>>>
>>> I still feel like this is kind of a hack, as it's not really
>>> fixing bond_3ad_initialize to actually work (which is the real problem
>>> as I understand it). If it's ok to skip all that for this case, then
>>> why do we ever need to call bond_3ad_initialize?
>>>
>> The way it is currently written you still need to call
>> bond_3ad_initialize() just not for setting the tick resolution. The
>> issue here is ad_ticks_per_sec is used in several places to calculate
>> timer periods, __ad_timer_to_ticks(), for various timers in the 802.3ad
>> protocol. And if this variable, ad_ticks_per_sec, is left uninitialized
>> all of these timer periods go to zero. Since the value passed in
>> bond_3ad_initialize() is an immediate value I simply moved it off of the
>> call stack and set the static global variable instead.
>> To fix bond_3ad_initialize(), probably something like the below is
>> needed, but I do not understand why the guard if check was placed in
>> bond_3ad_initialize().
>
>I looked at the history of the if guard in bond_3ad_initialize and it has
>existed since the creation of the git tree. It appears since commit
>5ee14e6d336f ("bonding: 3ad: apply ad_actor settings changes immediately")
>the if guard is no longer needed and removing the if guard would also fix
>the problem, I have not tested yet.
The logic in bond_3ad_initialize probably came that way when the
code was contributed sometime during 2.4.x.
Curiosity got the better of me, and I looked at the 2.4.35 code.
I'm not sure what the point of the test was even then, since
bond_3ad_initialize is only called when adding a first interface to the
bond, and there wasn't a way to tweak the sys_mac_addr then.
In the current code, I think the if test could fail if
sys_mac_addr is set manually to be equal to the bond's MAC prior to
adding the first interface to the bond. As far as I can tell, the only
result of failing the MAC test would be that the agg selection timer
wouldn't be started, which is an optimization to reduce LACP aggregator
flailing when multiple interfaces are added at the same time (IEEE
802.1AX-2000 6.4.9 and 6.4.12.1.n).
>I think this patch series can be accepted as-is because, it does fix the
>issue as demonstrated by the kselftest accompanying the series and is the
>smallest change to fix the issue.
>
>Further, I don't see why we want to set the file-scoped variable,
>ad_ticks_per_sec, inside bond_3ad_initialize() as ad_ticks_per_sec is
>utilized across all bonds. It seems like ad_ticks_per_sec should be
>changed to a const and set at the top of the file. I see no value in
>passing the value as an unnamed constant on the stack when
>bond_3ad_initialize is called. These changes however could be done in the
>net-next tree as follow-on cleanup patches.
>
>Jay, how would you like to proceed?
I don't have issue with moving ad_ticks_per_sec to a file scope
constant. The minimal change here, though, is effectively making the
tick_resolution parameter to bond_3ad_initialize be ignored, even though
the compiler won't complain about it.
Given that there is already mystery in how some of this works,
I'd prefer the patches to make the code clearer, so my vote is for the
"fix it right" method, i.e., make ad_ticks_per_sec a real constant,
remove the tick_resolution parameter from bond_3ad_initialize and drop
the "if MAC_ADDRESS_COMPARE" test therein.
-J
---
-Jay Vosburgh, [email protected]