| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CADvbK_dS5Q9j8YF3inE5BULhcRwwmOTf8E+ahrukWBPEy_VsFw@mail.gmail.com>
Date: Wed, 16 Feb 2022 12:13:27 +0800
From: Xin Long <lucien.xin@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem <davem@...emloft.net>, network dev <netdev@...r.kernel.org>,
Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next 2/2] net: allow out-of-order netdev unregistration
On Wed, Feb 16, 2022 at 6:53 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Sprinkle for each loops to allow netdevices to be unregistered
> out of order, as their refs are released.
>
> This prevents problems caused by dependencies between netdevs
> which want to release references in their ->priv_destructor.
> See commit d6ff94afd90b ("vlan: move dev_put into vlan_dev_uninit")
> for example.
>
> Eric has removed the only known ordering requirement in
> commit c002496babfd ("Merge branch 'ipv6-loopback'")
> so let's try this and see if anything explodes...
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> net/core/dev.c | 64 +++++++++++++++++++++++++++++---------------------
> 1 file changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2749776e2dd2..05fa867f1878 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9811,8 +9811,8 @@ int netdev_unregister_timeout_secs __read_mostly = 10;
> #define WAIT_REFS_MIN_MSECS 1
> #define WAIT_REFS_MAX_MSECS 250
> /**
> - * netdev_wait_allrefs - wait until all references are gone.
> - * @dev: target net_device
> + * netdev_wait_allrefs_any - wait until all references are gone.
> + * @list: list of net_devices to wait on
> *
> * This is called when unregistering network devices.
> *
> @@ -9822,37 +9822,45 @@ int netdev_unregister_timeout_secs __read_mostly = 10;
> * We can get stuck here if buggy protocols don't correctly
> * call dev_put.
> */
> -static void netdev_wait_allrefs(struct net_device *dev)
> +static struct net_device *netdev_wait_allrefs_any(struct list_head *list)
> {
> unsigned long rebroadcast_time, warning_time;
> - int wait = 0, refcnt;
> + struct net_device *dev;
> + int wait = 0;
>
> - linkwatch_forget_dev(dev);
> + list_for_each_entry(dev, list, todo_list)
> + linkwatch_forget_dev(dev);
>
> rebroadcast_time = warning_time = jiffies;
> - refcnt = netdev_refcnt_read(dev);
>
> - while (refcnt != 1) {
> + list_for_each_entry(dev, list, todo_list)
> + if (netdev_refcnt_read(dev) == 1)
> + return dev;
> +
> + while (true) {
> if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
> rtnl_lock();
>
> /* Rebroadcast unregister notification */
> - call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
> + list_for_each_entry(dev, list, todo_list)
> + call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
>
> __rtnl_unlock();
> rcu_barrier();
> rtnl_lock();
>
> - if (test_bit(__LINK_STATE_LINKWATCH_PENDING,
> - &dev->state)) {
> - /* We must not have linkwatch events
> - * pending on unregister. If this
> - * happens, we simply run the queue
> - * unscheduled, resulting in a noop
> - * for this device.
> - */
> - linkwatch_run_queue();
> - }
> + list_for_each_entry(dev, list, todo_list)
> + if (test_bit(__LINK_STATE_LINKWATCH_PENDING,
> + &dev->state)) {
> + /* We must not have linkwatch events
> + * pending on unregister. If this
> + * happens, we simply run the queue
> + * unscheduled, resulting in a noop
> + * for this device.
> + */
> + linkwatch_run_queue();
> + break;
> + }
>
> __rtnl_unlock();
>
> @@ -9867,14 +9875,18 @@ static void netdev_wait_allrefs(struct net_device *dev)
> wait = min(wait << 1, WAIT_REFS_MAX_MSECS);
> }
>
> - refcnt = netdev_refcnt_read(dev);
> + list_for_each_entry(dev, list, todo_list)
> + if (netdev_refcnt_read(dev) == 1)
> + return dev;
>
> - if (refcnt != 1 &&
> - time_after(jiffies, warning_time +
> + if (time_after(jiffies, warning_time +
> netdev_unregister_timeout_secs * HZ)) {
> - pr_emerg("unregister_netdevice: waiting for %s to become free. Usage count = %d\n",
> - dev->name, refcnt);
> - ref_tracker_dir_print(&dev->refcnt_tracker, 10);
> + list_for_each_entry(dev, list, todo_list) {
> + pr_emerg("unregister_netdevice: waiting for %s to become free. Usage count = %d\n",
> + dev->name, netdev_refcnt_read(dev));
> + ref_tracker_dir_print(&dev->refcnt_tracker, 10);
> + }
> +
> warning_time = jiffies;
> }
> }
> @@ -9942,11 +9954,9 @@ void netdev_run_todo(void)
> }
>
> while (!list_empty(&list)) {
> - dev = list_first_entry(&list, struct net_device, todo_list);
> + dev = netdev_wait_allrefs_any(&list);
> list_del(&dev->todo_list);
>
> - netdev_wait_allrefs(dev);
> -
> /* paranoia */
> BUG_ON(netdev_refcnt_read(dev) != 1);
> BUG_ON(!list_empty(&dev->ptype_all));
> --
> 2.34.1
>
Reviewed-by: Xin Long <lucien.xin@...il.com>
Thanks for the fix, I will revert d6ff94afd90b once this gets applied.
Powered by blists - more mailing lists