[PATCH net] net: avoid race between device unregistration and set_channels

[PATCH net] net: avoid race between device unregistration and set_channels

[Antoine Tenart]

The following trace can be seen if a device is being unregistered while
its number of channels are being modified.

  DEBUG_LOCKS_WARN_ON(lock->magic != lock)
  WARNING: CPU: 3 PID: 3754 at kernel/locking/mutex.c:564 __mutex_lock+0xc8a/0x1120
  CPU: 3 UID: 0 PID: 3754 Comm: ethtool Not tainted 6.13.0-rc6+ #771
  RIP: 0010:__mutex_lock+0xc8a/0x1120
  Call Trace:
   <TASK>
   ethtool_check_max_channel+0x1ea/0x880
   ethnl_set_channels+0x3c3/0xb10
   ethnl_default_set_doit+0x306/0x650
   genl_family_rcv_msg_doit+0x1e3/0x2c0
   genl_rcv_msg+0x432/0x6f0
   netlink_rcv_skb+0x13d/0x3b0
   genl_rcv+0x28/0x40
   netlink_unicast+0x42e/0x720
   netlink_sendmsg+0x765/0xc20
   __sys_sendto+0x3ac/0x420
   __x64_sys_sendto+0xe0/0x1c0
   do_syscall_64+0x95/0x180
   entry_SYSCALL_64_after_hwframe+0x76/0x7e

This is because unregister_netdevice_many_notify might run before
set_channels (both are under rtnl). When that happens, the rss lock is
being destroyed before being used again. Fix this by destroying the rss
lock in run_todo, outside an rtnl lock section and after all references
to net devices are gone.

Note that allowing to run set_channels after the rtnl section of the
unregistration path should be fine as it still runs before the
destructors (thanks to refcount). This patch does not change that.

Fixes: 87925151191b ("net: ethtool: add a mutex protecting RSS contexts")
Cc: Edward Cree <ecree.xilinx@gmail.com>
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 net/core/dev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index a9f62f5aeb84..d8491a275e2e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10931,6 +10931,8 @@ void netdev_run_todo(void)
 		WARN_ON(rcu_access_pointer(dev->ip_ptr));
 		WARN_ON(rcu_access_pointer(dev->ip6_ptr));
 
+		mutex_destroy(&dev->ethtool->rss_lock);
+
 		netdev_do_free_pcpu_stats(dev);
 		if (dev->priv_destructor)
 			dev->priv_destructor(dev);
@@ -11566,8 +11568,6 @@ void unregister_netdevice_many_notify(struct list_head *head,
 		if (dev->netdev_ops->ndo_uninit)
 			dev->netdev_ops->ndo_uninit(dev);
 
-		mutex_destroy(&dev->ethtool->rss_lock);
-
 		net_shaper_flush_netdev(dev);
 
 		if (skb)
-- 
2.47.1

Re: [PATCH net] net: avoid race between device unregistration and set_channels

[Edward Cree]

On 13/01/2025 16:18, Antoine Tenart wrote:
> This is because unregister_netdevice_many_notify might run before
> set_channels (both are under rtnl). When that happens, the rss lock is
> being destroyed before being used again. Fix this by destroying the rss
> lock in run_todo, outside an rtnl lock section and after all references
> to net devices are gone.

The latter (refs gone) being the important part?  Doesn't seem
 particularly relevant that we've dropped rtnl, this wording had me
 confused for a little while as to why this closed the race.

> Note that allowing to run set_channels after the rtnl section of the
> unregistration path should be fine as it still runs before the
> destructors (thanks to refcount). This patch does not change that.
> 
> Fixes: 87925151191b ("net: ethtool: add a mutex protecting RSS contexts")
> Cc: Edward Cree <ecree.xilinx@gmail.com>
> Signed-off-by: Antoine Tenart <atenart@kernel.org>

Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>

Re: [PATCH net] net: avoid race between device unregistration and set_channels

[Antoine Tenart]

Quoting Edward Cree (2025-01-14 11:09:07)
> On 13/01/2025 16:18, Antoine Tenart wrote:
> > This is because unregister_netdevice_many_notify might run before
> > set_channels (both are under rtnl). When that happens, the rss lock is
> > being destroyed before being used again. Fix this by destroying the rss
> > lock in run_todo, outside an rtnl lock section and after all references
> > to net devices are gone.
> 
> The latter (refs gone) being the important part?  Doesn't seem
>  particularly relevant that we've dropped rtnl, this wording had me
>  confused for a little while as to why this closed the race.

Both are important as being outside the rtnl lock section means
set_channels had a chance to run, and waiting for refs that it
completed. Well, otherwise there would be a deadlock, so maybe this is a
superfluous detail after all.

> > Note that allowing to run set_channels after the rtnl section of the
> > unregistration path should be fine as it still runs before the
> > destructors (thanks to refcount). This patch does not change that.
> > 
> > Fixes: 87925151191b ("net: ethtool: add a mutex protecting RSS contexts")
> > Cc: Edward Cree <ecree.xilinx@gmail.com>
> > Signed-off-by: Antoine Tenart <atenart@kernel.org>
> 
> Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>

Thanks!

Re: [PATCH net] net: avoid race between device unregistration and set_channels

[Jakub Kicinski]

On Mon, 13 Jan 2025 17:18:40 +0100 Antoine Tenart wrote:
> This is because unregister_netdevice_many_notify might run before
> set_channels (both are under rtnl). 

But that is very bad, not at all sane. The set call should not proceed
once dismantle begins.

How about this?

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index 849c98e637c6..913c8e329a06 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -90,7 +90,7 @@ int ethnl_ops_begin(struct net_device *dev)
                pm_runtime_get_sync(dev->dev.parent);
 
        if (!netif_device_present(dev) ||
-           dev->reg_state == NETREG_UNREGISTERING) {
+           dev->reg_state > NETREG_REGISTERED) {
                ret = -ENODEV;
                goto err;
        }

Re: [PATCH net] net: avoid race between device unregistration and set_channels

[Edward Cree]

On 14/01/2025 19:24, Jakub Kicinski wrote:
> On Mon, 13 Jan 2025 17:18:40 +0100 Antoine Tenart wrote:
>> This is because unregister_netdevice_many_notify might run before
>> set_channels (both are under rtnl). 
> 
> But that is very bad, not at all sane. The set call should not proceed
> once dismantle begins.
> 
> How about this?
> 
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index 849c98e637c6..913c8e329a06 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -90,7 +90,7 @@ int ethnl_ops_begin(struct net_device *dev)
>                 pm_runtime_get_sync(dev->dev.parent);
>  
>         if (!netif_device_present(dev) ||
> -           dev->reg_state == NETREG_UNREGISTERING) {
> +           dev->reg_state > NETREG_REGISTERED) {
>                 ret = -ENODEV;
>                 goto err;
>         }
> 

Would __dev_ethtool() need a similar check?

Re: [PATCH net] net: avoid race between device unregistration and set_channels

[Antoine Tenart]

Quoting Edward Cree (2025-01-15 03:51:12)
> On 14/01/2025 19:24, Jakub Kicinski wrote:
> > On Mon, 13 Jan 2025 17:18:40 +0100 Antoine Tenart wrote:
> >> This is because unregister_netdevice_many_notify might run before
> >> set_channels (both are under rtnl). 
> > 
> > But that is very bad, not at all sane. The set call should not proceed
> > once dismantle begins.
> > 
> > How about this?
> > 
> > diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index 849c98e637c6..913c8e329a06 100644
> > --- a/net/ethtool/netlink.c
> > +++ b/net/ethtool/netlink.c
> > @@ -90,7 +90,7 @@ int ethnl_ops_begin(struct net_device *dev)
> >                 pm_runtime_get_sync(dev->dev.parent);
> >  
> >         if (!netif_device_present(dev) ||
> > -           dev->reg_state == NETREG_UNREGISTERING) {
> > +           dev->reg_state > NETREG_REGISTERED) {
> >                 ret = -ENODEV;
> >                 goto err;
> >         }
> > 
> 
> Would __dev_ethtool() need a similar check?

It doesn't because it calls __dev_get_by_name() and returns -ENODEV in
case dismantle started.

Re: [PATCH net] net: avoid race between device unregistration and set_channels

[Edward Cree]

On 15/01/2025 09:41, Antoine Tenart wrote:
> Quoting Edward Cree (2025-01-15 03:51:12)
>> Would __dev_ethtool() need a similar check?
> 
> It doesn't because it calls __dev_get_by_name() and returns -ENODEV in
> case dismantle started.
> 
So, to check I'm understanding correctly - this is because
 ethnl_default_set_doit() calls ethnl_parse_header_dev_get()
 before it takes rtnl, whereas ethtool takes rtnl before it
 calls __dev_get_by_name()?
Subtle enough difference that the commit message should
 probably explain it.

Re: [PATCH net] net: avoid race between device unregistration and set_channels

[Antoine Tenart]

Quoting Edward Cree (2025-01-15 12:04:58)
> On 15/01/2025 09:41, Antoine Tenart wrote:
> > Quoting Edward Cree (2025-01-15 03:51:12)
> >> Would __dev_ethtool() need a similar check?
> > 
> > It doesn't because it calls __dev_get_by_name() and returns -ENODEV in
> > case dismantle started.
> > 
> So, to check I'm understanding correctly - this is because
>  ethnl_default_set_doit() calls ethnl_parse_header_dev_get()
>  before it takes rtnl, whereas ethtool takes rtnl before it
>  calls __dev_get_by_name()?
> Subtle enough difference that the commit message should
>  probably explain it.

That's right, and because unlist_netdevice is called from
unregister_netdevice_many_notify. I can add the explanation in the
commit log.

Thanks,
Antoine

Re: [PATCH net] net: avoid race between device unregistration and set_channels

[Antoine Tenart]

Quoting Jakub Kicinski (2025-01-14 20:24:01)
> On Mon, 13 Jan 2025 17:18:40 +0100 Antoine Tenart wrote:
> > This is because unregister_netdevice_many_notify might run before
> > set_channels (both are under rtnl). 
> 
> But that is very bad, not at all sane. The set call should not proceed
> once dismantle begins.
> 
> How about this?
> 
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index 849c98e637c6..913c8e329a06 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -90,7 +90,7 @@ int ethnl_ops_begin(struct net_device *dev)
>                 pm_runtime_get_sync(dev->dev.parent);
>  
>         if (!netif_device_present(dev) ||
> -           dev->reg_state == NETREG_UNREGISTERING) {
> +           dev->reg_state > NETREG_REGISTERED) {
>                 ret = -ENODEV;
>                 goto err;
>         }

That looks better, I'll send a v2 with this. Now I recall adding the
above for a similar issue...

This makes me think we should do the same in the trylock/restart_syscall
series as this will also allow calls to run after dismantle begins. It
also might be time to make dev_isalive available and use it outside of
net-sysfs.

Thanks!
Antoine