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 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:14:40 +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

Hello Tetsuo,

On Wed, Apr 20, 2022 at 1:17 PM Tetsuo Handa
<penguin-kernel@...ove.sakura.ne.jp> wrote:
> On 2022/04/20 18:53, Loic Poulain wrote:
>>> @@ -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);
>>>                 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:
>
> Do you want
>
>         debugfs_remove(wwan_hwsim_debugfs_devcreate);
>
> here (as a separate patch)?

Nope. But I will not be against such a patch. I remove the "devcreate"
file in wwwan_hwsim_exit() to prevent new emulated device creation
while the workqueue flushing, which can take a sufficient time. Here
we cleanup the leftovers of the attempt to automatically create
emulated devices. Here is no workqueue flushing, so the race window is
very tight.

In other words, the preparatory debugfs file removal is practically
not required here, but it will not hurt anyone. And possibly will make
the code less questionable.

>>>         wwan_hwsim_free_devs();
>>> +       destroy_workqueue(wwan_wq);
>>>         debugfs_remove(wwan_hwsim_debugfs_topdir);
>>>         class_destroy(wwan_hwsim_class);
>>>
>>> @@ -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 */
>>
>> Wouldn't it be simpler to just remove the flush call. It Looks like
>> all ports have been removed at that point, and all works cancelled,
>> right?
>
> I guess that this flush_scheduled_work() is for waiting for schedule_work(&dev->del_work) from
> wwan_hwsim_debugfs_devdestroy_write(). That is, if wwan_hwsim_debugfs_devdestroy_write() already
> scheduled this work, wwan_hwsim_dev_del() from wwan_hwsim_dev_del_work() might be still in progress
> even after wwan_hwsim_dev_del() from wwan_hwsim_free_devs() from wwan_hwsim_exit() returned.

Exactly. This code will wait for the completion of the work that was
scheduled somewhere else.

-- 
Sergey