netdev - Re: [PATCH net-next 2/2] net: allow out-of-order netdev unregistration

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANn89iLzQbsWh-pruEv2HO=+axpVR4KuGeo0GogzOTJL4ok7sg@mail.gmail.com>
Date:   Thu, 17 Feb 2022 22:36:12 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>, Xin Long <lucien.xin@...il.com>
Subject: Re: [PATCH net-next 2/2] net: allow out-of-order netdev unregistration

On Tue, Feb 15, 2022 at 2:53 PM 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);

Jakub, this part of the code added quadratic behavior (and soft lockups)
when a large list of devices is dismantled at once,
because we call netdev_wait_allrefs_any() N times.

I will test this fix, unless I have missed something ?

diff --git a/net/core/dev.c b/net/core/dev.c
index 05fa867f18787e709dcaccfea1df350c424eff80..74e77f861e2c99127b0349aa0f8a4be412cc187e
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9828,9 +9828,6 @@ static struct net_device
*netdev_wait_allrefs_any(struct list_head *list)
        struct net_device *dev;
        int wait = 0;

-       list_for_each_entry(dev, list, todo_list)
-               linkwatch_forget_dev(dev);
-
        rebroadcast_time = warning_time = jiffies;

        list_for_each_entry(dev, list, todo_list)
@@ -9953,6 +9950,9 @@ void netdev_run_todo(void)
                dev->reg_state = NETREG_UNREGISTERED;
        }

+       list_for_each_entry(dev, &list, todo_list)
+               linkwatch_forget_dev(dev);
+
        while (!list_empty(&list)) {
                dev = netdev_wait_allrefs_any(&list);
                list_del(&dev->todo_list);