* Re: [PATCHv2 net] bonding: fix missed rcu protection [not found] <20220517082312.805824-1-liuhangbin@gmail.com> @ 2022-05-17 17:32 ` Jonathan Toppins 2022-05-18 2:18 ` Hangbin Liu 2022-05-19 14:34 ` Vladimir Oltean 0 siblings, 2 replies; 4+ messages in thread From: Jonathan Toppins @ 2022-05-17 17:32 UTC (permalink / raw) To: liuhangbin Cc: andy, davem, dsahern, eric.dumazet, j.vosburgh, jtoppins, kuba, netdev, pabeni, syzbot+92beb3d46aab498710fa, vfalico, vladimir.oltean, Eric Dumazet, linux-kernel 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. I only compile tested it, so YMMV. If this amount of change is too much v2 from Hangbin looks correct to me. drivers/net/bonding/bond_main.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) 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); + + dev_put(real_dev); + return ret; + +software: info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | SOF_TIMESTAMPING_SOFTWARE; info->phc_index = -1; - return 0; } -- 2.27.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCHv2 net] bonding: fix missed rcu protection 2022-05-17 17:32 ` [PATCHv2 net] bonding: fix missed rcu protection Jonathan Toppins @ 2022-05-18 2:18 ` Hangbin Liu 2022-05-18 15:54 ` Jonathan Toppins 2022-05-19 14:34 ` Vladimir Oltean 1 sibling, 1 reply; 4+ messages in thread From: Hangbin Liu @ 2022-05-18 2:18 UTC (permalink / raw) To: Jonathan Toppins Cc: andy, davem, dsahern, eric.dumazet, j.vosburgh, kuba, netdev, pabeni, syzbot+92beb3d46aab498710fa, vfalico, vladimir.oltean, Eric Dumazet, linux-kernel 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(). > + > + 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCHv2 net] bonding: fix missed rcu protection 2022-05-18 2:18 ` Hangbin Liu @ 2022-05-18 15:54 ` Jonathan Toppins 0 siblings, 0 replies; 4+ messages in thread From: Jonathan Toppins @ 2022-05-18 15:54 UTC (permalink / raw) To: Hangbin Liu Cc: andy, davem, dsahern, eric.dumazet, j.vosburgh, kuba, netdev, pabeni, syzbot+92beb3d46aab498710fa, vfalico, vladimir.oltean, Eric Dumazet, linux-kernel 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCHv2 net] bonding: fix missed rcu protection 2022-05-17 17:32 ` [PATCHv2 net] bonding: fix missed rcu protection Jonathan Toppins 2022-05-18 2:18 ` Hangbin Liu @ 2022-05-19 14:34 ` Vladimir Oltean 1 sibling, 0 replies; 4+ messages in thread From: Vladimir Oltean @ 2022-05-19 14:34 UTC (permalink / raw) To: Jonathan Toppins Cc: liuhangbin, andy, davem, dsahern, eric.dumazet, j.vosburgh, kuba, netdev, pabeni, syzbot+92beb3d46aab498710fa, vfalico, Eric Dumazet, linux-kernel 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. > I only compile tested it, so YMMV. > > If this amount of change is too much v2 from Hangbin looks correct to > me. Seems to be too big of a change for what the issue is, yes, sorry. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-19 14:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20220517082312.805824-1-liuhangbin@gmail.com>
2022-05-17 17:32 ` [PATCHv2 net] bonding: fix missed rcu protection Jonathan Toppins
2022-05-18 2:18 ` Hangbin Liu
2022-05-18 15:54 ` Jonathan Toppins
2022-05-19 14:34 ` Vladimir Oltean
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®