Messages in this thread | | | Date | Wed, 18 May 2022 11:54:27 -0400 | Subject | Re: [PATCHv2 net] bonding: fix missed rcu protection | From | Jonathan Toppins <> |
| |
On 5/17/22 22:18, Hangbin Liu wrote: > On Tue, May 17, 2022 at 01:32:58PM -0400, Jonathan Toppins wrote: >> Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> >> --- >> RESEND, list still didn't receive my last version >> >> The diffstat is slightly larger but IMO a slightly more readable version. >> When I was reading v2 I found myself jumping around. > > Hi Jon, > > Thanks for the commit. But.. > >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 38e152548126..f9d27b63c454 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -5591,23 +5591,32 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev, >> const struct ethtool_ops *ops; >> struct net_device *real_dev; >> struct phy_device *phydev; >> + int ret = 0; >> >> + rcu_read_lock(); >> real_dev = bond_option_active_slave_get_rcu(bond); >> - if (real_dev) { >> - ops = real_dev->ethtool_ops; >> - phydev = real_dev->phydev; >> - >> - if (phy_has_tsinfo(phydev)) { >> - return phy_ts_info(phydev, info); >> - } else if (ops->get_ts_info) { >> - return ops->get_ts_info(real_dev, info); >> - } >> - } >> + if (real_dev) >> + dev_hold(real_dev); >> + rcu_read_unlock(); >> + >> + if (!real_dev) >> + goto software; >> >> + ops = real_dev->ethtool_ops; >> + phydev = real_dev->phydev; >> + >> + if (phy_has_tsinfo(phydev)) >> + ret = phy_ts_info(phydev, info); >> + else if (ops->get_ts_info) >> + ret = ops->get_ts_info(real_dev, info); > else { > dev_put(real_dev); > goto software; > } > > Here we need another check and goto software if !phy_has_tsinfo() and > no ops->get_ts_info. With this change we also have 2 goto and dev_put().
Ah yes. I cannot think of a way to make this simpler. The patch below looks good.
> >> + >> + dev_put(real_dev); >> + return ret; >> + >> +software: >> info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | >> SOF_TIMESTAMPING_SOFTWARE; >> info->phc_index = -1; >> - >> return 0; >> } > > As Jakub remind, dev_hold() and dev_put() can take NULL now. So how about > this new patch: > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 38e152548126..b5c5196e03ee 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -5591,16 +5591,23 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev, > const struct ethtool_ops *ops; > struct net_device *real_dev; > struct phy_device *phydev; > + int ret = 0; > > + rcu_read_lock(); > real_dev = bond_option_active_slave_get_rcu(bond); > + dev_hold(real_dev); > + rcu_read_unlock(); > + > if (real_dev) { > ops = real_dev->ethtool_ops; > phydev = real_dev->phydev; > > if (phy_has_tsinfo(phydev)) { > - return phy_ts_info(phydev, info); > + ret = phy_ts_info(phydev, info); > + goto out; > } else if (ops->get_ts_info) { > - return ops->get_ts_info(real_dev, info); > + ret = ops->get_ts_info(real_dev, info); > + goto out; > } > } > > @@ -5608,7 +5615,9 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev, > SOF_TIMESTAMPING_SOFTWARE; > info->phc_index = -1; > > - return 0; > +out: > + dev_put(real_dev); > + return ret; > } > > Thanks > Hangbin >
| |