netdev - Re: [PATCH net-next] bonding: use rtnl_deref in bond_change_rx

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] [day] [month] [year] [list]

Message-ID: <20140716093411.GB12030@mikrodark.usersys.redhat.com>
Date:	Wed, 16 Jul 2014 11:34:11 +0200
From:	Veaceslav Falico <vfalico@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev@...r.kernel.org, Jay Vosburgh <j.vosburgh@...il.com>,
	Andy Gospodarek <andy@...yhouse.net>
Subject: Re: [PATCH net-next] bonding: use rtnl_deref in
 bond_change_rx_flags()

On Wed, Jul 16, 2014 at 11:05:48AM +0200, Eric Dumazet wrote:
>On Wed, 2014-07-16 at 09:24 +0200, Veaceslav Falico wrote:
>> As it's always called with RTNL held, via dev_set_allmulti/promiscuity.
>>
>> CC: Jay Vosburgh <j.vosburgh@...il.com>
>> CC: Andy Gospodarek <andy@...yhouse.net>
>> Signed-off-by: Veaceslav Falico <vfalico@...il.com>
>> ---
>>  drivers/net/bonding/bond_main.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index d643807..2998710 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -498,7 +498,7 @@ static int bond_set_promiscuity(struct bonding *bond, int inc)
>>  	int err = 0;
>>
>>  	if (bond_uses_primary(bond)) {
>> -		struct slave *curr_active = bond_deref_active_protected(bond);
>> +		struct slave *curr_active = rtnl_dereference(bond->curr_active_slave);
>>
>>  		/* write lock already acquired */
>
>This seems dubious to me. What really protects curr_active_slave being
>modified ?

That's indeed all messed up, also I assume you've meant "c_a_s variable
changes its value" but not "the slave's internal parts are modified".

>
>Considering the presence of the previous comment, I assumed the sync
>point was the write lock. Not rtnl.

Good point, this comment should be removed. I'll send v2 if we'll agree
with this one removed.

>
>If write lock is never held without rtnl, then maybe the write lock is
>useless, I don't know.

The c_a_s is set under rtnl, i.e. we can't have another slave there while
we hold the rtnl.

However, quite a few functions (alb usually) uses the c_a_s write lock to
assure that the *current* slave isn't modified again (like - by re-entering
this function) or in parallel with some another function, or even changing
the current slave to which c_a_s points.

i.e. rtnl prevents changing the slave to which c_a_s points, while the
spinlock protects the interiors of the slave from changing, and the slave
itself.

So, the write lock isn't exactly useless currently, however it should go
away.

Yes, I know it's a mess. :(

>
>But after your patch its not really consistent and we increase
>confusion.
>
>>  		if (curr_active)
>> @@ -524,7 +524,7 @@ static int bond_set_allmulti(struct bonding *bond, int inc)
>>  	int err = 0;
>>
>>  	if (bond_uses_primary(bond)) {
>> -		struct slave *curr_active = bond_deref_active_protected(bond);
>> +		struct slave *curr_active = rtnl_dereference(bond->curr_active_slave);
>>
>>  		/* write lock already acquired */
>>  		if (curr_active)
>
>
--
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