netdev - Re: [PATCH net-next 3/3] bonding: Fix the RTNL assertion failed for 802.3ad state machine

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: <5300.1392689172@death.nxdomain>
Date:	Mon, 17 Feb 2014 18:06:12 -0800
From:	Jay Vosburgh <fubar@...ibm.com>
To:	Ding Tianhong <dingtianhong@...wei.com>
cc:	vfalico@...hat.com, andy@...yhouse.net, cwang@...pensource.com,
	jiri@...nulli.us, thomas@...nzmann.de, eric.dumazet@...il.com,
	sfeldma@...ulusnetworks.com, davem@...emloft.net,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next 3/3] bonding: Fix the RTNL assertion failed for 802.3ad state machine

Ding Tianhong <dingtianhong@...wei.com> wrote:

>The 802.3ad state machine don't run in RTNL, but when the slave's
>state changed, the rtmsg_ifinfo will be called, it will cause
>warning message because the RTML is not locked, acquiring RTNL
>for the __enable_port and __disable_port cases is difficult, as
>those calls generally already hold the state machine lock, and
>can't unconditionally call rtnl_lock because either they already
>hold RTNL (for calls via bond_3ad_unbind_slave) or due to the
>potential for deadlock with bond_3ad_adapter_speed_changed,
>bond_3ad_adapter_duplex_changed, bond_3ad_link_change, or
>bond_3ad_update_lacp_rate. All four of those are called with RTNL
>held, and acquire the state machine lock second, The calling contexts for
>__enable_port and __disable_port already hold the state machine lock,
>and may or may not need RTNL.
>
>So according to the Jay's opinion, the __enable_port and __disable_port
>should not call rtmsg_ifinfo in the state machine lock, any change in
>the state of slave could set a flag in the slave, it will indicated that
>an rtmsg_ifinfo should be called at the end of the state machine.

	To clarify, my opinion being referenced here was really asking
Scott Feldman <sfeldma@...ulusnetworks.com> if: (a) the calls had to be
synchronous, and, (b) if the intermediate calls to adjust flags within
the ARP monitor "cycle through slaves looking for a functional slave"
all required notifications.  My suspicion is that the answer to both of
those is "no," but I haven't heard from Scott.

>Cc: Jay Vosburgh <fubar@...ibm.com>
>Cc: Veaceslav Falico <vfalico@...hat.com>
>Cc: Andy Gospodarek <andy@...yhouse.net>
>Signed-off-by: Ding Tianhong <dingtianhong@...wei.com>
>---
> drivers/net/bonding/bond_3ad.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index cce1f1b..e80b78f 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -181,7 +181,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
>  */
> static inline void __disable_port(struct port *port)
> {
>-	bond_set_slave_inactive_flags(port->slave);
>+	bond_set_slave_flags(port->slave, BOND_STATE_BACKUP, false);
> }
>
> /**
>@@ -193,7 +193,7 @@ static inline void __enable_port(struct port *port)
> 	struct slave *slave = port->slave;
>
> 	if ((slave->link == BOND_LINK_UP) && IS_UP(slave->dev))
>-		bond_set_slave_active_flags(slave);
>+		bond_set_slave_flags(slave, BOND_STATE_ACTIVE, false);

	I don't agree that we need to have two separate systems (your
new bond_set_slave_flags plus bond_set_slave_{active,inactive}_flags)
that both tweak the "active" or "inactive" flags for a slave.  It would
be much cleaner and consistent with the current code to add a "notify"
boolean to the existing functions.

	-J

> }
>
> /**
>@@ -2123,6 +2123,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
> re_arm:
> 	rcu_read_unlock();
> 	read_unlock(&bond->lock);
>+	bond_slave_state_notify(bond, false);
> 	queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
> }
>
>-- 
>1.8.0

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