| Message ID | 066463d84fa14d5f61247b95340fca12d4d3bf34.1721784184.git.jamie.bainbridge@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net-sysfs: check device is present when showing paths | expand |
On Wed, 2024-07-24 at 01:46 +0000, Jamie Bainbridge wrote: > A sysfs reader can race with a device reset or removal. Kind of, yes, but please check what the race actually is. > This was fixed for speed_show with commit 4224cfd7fb65 ("net-sysfs: add > check for netdevice being present to speed_show") so add the same check > to carrier_show. You didn't say why it's needed here, so ... why is it? FWIW, I don't think it actually _is_ needed, since the netdev struct itself is still around, linkwatch_sync_dev() will not do anything that's not still needed anyway (the removal from list must clearly either still happen or nothing happens in the function). This will not call into the driver (which would be the problematic part). So while I don't think this is _wrong_ per se, I also don't think it's necessary, nor are you demonstrating that it is. And for userspace it should be pretty much immaterial whether it gets a real value or -EINVAL in the race, or -ENOENT because the file disappeared anyway? johannes
On Wed, 2024-07-24 at 11:35 +0200, Johannes Berg wrote: > On Wed, 2024-07-24 at 01:46 +0000, Jamie Bainbridge wrote: > > A sysfs reader can race with a device reset or removal. > > Kind of, yes, but please check what the race actually is. > > > This was fixed for speed_show with commit 4224cfd7fb65 ("net-sysfs: add > > check for netdevice being present to speed_show") so add the same check > > to carrier_show. > > You didn't say why it's needed here, so ... why is it? > > FWIW, I don't think it actually _is_ needed, since the netdev struct > itself is still around, linkwatch_sync_dev() will not do anything that's > not still needed anyway (the removal from list must clearly either still > happen or nothing happens in the function). This will not call into the > driver (which would be the problematic part). > > So while I don't think this is _wrong_ per se, I also don't think it's > necessary, nor are you demonstrating that it is. > > And for userspace it should be pretty much immaterial whether it gets a > real value or -EINVAL in the race, or -ENOENT because the file > disappeared anyway? > All of which, btw, is also true for patches 3 and 4 in this set. For patch 2 it seems applicable. I do wonder if ethtool itself, at least ethtool netlink, doesn't have a similar problem though, since it just uses netdev_get_by_name() / netdev_get_by_index()? johannes
On Wed, 24 Jul 2024 at 19:42, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Wed, 2024-07-24 at 11:35 +0200, Johannes Berg wrote: > > On Wed, 2024-07-24 at 01:46 +0000, Jamie Bainbridge wrote: > > > A sysfs reader can race with a device reset or removal. > > > > Kind of, yes, but please check what the race actually is. > > > > > This was fixed for speed_show with commit 4224cfd7fb65 ("net-sysfs: add > > > check for netdevice being present to speed_show") so add the same check > > > to carrier_show. > > > > You didn't say why it's needed here, so ... why is it? > > > > FWIW, I don't think it actually _is_ needed, since the netdev struct > > itself is still around, linkwatch_sync_dev() will not do anything that's > > not still needed anyway (the removal from list must clearly either still > > happen or nothing happens in the function). This will not call into the > > driver (which would be the problematic part). > > > > So while I don't think this is _wrong_ per se, I also don't think it's > > necessary, nor are you demonstrating that it is. > > > > And for userspace it should be pretty much immaterial whether it gets a > > real value or -EINVAL in the race, or -ENOENT because the file > > disappeared anyway? > > > > All of which, btw, is also true for patches 3 and 4 in this set. > > For patch 2 it seems applicable. > > I do wonder if ethtool itself, at least ethtool netlink, doesn't have a > similar problem though, since it just uses netdev_get_by_name() / > netdev_get_by_index()? > > johannes You are correct, patch 2 (duplex) is the one where we panicked during device reset. I thought to fix the other "show" functions in advance while I was there. I will revise this and re-submit with only the necessary patch. Thanks for the review, it is appreciated. Jamie
Hi Jamie, On Wed, 24 Jul 2024 11:46:50 +1000, Jamie Bainbridge wrote: > A sysfs reader can race with a device reset or removal. > > This was fixed for speed_show with commit 4224cfd7fb65 ("net-sysfs: add > check for netdevice being present to speed_show") so add the same check > to carrier_show. > > Fixes: facd15dfd691 ("net: core: synchronize link-watch when carrier is queried") > > Signed-off-by: Jamie Bainbridge <jamie.bainbridge@gmail.com> nit: When resubmitting your fix, there is no need for a blank line between the Fixes and SOB tags. Thanks, Shigeru > --- > net/core/net-sysfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c > index 0e2084ce7b7572bff458ed7e02358d9258c74628..7fabe5afa3012ecad6551e12f478b755952933b8 100644 > --- a/net/core/net-sysfs.c > +++ b/net/core/net-sysfs.c > @@ -206,7 +206,7 @@ static ssize_t carrier_show(struct device *dev, > if (!rtnl_trylock()) > return restart_syscall(); > > - if (netif_running(netdev)) { > + if (netif_running(netdev) && netif_device_present(netdev)) { > /* Synchronize carrier state with link watch, > * see also rtnl_getlink(). > */ > -- > 2.39.2 > >
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index 0e2084ce7b7572bff458ed7e02358d9258c74628..7fabe5afa3012ecad6551e12f478b755952933b8 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -206,7 +206,7 @@ static ssize_t carrier_show(struct device *dev, if (!rtnl_trylock()) return restart_syscall(); - if (netif_running(netdev)) { + if (netif_running(netdev) && netif_device_present(netdev)) { /* Synchronize carrier state with link watch, * see also rtnl_getlink(). */
A sysfs reader can race with a device reset or removal. This was fixed for speed_show with commit 4224cfd7fb65 ("net-sysfs: add check for netdevice being present to speed_show") so add the same check to carrier_show. Fixes: facd15dfd691 ("net: core: synchronize link-watch when carrier is queried") Signed-off-by: Jamie Bainbridge <jamie.bainbridge@gmail.com> --- net/core/net-sysfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)