From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov Subject: Re: [PATCH net] bonding: fix div by zero while enslaving and transmitting Date: Fri, 12 Sep 2014 15:27:49 +0200 Message-ID: <5412F4D5.3070101@redhat.com> References: <1410524556-27128-1-git-send-email-nikolay@redhat.com> <1410527381.7106.81.camel@edumazet-glaptop2.roam.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, Jiri Pirko , Andy Gospodarek , Jay Vosburgh , Veaceslav Falico To: Eric Dumazet Return-path: Received: from mx1.redhat.com ([209.132.183.28]:29531 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754664AbaILN2J (ORCPT ); Fri, 12 Sep 2014 09:28:09 -0400 In-Reply-To: <1410527381.7106.81.camel@edumazet-glaptop2.roam.corp.google.com> Sender: netdev-owner@vger.kernel.org List-ID: On 09/12/2014 03:09 PM, Eric Dumazet wrote: > On Fri, 2014-09-12 at 14:22 +0200, Nikolay Aleksandrov wrote: > ... >> >> CC: Jiri Pirko >> CC: Andy Gospodarek >> CC: Jay Vosburgh >> CC: Veaceslav Falico >> Fixes: 5378c2e6ea236d ("bonding: move bond-specific init after enslave happens") >> Signed-off-by: Nikolay Aleksandrov >> --- >> drivers/net/bonding/bond_main.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 57912ee231cb..10ad434ea184 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -1552,6 +1552,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) >> goto err_detach; >> } >> >> + /* Increment slave_cnt before linking in the slave so we won't end up in >> + * bond_start_xmit with bond_has_slaves() true and slave_cnt == 0. >> + */ >> + bond->slave_cnt++; > > It looks like explicit barriers are missing. > > #define bond_has_slaves(bond) !list_empty(bond_slave_list(bond)) > > So your increment into slave_cnt must be committed into memory before > any change to slave_list. But you need to check how removal of a slave > is handled. > That is handled by decrementing slave_cnt after executing synchronize_rcu() after unlinking the last slave thus making the list empty and all xmitters entering will see bond_has_slaves() as empty before they see slave_cnt as 0. In every other case the worst that could happen is that a few packets will see wrong slave_cnt, but that is not a problem since we walk the list to find the slave with the id. > Now I wonder why bond_has_slaves(bond) is not a test against > bond->slave_cnt > It used to be once, I don't remember the reason it's not anymore. > Note that even if this would be the case, bond xmit seems racy : > > if (bond_has_slaves(bond)) > ret = __bond_start_xmit(skb, dev); > Yes, true but we make sure it doesn't see slave_cnt as 0 with bond_has_slaves() evaluating to true. > As slave_cnt could change (and eventually reach 0) between the two > places. This shouldn't be possible because of the synchronize_rcu() after unlinking the slave. slave_cnt is decremented only after that so every reader will see the list empty before they see slave_cnt as 0. > > My feeling is that RCU conversion is not properly done in this driver. > > Either bond->slave_cnt should be read _once_ for the whole duration of > bond_start_xmit() call, _OR_, be stored in a real Read Copy structure, > so that struct->slave_cnt _cannot_ change during bond_start_xmit() > >> res = bond_master_upper_dev_link(bond_dev, slave_dev, new_slave); >> if (res) { >> netdev_dbg(bond_dev, "Error %d calling bond_master_upper_dev_link\n", res); >> @@ -1564,7 +1568,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) >> goto err_upper_unlink; >> } >> >> - bond->slave_cnt++; >> bond_compute_features(bond); >> bond_set_carrier(bond); >> >> @@ -1590,6 +1593,7 @@ err_upper_unlink: >> >> err_unregister: >> netdev_rx_handler_unregister(slave_dev); >> + bond->slave_cnt--; >> >> err_detach: >> if (!bond_uses_primary(bond)) > >