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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20130720150041.GE9149@redhat.com>
Date:	Sat, 20 Jul 2013 17:00:41 +0200
From:	Veaceslav Falico <vfalico@...hat.com>
To:	Nikolay Aleksandrov <nikolay@...hat.com>
Cc:	dingtianhong <dingtianhong@...wei.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 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
>
--
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