netdev - Re: [PATCH net-next v5 2/6] bonding: remove the no effect lock for bond_3ad_lacpdu

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <11033.1380169422@death.nxdomain>
Date:	Wed, 25 Sep 2013 21:23:42 -0700
From:	Jay Vosburgh <fubar@...ibm.com>
To:	Ding Tianhong <dingtianhong@...wei.com>
cc:	Veaceslav Falico <vfalico@...hat.com>,
	Andy Gospodarek <andy@...yhouse.net>,
	"David S. Miller" <davem@...emloft.net>,
	Nikolay Aleksandrov <nikolay@...hat.com>,
	Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v5 2/6] bonding: remove the no effect lock for bond_3ad_lacpdu_recv()

Ding Tianhong <dingtianhong@...wei.com> wrote:

>On 2013/9/25 18:33, Veaceslav Falico wrote:
>> On Wed, Sep 25, 2013 at 05:52:19PM +0800, Ding Tianhong wrote:
>>> There is no pointer needed read lock protection, remove the unnecessary lock
>>> and improve performance for the 3ad recv path.

	How much does removing the lock around the LACPDU receive
processing improve performance?  This is not high rate traffic; the
"fast" rate is one LACPDU per second (per port); the default rate is one
every 30 seconds.

>> I don't really understand it. Here's the code path:
>> 
>> rx_handler (holding rcu_read_lock()) -> bond_handle_frame() ->
>> bond->recv_probe -> bond_3ad_lacpdu_recv(). So we're holding only the
>> rcu_read_lock() there. What stops us from racing with
>> bond_3ad_unbind_slave(), for example?
>> 
>> As in:
>> 
>> CPU0                CPU1
>> --------            -----------
>> ...                bond_3ad_unbind_slave()
>> bond_3ad_rx_indication()    ...
>> if (!port->slave) {        ...            //slave is ok
>>                 port->slave = NULL;
>> ad_marker_info_received()    ...
>> ad_marker_send()        ...
>> slave = port->slave;        ...
>> skb->dev = slave->dev;        ...
>>        ^^^ NULL pointer dereference.
>> 
>> 
>> I'm not saying that this approach is wrong, maybe I'm missing something,
>> but when removing locks it's usually a good thing to do - to comment it in
>> depth in the commit message why it's not already needed.
>> 
>
>no, it will not happend, pls review the cold:
>	netdev_rx_handler_unregister(slave_dev);
>	write_lock_bh(&bond->lock);
>
>	/* Inform AD package of unbinding of slave. */
>	if (bond->params.mode == BOND_MODE_8023AD) {
>		/* must be called before the slave is
>		 * detached from the list
>		 */
>		bond_3ad_unbind_slave(slave);
>	}
>netdev_rx_handler_unregiste() will remvoe the rx_handle before the bond_3ad_unbind_slave(),
>it was safe to run bond_3ad_rx_indication(). 

	I'm not sure this is safe if bond_3ad_rx_indication is started
prior to the unbind, e.g.,

CPU 0				CPU 1
----				-----
bond_3ad_rx_indication
[ pass port->slave test ]
[ ... ]				rx_handler_unregister

[ state machine lock could be
  contended, forcing us to wait ]
__get_state_machine_lock

				write_lock(&bond->lock)
				bond_3ad_unbind_slave()
				[ ... ]
				port->slave = NULL;

[ got the lock ]
ad_rx_machine(lacpdu, port)
[ detect loopback ]
pr_err(... port->slave->bond->dev->name)

	or that ad_marker case that Veaceslav describes.

	-J

>Best regards
>Ding Tianhong
>
>
>>>
>>> Signed-off-by: Ding Tianhong <dingtianhong@...wei.com>
>>> Cc: Nikolay Aleksandrov <nikolay@...hat.com>
>>> ---
>>> drivers/net/bonding/bond_3ad.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>>> index 7a3860f..c134f43 100644
>>> --- a/drivers/net/bonding/bond_3ad.c
>>> +++ b/drivers/net/bonding/bond_3ad.c
>>> @@ -2494,9 +2494,7 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
>>>     if (!lacpdu)
>>>         return ret;
>>>
>>> -    read_lock(&bond->lock);
>>>     ret = bond_3ad_rx_indication(lacpdu, slave, skb->len);
>>> -    read_unlock(&bond->lock);
>>>     return ret;
>>> }
>>>
>>> -- 
>>> 1.8.2.1

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@...ibm.com

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html