Re: [PATCH RFC V1 net-next 1/4] net: ethtool: Refactor identical get_ts_info implementations.

[Jay Vosburgh]

Hangbin Liu <liuhangbin@xxxxxxxxx> 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@xxxxxxxxxxxxx