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

Powered by blists - more mailing lists