netdev - Re: [PATCH RFC V1 net-next 1/4] net: ethtool: Refactor identical get_ts

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

Date:   Thu, 20 Jan 2022 21:20:39 -0800
From:   Jay Vosburgh <jay.vosburgh@...onical.com>
To:     Hangbin Liu <liuhangbin@...il.com>
cc:     Vladimir Oltean <vladimir.oltean@....com>,
        Richard Cochran <richardcochran@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>
Subject: Re: [PATCH RFC V1 net-next 1/4] net: ethtool: Refactor identical get_ts_info implementations.

Hangbin Liu <liuhangbin@...il.com> wrote:

>On Thu, Jan 20, 2022 at 04:13:29PM +0000, Vladimir Oltean wrote:
>> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> > index b60e22f6394a..f28b88b67b9e 100644
>> > --- a/drivers/net/bonding/bond_main.c
>> > +++ b/drivers/net/bonding/bond_main.c
>> > @@ -5353,23 +5353,13 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
>> >  				    struct ethtool_ts_info *info)
>> >  {
>> >  	struct bonding *bond = netdev_priv(bond_dev);
>> > -	const struct ethtool_ops *ops;
>> >  	struct net_device *real_dev;
>> > -	struct phy_device *phydev;
>> >  
>> >  	rcu_read_lock();
>> >  	real_dev = bond_option_active_slave_get_rcu(bond);
>> >  	rcu_read_unlock();
>> 
>> Side note: I'm a bit confused about this rcu_read_lock() ->
>> rcu_dereference_protected() -> rcu_read_unlock() pattern, and use of the
>> real_dev outside the RCU critical section. Isn't ->get_ts_info()
>> protected by the rtnl_mutex? Shouldn't there be a
>> bond_option_active_slave_get() which uses rtnl_dereference()?
>> I see the code has been recently added by Hangbin Liu.
>
>Hi Vladimir,
>
>Yes, it should be enough to use rtnl_dereference() since ->get_ts_info is
>protected by the rtnl_lock. I just thought there is an existing get active
>slave function and rcu read should be OK to be used here. So I just used the
>existing one.
>
>Hi Jay,
>
>Do you think if there is a need to add a rtnl version of
>bond_option_active_slave_get()?

	I think the RCU primitives used should match the calling
context, so, yes, there should be an "rtnl" flavor in this call path as
that's really what's being relied upon.

	You could add a "...active_slave_get_rtnl" variant, or there's
rcu_deference_rtnl() that goes both ways (rcu_read_lock or RTNL).  That
could be used in bond_option_active_slave_get_rcu() in place of the
current rcu_dereference().  I don't have a strong preference one way or
the other.

	Either way, as mentioned, I agree that the rcu_read_lock/unlock
in bond_ethtool_get_ts_info() is superfluous, since the whole call is
under RTNL.

	-J

---
	-Jay Vosburgh, jay.vosburgh@...onical.com