Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski [off-list ref]:
On Mon, 8 Nov 2021 14:53:40 +0000 you wrote:
When the amt module is being removed, it calls flush_delayed_work() to exit
source_gc_wq. But it wouldn't be exited properly because the
amt_source_gc_work(), which is the callback function of source_gc_wq
internally calls mod_delayed_work() again.
So, amt_source_gc_work() would be called after the amt module is removed.
Therefore kernel panic would occur.
In order to avoid it, cancel_delayed_work() should be used instead of
flush_delayed_work().
[...]
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-11-11 15:37:52
On Mon, 8 Nov 2021 14:53:40 +0000 Taehee Yoo wrote:
When the amt module is being removed, it calls flush_delayed_work() to exit
source_gc_wq. But it wouldn't be exited properly because the
amt_source_gc_work(), which is the callback function of source_gc_wq
internally calls mod_delayed_work() again.
So, amt_source_gc_work() would be called after the amt module is removed.
Therefore kernel panic would occur.
In order to avoid it, cancel_delayed_work() should be used instead of
flush_delayed_work().
Somehow I convinced myself this is correct but now I'm not sure, again.
This doesn't guarantee that the work is not running _right_ now and
will re-arm itself on the next clock cycle, so to speak.
CPU 0 CPU 1
----- -----
worker gets the work
clears pending
work starts running
cancel_work
grabs pending
clears pending
mod_delayed_work()
You need cancel_delayed_work_sync(), right?
Hi Jakub,
Thank you for your review!
On 11/12/21 12:37 AM, Jakub Kicinski wrote:
> On Mon, 8 Nov 2021 14:53:40 +0000 Taehee Yoo wrote:
>> When the amt module is being removed, it calls flush_delayed_work()
to exit
>> source_gc_wq. But it wouldn't be exited properly because the
>> amt_source_gc_work(), which is the callback function of source_gc_wq
>> internally calls mod_delayed_work() again.
>> So, amt_source_gc_work() would be called after the amt module is
removed.
>> Therefore kernel panic would occur.
>> In order to avoid it, cancel_delayed_work() should be used instead of
>> flush_delayed_work().
>
> Somehow I convinced myself this is correct but now I'm not sure, again.
>
>> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
>> index c384b2694f9e..47a04c330885 100644
>> --- a/drivers/net/amt.c
>> +++ b/drivers/net/amt.c
>> @@ -3286,7 +3286,7 @@ static void __exit amt_fini(void)
>> {
>> rtnl_link_unregister(&amt_link_ops);
>> unregister_netdevice_notifier(&amt_notifier_block);
>> - flush_delayed_work(&source_gc_wq);
>> + cancel_delayed_work(&source_gc_wq);
>
> This doesn't guarantee that the work is not running _right_ now and
> will re-arm itself on the next clock cycle, so to speak.
>
> CPU 0 CPU 1
> ----- -----
>
> worker gets the work
> clears pending
> work starts running
> cancel_work
> grabs pending
> clears pending
> mod_delayed_work()
>
> You need cancel_delayed_work_sync(), right?
>
you're right!
I think cancel_delayed_work() is async so that it can't clearly fix this
problem.
So, I will send a new patch after some tests.
Thank you so much for catching it!
Thanks a lot,
Taehee