netdev - Re: [3/4] bonding: the calling of bond->slave

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 for Android: free password hash cracker in your pocket

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

Date:	Mon, 22 Jul 2013 08:47:49 +0800
From:	Ding Tianhong <dingtianhong@...wei.com>
To:	Veaceslav Falico <vfalico@...hat.com>
CC:	Nikolay Aleksandrov <nikolay@...hat.com>,
	Jay Vosburgh <fubar@...ibm.com>,
	Andy Gospodarek <andy@...yhouse.net>,
	"David S. Miller" <davem@...emloft.net>,
	Netdev <netdev@...r.kernel.org>
Subject: Re: [3/4] bonding: the calling of bond->slave_cnt need protection

On 2013/7/20 23:00, Veaceslav Falico wrote:
> On Sat, Jul 20, 2013 at 02:42:37PM +0200, Nikolay Aleksandrov wrote:
>> On 07/20/2013 12:47 PM, Veaceslav Falico wrote:
>>> On Sat, Jul 20, 2013 at 03:23:57PM +0800, dingtianhong wrote:
>>>> The bonding_store_mode has rtnl protection, so no need to get read lock
>>>> for bond->slave_cnt, but the bonding_store_fail_over_mac need to protect
>>>> the bond->slave_cnt, so add read_lock().
>>>>
>>>> Signed-off-by: Ding Tianhong <dingtianhong@...wei.com>
>>>>
>> <snip>
>>>
>>> Maybe it's Saturday, but I really don't see *any* point in this locking.
>>>
>>> I think you've meant that we need the rtnl protection while reading
>>> slave_cnt AND updating the .fail_over_mac, so that in between we won't add
>>> new slaves with outdated params.
>>>
>>> Something like this (untested):
>>>
>> Indeed, Veaceslav's way is the correct one (I've looked at this race
>> before), but IMO it's not worth it to protect fail_over_mac as the worst
>> that could happen is inconsistency with the MAC addresses which isn't
>> fatal. Anyway, I still haven't had my coffee and might be missing something :-)
> 
> Yep, agree that it's kind of minor and hard to hit in real life.
> 
> OTOH, getting the rtnl here costs us virtually nothing and might save
> someone from a headache :). And it also follows the logic "don't change
> anything slave-related without rtnl".
> 
> So I'd rather have it, as a minor improvement :).
> 
>>
>> Cheers,
>> Nik

yes , i think it is hard to hit the problem in real lift, just looks better. :)

>>
> 
> 


--
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