| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3528.1627499144@famine>
Date: Wed, 28 Jul 2021 12:05:44 -0700
From: Jay Vosburgh <jay.vosburgh@...onical.com>
To: Nikolay Aleksandrov <nikolay@...dia.com>
cc: Yufeng Mo <moyufeng@...wei.com>, davem@...emloft.net,
kuba@...nel.org, jiri@...nulli.us, netdev@...r.kernel.org,
shenjian15@...wei.com, lipeng321@...wei.com,
yisen.zhuang@...wei.com, linyunsheng@...wei.com,
zhangjiaran@...wei.com, huangguangbin2@...wei.com,
chenhao288@...ilicon.com, salil.mehta@...wei.com,
linuxarm@...wei.com, linuxarm@...neuler.org
Subject: Re: [PATCH net-next] bonding: 3ad: fix the concurrency between __bond_release_one() and bond_3ad_state_machine_handler()
Nikolay Aleksandrov <nikolay@...dia.com> wrote:
>On 28/07/2021 09:19, Yufeng Mo wrote:
>> Some time ago, I reported a calltrace issue
>> "did not find a suitable aggregator", please see[1].
>> After a period of analysis and reproduction, I find
>> that this problem is caused by concurrency.
>>
>> Before the problem occurs, the bond structure is like follows:
>>
>> bond0 - slaver0(eth0) - agg0.lag_ports -> port0 - port1
>> \
>> port0
>> \
>> slaver1(eth1) - agg1.lag_ports -> NULL
>> \
>> port1
>>
>> If we run 'ifenslave bond0 -d eth1', the process is like below:
>>
>> excuting __bond_release_one()
>> |
>> bond_upper_dev_unlink()[step1]
>> | | |
>> | | bond_3ad_lacpdu_recv()
>> | | ->bond_3ad_rx_indication()
>> | | spin_lock_bh()
>> | | ->ad_rx_machine()
>> | | ->__record_pdu()[step2]
>> | | spin_unlock_bh()
>> | | |
>> | bond_3ad_state_machine_handler()
>> | spin_lock_bh()
>> | ->ad_port_selection_logic()
>> | ->try to find free aggregator[step3]
>> | ->try to find suitable aggregator[step4]
>> | ->did not find a suitable aggregator[step5]
>> | spin_unlock_bh()
>> | |
>> | |
>> bond_3ad_unbind_slave() |
>> spin_lock_bh()
>> spin_unlock_bh()
>>
>> step1: already removed slaver1(eth1) from list, but port1 remains
>> step2: receive a lacpdu and update port0
>> step3: port0 will be removed from agg0.lag_ports. The struct is
>> "agg0.lag_ports -> port1" now, and agg0 is not free. At the
>> same time, slaver1/agg1 has been removed from the list by step1.
>> So we can't find a free aggregator now.
>> step4: can't find suitable aggregator because of step2
>> step5: cause a calltrace since port->aggregator is NULL
>>
>> To solve this concurrency problem, the range of bond->mode_lock
>> is extended from only bond_3ad_unbind_slave() to both
>> bond_upper_dev_unlink() and bond_3ad_unbind_slave().
>>
>> [1]https://lore.kernel.org/netdev/10374.1611947473@famine/
>>
>> Signed-off-by: Yufeng Mo <moyufeng@...wei.com>
>> Acked-by: Jay Vosburgh <jay.vosburgh@...onical.com>
>> ---
>> drivers/net/bonding/bond_3ad.c | 7 +------
>> drivers/net/bonding/bond_main.c | 6 +++++-
>> 2 files changed, 6 insertions(+), 7 deletions(-)
>>
>[snip]
>> /**
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 0ff7567..deb019e 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2129,14 +2129,18 @@ static int __bond_release_one(struct net_device *bond_dev,
>> /* recompute stats just before removing the slave */
>> bond_get_stats(bond->dev, &bond->bond_stats);
>>
>> - bond_upper_dev_unlink(bond, slave);
>> /* unregister rx_handler early so bond_handle_frame wouldn't be called
>> * for this slave anymore.
>> */
>> netdev_rx_handler_unregister(slave_dev);
>>
>> + /* Sync against bond_3ad_state_machine_handler() */
>> + spin_lock_bh(&bond->mode_lock);
>> + bond_upper_dev_unlink(bond, slave);
>
>this calls netdev_upper_dev_unlink() which calls call_netdevice_notifiers_info() for
>NETDEV_PRECHANGEUPPER and NETDEV_CHANGEUPPER, both of which are allowed to sleep so you
>cannot hold the mode lock
Indeed it does, I missed that the callbacks can sleep.
>after netdev_rx_handler_unregister() the bond's recv_probe cannot be executed
>so you don't really need to unlink it under mode_lock or move mode_lock at all
I don't think moving the call to netdev_rx_handler_unregister is
sufficient to close the race. If it's moved above the call to
bond_upper_dev_unlink, the probe won't be called afterwards, but the
LACPDU could have arrived just prior to the unregister and changed the
port state in the bond_3ad_lacpdu_recv call sequence ("step 2",
something in the LACPDU causes AD_PORT_SELECTED to be cleared). Later,
bond_3ad_state_machine_handler runs in a separate work queue context,
and could process the effect of the LACPDU after the rx_handler
unregister, and still race with the upper_dev_unlink.
I suspect the solution is to rework ad_port_selection_logic to
correctly handle the situation where no aggregator is available. Off
the top of my head, I think something along the lines of:
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 6908822d9773..eb6223e4510e 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1537,6 +1537,10 @@ static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
slave_err(bond->dev, port->slave->dev,
"Port %d did not find a suitable aggregator\n",
port->actor_port_number);
+ aggregator = __get_first_agg(port);
+ ad_agg_selection_logic(aggregator, update_slave_arr);
+
+ return;
}
}
/* if all aggregator's ports are READY_N == TRUE, set ready=TRUE
I've not compiled or tested this, but the theory is that it will
reselect a new aggregator for the bond (which happens anyway later in
the function), then returns, leaving "port" as not AD_PORT_SELECTED.
The next run of the state machine should attempt to select it again, and
presumably succeed at that time.
This may leave the bond with no active ports for one interval
between runs of the state machine, unfortunately, but it should
eliminate the panic.
Another possibility might be netdev_rx_handler_unregister, then
bond_3ad_unbind_slave, and finally bond_upper_dev_unlink, but I'm not
sure right off if that would have other side effects.
Yufeng, would you be able to test the above and see if it
resolves the issue in your test?
-J
>> if (BOND_MODE(bond) == BOND_MODE_8023AD)
>> bond_3ad_unbind_slave(slave);
>> + spin_unlock_bh(&bond->mode_lock);
>>
>> if (bond_mode_can_use_xmit_hash(bond))
>> bond_update_slave_arr(bond, slave);
>>
>
---
-Jay Vosburgh, jay.vosburgh@...onical.com
Powered by blists - more mailing lists