The bond works in mode 4, and performs down/up operations on the bond
that is normally negotiated. The probability of bond-> slave_arr is NULL
Test commands:
ifconfig bond1 down
ifconfig bond1 up
The conflict occurs in the following process：
__dev_open (CPU A)
--bond_open
--queue_delayed_work(bond->wq,&bond->ad_work,0);
--bond_update_slave_arr
--bond_3ad_get_active_agg_info
ad_work(CPU B)
--bond_3ad_state_machine_handler
--ad_agg_selection_logic
ad_work runs on cpu B. In the function ad_agg_selection_logic, all
agg->is_active will be cleared. Before the new active aggregator is
selected on CPU B, bond_3ad_get_active_agg_info failed on CPU A,
bond->slave_arr will be set to NULL. The best aggregator in
ad_agg_selection_logic has not changed, no need to update slave arr.
The conflict occurred in that ad_agg_selection_logic clears
agg->is_active under mode_lock, but bond_open -> bond_update_slave_arr
is inspecting agg->is_active outside the lock.
Also, bond_update_slave_arr is normal for potential sleep when
allocating memory, so replace the WARN_ON with a call to might_sleep.
Signed-off-by: jinyiting <[email protected]>
---
Previous versions:
* https://lore.kernel.org/netdev/[email protected]/
drivers/net/bonding/bond_main.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 74cbbb2..83ef62d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4391,9 +4391,7 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
int agg_id = 0;
int ret = 0;
-#ifdef CONFIG_LOCKDEP
- WARN_ON(lockdep_is_held(&bond->mode_lock));
-#endif
+ might_sleep();
usable_slaves = kzalloc(struct_size(usable_slaves, arr,
bond->slave_cnt), GFP_KERNEL);
@@ -4406,7 +4404,9 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
if (BOND_MODE(bond) == BOND_MODE_8023AD) {
struct ad_info ad_info;
+ spin_lock_bh(&bond->mode_lock);
if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
+ spin_unlock_bh(&bond->mode_lock);
pr_debug("bond_3ad_get_active_agg_info failed\n");
/* No active aggragator means it's not safe to use
* the previous array.
@@ -4414,6 +4414,7 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
bond_reset_slave_arr(bond);
goto out;
}
+ spin_unlock_bh(&bond->mode_lock);
agg_id = ad_info.aggregator_id;
}
bond_for_each_slave(bond, slave, iter) {
--
1.7.12.4
From: jinyiting <[email protected]>
Date: Wed, 21 Apr 2021 16:38:21 +0800
> The bond works in mode 4, and performs down/up operations on the bond
> that is normally negotiated. The probability of bond-> slave_arr is NULL
>
> Test commands:
> ifconfig bond1 down
> ifconfig bond1 up
>
> The conflict occurs in the following process$B!'(B
>
> __dev_open (CPU A)
> --bond_open
> --queue_delayed_work(bond->wq,&bond->ad_work,0);
> --bond_update_slave_arr
> --bond_3ad_get_active_agg_info
>
> ad_work(CPU B)
> --bond_3ad_state_machine_handler
> --ad_agg_selection_logic
>
> ad_work runs on cpu B. In the function ad_agg_selection_logic, all
> agg->is_active will be cleared. Before the new active aggregator is
> selected on CPU B, bond_3ad_get_active_agg_info failed on CPU A,
> bond->slave_arr will be set to NULL. The best aggregator in
> ad_agg_selection_logic has not changed, no need to update slave arr.
>
> The conflict occurred in that ad_agg_selection_logic clears
> agg->is_active under mode_lock, but bond_open -> bond_update_slave_arr
> is inspecting agg->is_active outside the lock.
>
> Also, bond_update_slave_arr is normal for potential sleep when
> allocating memory, so replace the WARN_ON with a call to might_sleep.
>
> Signed-off-by: jinyiting <[email protected]>
> ---
>
> Previous versions:
> * https://lore.kernel.org/netdev/[email protected]/
>
> drivers/net/bonding/bond_main.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 74cbbb2..83ef62d 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4406,7 +4404,9 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> if (BOND_MODE(bond) == BOND_MODE_8023AD) {
> struct ad_info ad_info;
>
> + spin_lock_bh(&bond->mode_lock);
The code paths that call this function with mode_lock held will now deadlock.
David Miller <[email protected]> wrote:
>From: jinyiting <[email protected]>
>Date: Wed, 21 Apr 2021 16:38:21 +0800
>
>> The bond works in mode 4, and performs down/up operations on the bond
>> that is normally negotiated. The probability of bond-> slave_arr is NULL
>>
>> Test commands:
>> ifconfig bond1 down
>> ifconfig bond1 up
>>
>> The conflict occurs in the following process：
>>
>> __dev_open (CPU A)
>> --bond_open
>> --queue_delayed_work(bond->wq,&bond->ad_work,0);
>> --bond_update_slave_arr
>> --bond_3ad_get_active_agg_info
>>
>> ad_work(CPU B)
>> --bond_3ad_state_machine_handler
>> --ad_agg_selection_logic
>>
>> ad_work runs on cpu B. In the function ad_agg_selection_logic, all
>> agg->is_active will be cleared. Before the new active aggregator is
>> selected on CPU B, bond_3ad_get_active_agg_info failed on CPU A,
>> bond->slave_arr will be set to NULL. The best aggregator in
>> ad_agg_selection_logic has not changed, no need to update slave arr.
>>
>> The conflict occurred in that ad_agg_selection_logic clears
>> agg->is_active under mode_lock, but bond_open -> bond_update_slave_arr
>> is inspecting agg->is_active outside the lock.
>>
>> Also, bond_update_slave_arr is normal for potential sleep when
>> allocating memory, so replace the WARN_ON with a call to might_sleep.
>>
>> Signed-off-by: jinyiting <[email protected]>
>> ---
>>
>> Previous versions:
>> * https://lore.kernel.org/netdev/[email protected]/
>>
>> drivers/net/bonding/bond_main.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 74cbbb2..83ef62d 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -4406,7 +4404,9 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>> if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>> struct ad_info ad_info;
>>
>> + spin_lock_bh(&bond->mode_lock);
>
>The code paths that call this function with mode_lock held will now deadlock.
No path should be calling bond_update_slave_arr with mode_lock
already held (it expects RTNL only); did you find one?
My concern is that there's something else that does the opposite
order, i.e., mode_lock first, then RTNL, but I haven't found an example.
-J
---
-Jay Vosburgh, [email protected]
David Miller <[email protected]> wrote:
>From: Jay Vosburgh <[email protected]>
>Date: Mon, 26 Apr 2021 08:22:37 -0700
>
>> David Miller <[email protected]> wrote:
>>
>>>From: jinyiting <[email protected]>
>>>Date: Wed, 21 Apr 2021 16:38:21 +0800
>>>
>>>> The bond works in mode 4, and performs down/up operations on the bond
>>>> that is normally negotiated. The probability of bond-> slave_arr is NULL
>>>>
>>>> Test commands:
>>>> ifconfig bond1 down
>>>> ifconfig bond1 up
>>>>
>>>> The conflict occurs in the following process：
>>>>
>>>> __dev_open (CPU A)
>>>> --bond_open
>>>> --queue_delayed_work(bond->wq,&bond->ad_work,0);
>>>> --bond_update_slave_arr
>>>> --bond_3ad_get_active_agg_info
>>>>
>>>> ad_work(CPU B)
>>>> --bond_3ad_state_machine_handler
>>>> --ad_agg_selection_logic
>>>>
>>>> ad_work runs on cpu B. In the function ad_agg_selection_logic, all
>>>> agg->is_active will be cleared. Before the new active aggregator is
>>>> selected on CPU B, bond_3ad_get_active_agg_info failed on CPU A,
>>>> bond->slave_arr will be set to NULL. The best aggregator in
>>>> ad_agg_selection_logic has not changed, no need to update slave arr.
>>>>
>>>> The conflict occurred in that ad_agg_selection_logic clears
>>>> agg->is_active under mode_lock, but bond_open -> bond_update_slave_arr
>>>> is inspecting agg->is_active outside the lock.
>>>>
>>>> Also, bond_update_slave_arr is normal for potential sleep when
>>>> allocating memory, so replace the WARN_ON with a call to might_sleep.
>>>>
>>>> Signed-off-by: jinyiting <[email protected]>
>>>> ---
>>>>
>>>> Previous versions:
>>>> * https://lore.kernel.org/netdev/[email protected]/
>>>>
>>>> drivers/net/bonding/bond_main.c | 7 ++++---
>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>>> index 74cbbb2..83ef62d 100644
>>>> --- a/drivers/net/bonding/bond_main.c
>>>> +++ b/drivers/net/bonding/bond_main.c
>>>> @@ -4406,7 +4404,9 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>>>> if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>>>> struct ad_info ad_info;
>>>>
>>>> + spin_lock_bh(&bond->mode_lock);
>>>
>>>The code paths that call this function with mode_lock held will now deadlock.
>>
>> No path should be calling bond_update_slave_arr with mode_lock
>> already held (it expects RTNL only); did you find one?
>>
>> My concern is that there's something else that does the opposite
>> order, i.e., mode_lock first, then RTNL, but I haven't found an example.
>>
>
>This patch is removing a lockdep assertion masking sure that mode_lock was held
>when this function was called. That should have been triggering all the time, right?
The line in question is:
#ifdef CONFIG_LOCKDEP
WARN_ON(lockdep_is_held(&bond->mode_lock));
#endif
The WARN_ON is triggering if mode_lock is held, not asserting
that mode_lock is held. I think that's wrong anyway, since mode_lock
could be held by some other thread, leading to false positives, thus the
change to might_sleep.
-J
---
-Jay Vosburgh, [email protected]
From: Jay Vosburgh <[email protected]>
Date: Mon, 26 Apr 2021 08:22:37 -0700
> David Miller <[email protected]> wrote:
>
>>From: jinyiting <[email protected]>
>>Date: Wed, 21 Apr 2021 16:38:21 +0800
>>
>>> The bond works in mode 4, and performs down/up operations on the bond
>>> that is normally negotiated. The probability of bond-> slave_arr is NULL
>>>
>>> Test commands:
>>> ifconfig bond1 down
>>> ifconfig bond1 up
>>>
>>> The conflict occurs in the following process$B!'(B
>>>
>>> __dev_open (CPU A)
>>> --bond_open
>>> --queue_delayed_work(bond->wq,&bond->ad_work,0);
>>> --bond_update_slave_arr
>>> --bond_3ad_get_active_agg_info
>>>
>>> ad_work(CPU B)
>>> --bond_3ad_state_machine_handler
>>> --ad_agg_selection_logic
>>>
>>> ad_work runs on cpu B. In the function ad_agg_selection_logic, all
>>> agg->is_active will be cleared. Before the new active aggregator is
>>> selected on CPU B, bond_3ad_get_active_agg_info failed on CPU A,
>>> bond->slave_arr will be set to NULL. The best aggregator in
>>> ad_agg_selection_logic has not changed, no need to update slave arr.
>>>
>>> The conflict occurred in that ad_agg_selection_logic clears
>>> agg->is_active under mode_lock, but bond_open -> bond_update_slave_arr
>>> is inspecting agg->is_active outside the lock.
>>>
>>> Also, bond_update_slave_arr is normal for potential sleep when
>>> allocating memory, so replace the WARN_ON with a call to might_sleep.
>>>
>>> Signed-off-by: jinyiting <[email protected]>
>>> ---
>>>
>>> Previous versions:
>>> * https://lore.kernel.org/netdev/[email protected]/
>>>
>>> drivers/net/bonding/bond_main.c | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 74cbbb2..83ef62d 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -4406,7 +4404,9 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>>> if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>>> struct ad_info ad_info;
>>>
>>> + spin_lock_bh(&bond->mode_lock);
>>
>>The code paths that call this function with mode_lock held will now deadlock.
>
> No path should be calling bond_update_slave_arr with mode_lock
> already held (it expects RTNL only); did you find one?
>
> My concern is that there's something else that does the opposite
> order, i.e., mode_lock first, then RTNL, but I haven't found an example.
>
This patch is removing a lockdep assertion masking sure that mode_lock was held
when this function was called. That should have been triggering all the time, right?
Thanks.