netdev - Re: [PATCH 1/1] ixgbe: replace rtnl_lock with rcu_read

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>] [day] [month] [year] [list]

Date:	Mon, 6 Jun 2016 09:12:21 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	zhuyj <zyjzyj2000@...il.com>
Cc:	"e1000-devel@...ts.sourceforge.net" 
	<e1000-devel@...ts.sourceforge.net>,
	Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/1] ixgbe: replace rtnl_lock with rcu_read_lock

Just a quick scan has me wondering what code you are comparing it to?

The key bit here that is the reason for taking the RTNL lock is
because this section is handled in the watchdog which is not an RTNL
protected region, and because it is messing with devices other than
the ones controlled by the ixgbe driver itself.  As far as I can tell
I don't see similar code in the other drivers.  You end up having to
take the RTNL lock any time you start trying to manipulate some state
of a device that is not protected through other means.  For example
pretty much all the net device operations expect that the RTNL lock
has already been taken before calling them, however a driver can call
into those functions if it is maintaining the state for the devices
without the RTNL lock assuming it has some other means of keeping the
state consistent such as a __IXGBE_DOWN state bit in the ixgbe driver.

- Alex

On Mon, Jun 6, 2016 at 6:37 AM, zhuyj <zyjzyj2000@...il.com> wrote:
> Hi, Alex
>
> Thanks for your reply.
>
> I checked all the nic driver in driver/net/ethernet/intel. And I found that
> only here the rtnl_lock is used.
> So I am curios why rtnl_lock is used when waking up the tupper device tx
> queue here. And the rtnl_lock is not used in other places.
>
> Best Regards!
> Zhu Yanjun
>
> On Mon, Jun 6, 2016 at 2:40 AM, Alexander Duyck <alexander.duyck@...il.com>
> wrote:
>>
>> On Sun, Jun 5, 2016 at 2:14 AM,  <zyjzyj2000@...il.com> wrote:
>> > From: Zhu Yanjun <zyjzyj2000@...il.com>
>> >
>> >
>> > Signed-off-by: Zhu Yanjun <zyjzyj2000@...il.com>
>> > ---
>> >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> > index 088c47c..cb19cbc 100644
>> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> > @@ -6840,7 +6840,7 @@ static void ixgbe_watchdog_link_is_up(struct
>> > ixgbe_adapter *adapter)
>> >         netif_tx_wake_all_queues(adapter->netdev);
>> >
>> >         /* enable any upper devices */
>> > -       rtnl_lock();
>> > +       rcu_read_lock();
>> >         netdev_for_each_all_upper_dev_rcu(adapter->netdev, upper, iter)
>> > {
>> >                 if (netif_is_macvlan(upper)) {
>> >                         struct macvlan_dev *vlan = netdev_priv(upper);
>> > @@ -6849,7 +6849,7 @@ static void ixgbe_watchdog_link_is_up(struct
>> > ixgbe_adapter *adapter)
>> >                                 netif_tx_wake_all_queues(upper);
>> >                 }
>> >         }
>> > -       rtnl_unlock();
>> > +       rcu_read_unlock();
>> >
>> >         /* update the default user priority for VFs */
>> >         ixgbe_update_default_up(adapter);
>>
>> The rtnl_lock is being used to prevent any changes to the upper
>> devices while the interface is going through and updating the Tx queue
>> configuration on them.  Without that lock you introduce possible bugs
>> since you could have queues freed or added while this loop is in the
>> middle of trying to update the state of it.
>>
>> As a general rule you use rcu_read_lock when you are only reading an
>> RCU protected structure, you use rtnl_lock when you have to protect
>> the system from any other changes while you are updating network
>> configuration.  In this case netif_tx_wake_all_queues changes the
>> state of the upper device.  The use of rtnl_lock here is intentional
>> and it is best to just leave it as is unless you have some actual bug
>> you are seeing.
>>
>> - Alex
>
>