Re: [PATCH net] amt: use cancel_delayed_work() instead of flush_delayed_work() in amt_fini()

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