| Message ID | 20241217140323.782346-1-kory.maincent@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ethtool: Fix suspicious rcu_dereference usage | expand |
On Tue, Dec 17, 2024 at 3:03 PM Kory Maincent <kory.maincent@bootlin.com> wrote: > > The __ethtool_get_ts_info function can be called with or without the > rtnl lock held. When the rtnl lock is not held, using rtnl_dereference() > triggers a warning due to improper lock context. > > Replace rtnl_dereference() with rcu_dereference_rtnl() to safely > dereference the RCU pointer in both scenarios, ensuring proper handling > regardless of the rtnl lock state. > > Reported-by: syzbot+a344326c05c98ba19682@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/netdev/676147f8.050a0220.37aaf.0154.GAE@google.com/ > Fixes: b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to support several hwtstamp by net topology") > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> > --- > net/ethtool/common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ethtool/common.c b/net/ethtool/common.c > index 02f941f667dd..ec6f2e2caaf9 100644 > --- a/net/ethtool/common.c > +++ b/net/ethtool/common.c > @@ -870,7 +870,7 @@ int __ethtool_get_ts_info(struct net_device *dev, > { > struct hwtstamp_provider *hwprov; > > - hwprov = rtnl_dereference(dev->hwprov); > + hwprov = rcu_dereference_rtnl(dev->hwprov); > /* No provider specified, use default behavior */ > if (!hwprov) { > const struct ethtool_ops *ops = dev->ethtool_ops; I have to ask : Can you tell how this patch has been tested ? rcu is not held according to syzbot report. If rtnl and rcu are not held, lockdep will still complain.
On Tue, 17 Dec 2024 16:47:07 +0100 Eric Dumazet <edumazet@google.com> wrote: > On Tue, Dec 17, 2024 at 3:03 PM Kory Maincent <kory.maincent@bootlin.com> > wrote: > > > > The __ethtool_get_ts_info function can be called with or without the > > rtnl lock held. When the rtnl lock is not held, using rtnl_dereference() > > triggers a warning due to improper lock context. > > > > Replace rtnl_dereference() with rcu_dereference_rtnl() to safely > > dereference the RCU pointer in both scenarios, ensuring proper handling > > regardless of the rtnl lock state. > > > > Reported-by: syzbot+a344326c05c98ba19682@syzkaller.appspotmail.com > > Closes: > > https://lore.kernel.org/netdev/676147f8.050a0220.37aaf.0154.GAE@google.com/ > > Fixes: b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to support > > several hwtstamp by net topology") Signed-off-by: Kory Maincent > > <kory.maincent@bootlin.com> --- net/ethtool/common.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/ethtool/common.c b/net/ethtool/common.c > > index 02f941f667dd..ec6f2e2caaf9 100644 > > --- a/net/ethtool/common.c > > +++ b/net/ethtool/common.c > > @@ -870,7 +870,7 @@ int __ethtool_get_ts_info(struct net_device *dev, > > { > > struct hwtstamp_provider *hwprov; > > > > - hwprov = rtnl_dereference(dev->hwprov); > > + hwprov = rcu_dereference_rtnl(dev->hwprov); > > /* No provider specified, use default behavior */ > > if (!hwprov) { > > const struct ethtool_ops *ops = dev->ethtool_ops; > > I have to ask : Can you tell how this patch has been tested ? Oh, it was not at all sufficiently tested. Sorry! I thought I had spotted the issue but I hadn't. > rcu is not held according to syzbot report. > > If rtnl and rcu are not held, lockdep will still complain. You are totally right. I may be able to see it with the timestamping kselftest. I will try. Regards,
On Tue, Dec 17, 2024 at 6:17 PM Kory Maincent <kory.maincent@bootlin.com> wrote: > > On Tue, 17 Dec 2024 16:47:07 +0100 > Eric Dumazet <edumazet@google.com> wrote: > > > On Tue, Dec 17, 2024 at 3:03 PM Kory Maincent <kory.maincent@bootlin.com> > > wrote: > > > > > > The __ethtool_get_ts_info function can be called with or without the > > > rtnl lock held. When the rtnl lock is not held, using rtnl_dereference() > > > triggers a warning due to improper lock context. > > > > > > Replace rtnl_dereference() with rcu_dereference_rtnl() to safely > > > dereference the RCU pointer in both scenarios, ensuring proper handling > > > regardless of the rtnl lock state. > > > > > > Reported-by: syzbot+a344326c05c98ba19682@syzkaller.appspotmail.com > > > Closes: > > > https://lore.kernel.org/netdev/676147f8.050a0220.37aaf.0154.GAE@google.com/ > > > Fixes: b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to support > > > several hwtstamp by net topology") Signed-off-by: Kory Maincent > > > <kory.maincent@bootlin.com> --- net/ethtool/common.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/net/ethtool/common.c b/net/ethtool/common.c > > > index 02f941f667dd..ec6f2e2caaf9 100644 > > > --- a/net/ethtool/common.c > > > +++ b/net/ethtool/common.c > > > @@ -870,7 +870,7 @@ int __ethtool_get_ts_info(struct net_device *dev, > > > { > > > struct hwtstamp_provider *hwprov; > > > > > > - hwprov = rtnl_dereference(dev->hwprov); > > > + hwprov = rcu_dereference_rtnl(dev->hwprov); > > > /* No provider specified, use default behavior */ > > > if (!hwprov) { > > > const struct ethtool_ops *ops = dev->ethtool_ops; > > > > I have to ask : Can you tell how this patch has been tested ? > > Oh, it was not at all sufficiently tested. Sorry! > I thought I had spotted the issue but I hadn't. > > > rcu is not held according to syzbot report. > > > > If rtnl and rcu are not held, lockdep will still complain. > > You are totally right. > I may be able to see it with the timestamping kselftest. I will try. syzbot has a repro that you can compile and run. Make sure to build and use a kernel with CONFIG_PROVE_LOCKING=y
diff --git a/net/ethtool/common.c b/net/ethtool/common.c index 02f941f667dd..ec6f2e2caaf9 100644 --- a/net/ethtool/common.c +++ b/net/ethtool/common.c @@ -870,7 +870,7 @@ int __ethtool_get_ts_info(struct net_device *dev, { struct hwtstamp_provider *hwprov; - hwprov = rtnl_dereference(dev->hwprov); + hwprov = rcu_dereference_rtnl(dev->hwprov); /* No provider specified, use default behavior */ if (!hwprov) { const struct ethtool_ops *ops = dev->ethtool_ops;
The __ethtool_get_ts_info function can be called with or without the rtnl lock held. When the rtnl lock is not held, using rtnl_dereference() triggers a warning due to improper lock context. Replace rtnl_dereference() with rcu_dereference_rtnl() to safely dereference the RCU pointer in both scenarios, ensuring proper handling regardless of the rtnl lock state. Reported-by: syzbot+a344326c05c98ba19682@syzkaller.appspotmail.com Closes: https://lore.kernel.org/netdev/676147f8.050a0220.37aaf.0154.GAE@google.com/ Fixes: b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to support several hwtstamp by net topology") Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> --- net/ethtool/common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)