netdev - Re: [PATCH net-next 3/3] bonding: fix bond_release

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]

Date:	Mon, 18 Feb 2013 15:17:59 -0800
From:	Jay Vosburgh <fubar@...ibm.com>
To:	Nikolay Aleksandrov <nikolay@...hat.com>
cc:	netdev@...r.kernel.org, davem@...emloft.net, andy@...yhouse.net
Subject: Re: [PATCH net-next 3/3] bonding: fix bond_release_all inconsistencies

Nikolay Aleksandrov <nikolay@...hat.com> wrote:

>On 18/02/13 22:56, Jay Vosburgh wrote:
>> Nikolay Aleksandrov <nikolay@...hat.com> wrote:
>> 
>>> This patch fixes the following inconsistencies in bond_release_all:
>>> - IFF_BONDING flag is not stripped from slaves
>>> - MTU is not restored
>>> - no netdev notifiers are sent
>>> Instead of trying to keep bond_release and bond_release_all in sync
>>> I think we can re-use bond_release as the environment for calling it
>>> is correct (RTNL is held). I have been running tests for the past
>>> week and they came out successful. The only way for bond_release to fail
>>> is for the slave to be attached in a different bond or to not be a slave
>>> but that cannot happen as RTNL is held and no slave manipulations can be
>>> achieved.
>> 
>> 	It might be worthwhile to add an "all" argument to bond_release
>> that skips some things that don't make sense if all slaves are being
>> released.  I'm thinking in particular of this block:
>> 
>> 	if (oldcurrent == slave) {
>> 		/*
>> 		 * Note that we hold RTNL over this sequence, so there
>> 		 * is no concern that another slave add/remove event
>> 		 * will interfere.
>> 		 */
>> 		write_unlock_bh(&bond->lock);
>> 		read_lock(&bond->lock);
>> 		write_lock_bh(&bond->curr_slave_lock);
>> 
>> 		bond_select_active_slave(bond);
>> 
>> 		write_unlock_bh(&bond->curr_slave_lock);
>> 		read_unlock(&bond->lock);
>> 		write_lock_bh(&bond->lock);
>> 	}
>> 
>> 	as it's written now, for the release all case, the code may go
>> to the trouble of assigning a new active slave each time one slave is
>> removed (including various log messages, maybe sending IGMPs, etc).  If
>> all slaves are being removed, that's pointless.  This could be something
>> like:
>> 
>> 	if (release_all) {
>> 		bond->curr_active_slave = NULL;
>> 	} else if (oldcurrent == slave) {
>> 		[ the current block of stuff ]
>> 	}
>> 
>> 	it's safe here to unconditionally set curr_active_slave to NULL
>> because we hold bond->lock for write.  The lock dance stuff for the
>> bond_select_active_slave() call is to satisfy its locking requirements.
>> 
>> 	-J
>I see your point and I agree. I will prepare another version that
>incorporates it, although I can't add it as an argument since
>bond_release is used as ndo_del_slave. I'll have to make it a global
>variable.

	No, just rename the current bond_release to __bond_release_one,
add the extra argument, and create a new bond_release .ndo_del_slave
that calls __bond_release_one with "all=0".  Then, bond_release_all
calls __bond_release_one with all=1.

	Also, there's only one caller of bond_release_all, and since the
new & improved bond_release_all is trivial, it could be open coded into
bond_uninit, eliminating bond_release_all as a function.

	-J

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