[PATCH v2] wwan_hwsim: Avoid flush_scheduled_work() usage

[PATCH v2] wwan_hwsim: Avoid flush_scheduled_work() usage

[Tetsuo Handa]

Flushing system-wide workqueues is dangerous and will be forbidden.
Replace system_wq with local wwan_wq.

While we are at it, make err_clean_devs: label of wwan_hwsim_init()
behave like wwan_hwsim_exit(), for it is theoretically possible to call
wwan_hwsim_debugfs_devcreate_write()/wwan_hwsim_debugfs_devdestroy_write()
by the moment wwan_hwsim_init_devs() returns.

Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
Changes in v2:
  Keep flush_workqueue(wwan_wq) explicit in order to match the comment.
  Make error path of wwan_hwsim_init() identical to wwan_hwsim_exit().

 drivers/net/wwan/wwan_hwsim.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c
index 5b62cf3b3c42..fad642f9ffd8 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))
-		return PTR_ERR(wwan_hwsim_class);
+	if (IS_ERR(wwan_hwsim_class)) {
+		err = PTR_ERR(wwan_hwsim_class);
+		goto err_wq_destroy;
+	}
 
 	wwan_hwsim_debugfs_topdir = debugfs_create_dir("wwan_hwsim", NULL);
 	wwan_hwsim_debugfs_devcreate =
@@ -523,9 +530,13 @@ static int __init wwan_hwsim_init(void)
 	return 0;
 
 err_clean_devs:
+	debugfs_remove(wwan_hwsim_debugfs_devcreate);	/* Avoid new devs */
 	wwan_hwsim_free_devs();
+	flush_workqueue(wwan_wq);	/* Wait deletion works completion */
 	debugfs_remove(wwan_hwsim_debugfs_topdir);
 	class_destroy(wwan_hwsim_class);
+err_wq_destroy:
+	destroy_workqueue(wwan_wq);
 
 	return err;
 }
@@ -534,9 +545,10 @@ 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 */
+	flush_workqueue(wwan_wq);	/* Wait deletion works completion */
 	debugfs_remove(wwan_hwsim_debugfs_topdir);
 	class_destroy(wwan_hwsim_class);
+	destroy_workqueue(wwan_wq);
 }
 
 module_init(wwan_hwsim_init);
-- 
2.32.0

Re: [PATCH v2] wwan_hwsim: Avoid flush_scheduled_work() usage

[Loic Poulain]

On Fri, 22 Apr 2022 at 05:01, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> Flushing system-wide workqueues is dangerous and will be forbidden.
> Replace system_wq with local wwan_wq.
>
> While we are at it, make err_clean_devs: label of wwan_hwsim_init()
> behave like wwan_hwsim_exit(), for it is theoretically possible to call
> wwan_hwsim_debugfs_devcreate_write()/wwan_hwsim_debugfs_devdestroy_write()
> by the moment wwan_hwsim_init_devs() returns.
>
> Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>

Reviewed-by: Loic Poulain <loic.poulain@linaro.org>


> ---
> Changes in v2:
>   Keep flush_workqueue(wwan_wq) explicit in order to match the comment.
>   Make error path of wwan_hwsim_init() identical to wwan_hwsim_exit().
>
>  drivers/net/wwan/wwan_hwsim.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c
> index 5b62cf3b3c42..fad642f9ffd8 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))
> -               return PTR_ERR(wwan_hwsim_class);
> +       if (IS_ERR(wwan_hwsim_class)) {
> +               err = PTR_ERR(wwan_hwsim_class);
> +               goto err_wq_destroy;
> +       }
>
>         wwan_hwsim_debugfs_topdir = debugfs_create_dir("wwan_hwsim", NULL);
>         wwan_hwsim_debugfs_devcreate =
> @@ -523,9 +530,13 @@ static int __init wwan_hwsim_init(void)
>         return 0;
>
>  err_clean_devs:
> +       debugfs_remove(wwan_hwsim_debugfs_devcreate);   /* Avoid new devs */
>         wwan_hwsim_free_devs();
> +       flush_workqueue(wwan_wq);       /* Wait deletion works completion */
>         debugfs_remove(wwan_hwsim_debugfs_topdir);
>         class_destroy(wwan_hwsim_class);
> +err_wq_destroy:
> +       destroy_workqueue(wwan_wq);
>
>         return err;
>  }
> @@ -534,9 +545,10 @@ 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 */
> +       flush_workqueue(wwan_wq);       /* Wait deletion works completion */
>         debugfs_remove(wwan_hwsim_debugfs_topdir);
>         class_destroy(wwan_hwsim_class);
> +       destroy_workqueue(wwan_wq);
>  }
>
>  module_init(wwan_hwsim_init);
> --
> 2.32.0
>

Re: [PATCH v2] wwan_hwsim: Avoid flush_scheduled_work() usage

[Jakub Kicinski]

On Fri, 22 Apr 2022 12:01:24 +0900 Tetsuo Handa wrote:
> Flushing system-wide workqueues is dangerous and will be forbidden.
> Replace system_wq with local wwan_wq.
> 
> While we are at it, make err_clean_devs: label of wwan_hwsim_init()
> behave like wwan_hwsim_exit(), for it is theoretically possible to call
> wwan_hwsim_debugfs_devcreate_write()/wwan_hwsim_debugfs_devdestroy_write()
> by the moment wwan_hwsim_init_devs() returns.
> 
> Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>

Commit cc271ab86606 ("wwan_hwsim: Avoid flush_scheduled_work() usage")
in net-next now, thanks!

Re: [PATCH v2] wwan_hwsim: Avoid flush_scheduled_work() usage

[Tetsuo Handa]

David, can you take this patch?

On 2022/04/22 15:38, Loic Poulain wrote:
> On Fri, 22 Apr 2022 at 05:01, Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>
>> Flushing system-wide workqueues is dangerous and will be forbidden.
>> Replace system_wq with local wwan_wq.
>>
>> While we are at it, make err_clean_devs: label of wwan_hwsim_init()
>> behave like wwan_hwsim_exit(), for it is theoretically possible to call
>> wwan_hwsim_debugfs_devcreate_write()/wwan_hwsim_debugfs_devdestroy_write()
>> by the moment wwan_hwsim_init_devs() returns.
>>
>> Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp
>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> 
> Reviewed-by: Loic Poulain <loic.poulain@linaro.org>
> 
> 
>> ---
>> Changes in v2:
>>   Keep flush_workqueue(wwan_wq) explicit in order to match the comment.
>>   Make error path of wwan_hwsim_init() identical to wwan_hwsim_exit().
>>
>>  drivers/net/wwan/wwan_hwsim.c | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c
>> index 5b62cf3b3c42..fad642f9ffd8 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))
>> -               return PTR_ERR(wwan_hwsim_class);
>> +       if (IS_ERR(wwan_hwsim_class)) {
>> +               err = PTR_ERR(wwan_hwsim_class);
>> +               goto err_wq_destroy;
>> +       }
>>
>>         wwan_hwsim_debugfs_topdir = debugfs_create_dir("wwan_hwsim", NULL);
>>         wwan_hwsim_debugfs_devcreate =
>> @@ -523,9 +530,13 @@ static int __init wwan_hwsim_init(void)
>>         return 0;
>>
>>  err_clean_devs:
>> +       debugfs_remove(wwan_hwsim_debugfs_devcreate);   /* Avoid new devs */
>>         wwan_hwsim_free_devs();
>> +       flush_workqueue(wwan_wq);       /* Wait deletion works completion */
>>         debugfs_remove(wwan_hwsim_debugfs_topdir);
>>         class_destroy(wwan_hwsim_class);
>> +err_wq_destroy:
>> +       destroy_workqueue(wwan_wq);
>>
>>         return err;
>>  }
>> @@ -534,9 +545,10 @@ 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 */
>> +       flush_workqueue(wwan_wq);       /* Wait deletion works completion */
>>         debugfs_remove(wwan_hwsim_debugfs_topdir);
>>         class_destroy(wwan_hwsim_class);
>> +       destroy_workqueue(wwan_wq);
>>  }
>>
>>  module_init(wwan_hwsim_init);
>> --
>> 2.32.0
>>