netdev - Re: [PATCH] wwan_hwsim: Avoid flush_scheduled

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] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 21 Apr 2022 19:35:52 +0300
From:   Sergey Ryazanov <ryazanov.s.a@...il.com>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Loic Poulain <loic.poulain@...aro.org>,
        Johannes Berg <johannes@...solutions.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH] wwan_hwsim: Avoid flush_scheduled_work() usage

On Wed, Apr 20, 2022 at 5:22 AM Tetsuo Handa
<penguin-kernel@...ove.sakura.ne.jp> wrote:
> Flushing system-wide workqueues is dangerous and will be forbidden.
> Replace system_wq with local wwan_wq.
>
> Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>

Looks good! Just a couple minor questions below.

Reviewed-by: Sergey Ryazanov <ryazanov.s.a@...il.com>

> ---
> Note: This patch is only compile tested. By the way, don't you want to call
> debugfs_remove(wwan_hwsim_debugfs_devcreate) at err_clean_devs label in
> wwan_hwsim_init() like wwan_hwsim_exit() does, for debugfs_create_file("devcreate")
> is called before "goto err_clean_devs" happens?

As I replied in another mail. This is not strictly required, but will
not hurt anyone.

>  drivers/net/wwan/wwan_hwsim.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c
> index 5b62cf3b3c42..2136319f588f 100644
> --- a/drivers/net/wwan/wwan_hwsim.c
> +++ b/drivers/net/wwan/wwan_hwsim.c
> @@ -33,6 +33,7 @@ static struct dentry *wwan_hwsim_debugfs_devcreate;
>  static DEFINE_SPINLOCK(wwan_hwsim_devs_lock);
>  static LIST_HEAD(wwan_hwsim_devs);
>  static unsigned int wwan_hwsim_dev_idx;
> +static struct workqueue_struct *wwan_wq;
>
>  struct wwan_hwsim_dev {
>         struct list_head list;
> @@ -371,7 +372,7 @@ static ssize_t wwan_hwsim_debugfs_portdestroy_write(struct file *file,
>          * waiting this callback to finish in the debugfs_remove() call. So,
>          * use workqueue.
>          */
> -       schedule_work(&port->del_work);
> +       queue_work(wwan_wq, &port->del_work);
>
>         return count;
>  }
> @@ -416,7 +417,7 @@ static ssize_t wwan_hwsim_debugfs_devdestroy_write(struct file *file,
>          * waiting this callback to finish in the debugfs_remove() call. So,
>          * use workqueue.
>          */
> -       schedule_work(&dev->del_work);
> +       queue_work(wwan_wq, &dev->del_work);
>
>         return count;
>  }
> @@ -506,9 +507,15 @@ static int __init wwan_hwsim_init(void)
>         if (wwan_hwsim_devsnum < 0 || wwan_hwsim_devsnum > 128)
>                 return -EINVAL;
>
> +       wwan_wq = alloc_workqueue("wwan_wq", 0, 0);
> +       if (!wwan_wq)
> +               return -ENOMEM;
> +
>         wwan_hwsim_class = class_create(THIS_MODULE, "wwan_hwsim");
> -       if (IS_ERR(wwan_hwsim_class))
> +       if (IS_ERR(wwan_hwsim_class)) {
> +               destroy_workqueue(wwan_wq);

How about jumping to some label from here and do the workqueue
destroying there? E.g.

err = PTR_ERR(wwan_hwsim_class);
goto err_wq_destroy;

This will keep code symmetric.

>                 return PTR_ERR(wwan_hwsim_class);
> +       }
>
>         wwan_hwsim_debugfs_topdir = debugfs_create_dir("wwan_hwsim", NULL);
>         wwan_hwsim_debugfs_devcreate =
> @@ -524,6 +531,7 @@ static int __init wwan_hwsim_init(void)
>
>  err_clean_devs:
>         wwan_hwsim_free_devs();
> +       destroy_workqueue(wwan_wq);
>         debugfs_remove(wwan_hwsim_debugfs_topdir);
>         class_destroy(wwan_hwsim_class);

As you can see there are no need to wait the workqueue flushing, since
it was not used. So the queue destroying call can be moved below the
class destroying to keep cleanup symmetrical to the init sequence.
E.g.

 err_clean_devs:
        wwan_hwsim_free_devs();
        debugfs_remove(wwan_hwsim_debugfs_topdir);
        class_destroy(wwan_hwsim_class);

+err_wq_destroy:
+       destroy_workqueue(wwan_wq);
+
       return err;
}

> @@ -534,7 +542,7 @@ static void __exit wwan_hwsim_exit(void)
>  {
>         debugfs_remove(wwan_hwsim_debugfs_devcreate);   /* Avoid new devs */
>         wwan_hwsim_free_devs();
> -       flush_scheduled_work();         /* Wait deletion works completion */
> +       destroy_workqueue(wwan_wq);             /* Wait deletion works completion */
>         debugfs_remove(wwan_hwsim_debugfs_topdir);
>         class_destroy(wwan_hwsim_class);
>  }

I do not care too much, but can we explicitly call the queue flushing
to make  the exit handler as clear as possible?

 {
        debugfs_remove(wwan_hwsim_debugfs_devcreate);   /* Avoid new devs */
        wwan_hwsim_free_devs();
-       flush_scheduled_work();         /* Wait deletion works completion */
+       flush_workqueue(wwan_wq);             /* Wait deletion works
completion */
        debugfs_remove(wwan_hwsim_debugfs_topdir);
        class_destroy(wwan_hwsim_class);
+       destroy_workqueue(wwan_wq);
 }

-- 
Sergey