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] [day] [month] [year] [list]

Message-ID: <29d14fb5-8eb4-bd83-65af-7c51a9345a66@I-love.SAKURA.ne.jp>
Date:   Fri, 22 Apr 2022 12:02:55 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:     Sergey Ryazanov <ryazanov.s.a@...il.com>,
        Loic Poulain <loic.poulain@...aro.org>
Cc:     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 2022/04/22 1:14, Sergey Ryazanov wrote:
>> 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.

OK. Since manual creation of emulated device via debugfs followed by
manual device deletion of emulated device via debugfs is possible before
automatic creation of emulated device via wwan_hwsim_init_devs() fails,
"/* Avoid new devs */" comment is applicable to this error path; I will
include debugfs_remove(wwan_hwsim_debugfs_devcreate) call.

On 2022/04/22 1:35, Sergey Ryazanov 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);
> 
> How about jumping to some label from here and do the workqueue
> destroying there? E.g.

OK.

>> @@ -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.

I will add

	debugfs_remove(wwan_hwsim_debugfs_devcreate);   /* Avoid new devs */

here, for "it was not used" part is theoretically not always true.

>> @@ -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?

OK.

On 2022/04/22 1:54, Sergey Ryazanov wrote:
> From what I understand, an inaccurate flushing of the system work
> queue can potentially cause a system freeze. That is why
> flush_scheduled_work() is planned to be removed. The hwsim module is
> just a random function user without any known issues. So, a 'fixes'
> tag is not required here, and there is no need to bother the stable
> team with a change backport.

Right, 'Fixes:' tag is not needed for this patch.

Flushing the system-wide workqueue is problematic under e.g. GFP_NOFS/GFP_NOIO context.
Removing flush_scheduled_work() is for proactively avoiding new problems like
https://lkml.kernel.org/r/385ce718-f965-4005-56b6-34922c4533b8@I-love.SAKURA.ne.jp
and https://lkml.kernel.org/r/20220225112405.355599-10-Jerome.Pouiller@silabs.com .

> 
> Anyway, Tetsuo, you missed a target tree in the subject. If this is
> not a fix, then you probably should target your changes to the
> 'net-next' tree.
> 

OK. I posted v2 patch at
https://lkml.kernel.org/r/7390d51f-60e2-3cee-5277-b819a55ceabe@I-love.SAKURA.ne.jp .


Thank you for responding.