From mboxrd@z Thu Jan 1 00:00:00 1970 From: Veaceslav Falico Subject: Re: [PATCH net-next] bonding: RCUify bond_set_rx_mode() Date: Mon, 5 Aug 2013 14:31:03 +0200 Message-ID: <20130805123103.GH22756@redhat.com> References: <1375694776-3429-1-git-send-email-vfalico@redhat.com> <51FF7CC4.7010806@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Cc: netdev@vger.kernel.org, Jay Vosburgh , Andy Gospodarek To: Nikolay Aleksandrov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:2564 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751456Ab3HEMb5 (ORCPT ); Mon, 5 Aug 2013 08:31:57 -0400 Content-Disposition: inline In-Reply-To: <51FF7CC4.7010806@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, Aug 05, 2013 at 12:21:56PM +0200, Nikolay Aleksandrov wrote: >On 08/05/2013 11:26 AM, Veaceslav Falico wrote: >> Currently, we might easily deadlock with bond_set_rx_mode() and >> bond_hw_addr_swap(). bond_set_rx_mode() is called via dev_set_rx_mode(), >> which already holds the netif_addr_lock_bh(bond), and inside it takes the >> bond->curr_active_slave lock, while bond_hw_addr_swap() is called with >> bond->curr_active_slave lock held and then takes netif_addr_lock_bh(bond), >> which results in deadlock. >> >> CPU0 CPU1 >> ---- ---- >> lock(&bonding_netdev_addr_lock_key); >> lock(&bond->curr_slave_lock); >> lock(&bonding_netdev_addr_lock_key); >> lock(&bond->curr_slave_lock); >> >> Fix this by using the RCU primites in bond_set_rx_mode(). We're safe wrt >> racing of dev_?c_(un)sync() because we hold >> lock(&bonding_netdev_addr_lock_key), and thus nobody will be able to modify >> these lists before we finish. >> >Hi, >I don't think this deadlock can actually happen because bond_hw_addr_swap() is >called from bond_change_active_slave() only in USES_PRIMARY mode, and in such >mode it's always called with rtnl acquired before that, and since >dev_set_rx_mode is called with rtnl, IMO such deadlock can't happen. Yep, indeed, missed the part with USES_PRIMARY(). So the lockdep had a false alarm. >Also I think bond_set_rx_mode() can work without RCU because of the held rtnl >and converted to ASSERT_RTNL (this is optional) + rtnl_dereference for the >curr_active_slave. Yes, we don't need the real rcu cause we're under rtnl and everybody else who touches it also is under rtnl. Awesome catch. Thanks, will resubmit another patch (hard to call it v2...). > >Cheers, > Nik > >> CC: Jay Vosburgh >> CC: Andy Gospodarek >> CC: Nikolay Aleksandrov >> Signed-off-by: Veaceslav Falico >> --- >