| Message ID | 1375732053-829-1-git-send-email-vfalico@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Veaceslav Falico <vfalico@redhat.com> Date: Mon, 5 Aug 2013 21:47:33 +0200 > @@ -2999,7 +3002,10 @@ void bond_activebackup_arp_mon(struct work_struct *work) > if (!rtnl_trylock()) { > read_lock(&bond->lock); > delta_in_ticks = 1; > - should_notify_peers = false; > + if (should_notify_peers) { > + bond->send_peer_notif++; I doubt this increment to a shared datastructure is safe with the locks you hold here. You don't hold RTNL and you only have bond->lock as a reader. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 05, 2013 at 04:12:12PM -0700, David Miller wrote: >From: Veaceslav Falico <vfalico@redhat.com> >Date: Mon, 5 Aug 2013 21:47:33 +0200 > >> @@ -2999,7 +3002,10 @@ void bond_activebackup_arp_mon(struct work_struct *work) >> if (!rtnl_trylock()) { >> read_lock(&bond->lock); >> delta_in_ticks = 1; >> - should_notify_peers = false; >> + if (should_notify_peers) { >> + bond->send_peer_notif++; > >I doubt this increment to a shared datastructure is safe with >the locks you hold here. > >You don't hold RTNL and you only have bond->lock as a reader. You're right, we can race here. This whole locking juggling should be changed. Self-NAK for this, I'll rework it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 5697043..fd92738 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2383,7 +2383,10 @@ void bond_mii_monitor(struct work_struct *work) if (!rtnl_trylock()) { read_lock(&bond->lock); delay = 1; - should_notify_peers = false; + if (should_notify_peers) { + bond->send_peer_notif++; + should_notify_peers = false; + } goto re_arm; } @@ -2999,7 +3002,10 @@ void bond_activebackup_arp_mon(struct work_struct *work) if (!rtnl_trylock()) { read_lock(&bond->lock); delta_in_ticks = 1; - should_notify_peers = false; + if (should_notify_peers) { + bond->send_peer_notif++; + should_notify_peers = false; + } goto re_arm; }
In bond_mii_monitor()/bond_activebackup_arp_mon(), we verify if we have any notifications to be sent before trying to get the rtnl_trylock(), and if we have - we set should_notify_peers and withdraw a notification. However, if we fail the get the rtnl_trylock(), we don't send out any notification and don't put the notification back to bond->send_peer_notif. Fix it by putting back the notification to send_peer_notif in case of rtnl_trylock() failure. CC: Jay Vosburgh <fubar@us.ibm.com> CC: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Veaceslav Falico <vfalico@redhat.com> --- drivers/net/bonding/bond_main.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)