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
| ||
|
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
Powered by blists - more mailing lists