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

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

Date:   Thu, 11 Nov 2021 07:37:48 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Taehee Yoo <ap420073@...il.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net] amt: use cancel_delayed_work() instead of
 flush_delayed_work() in amt_fini()

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?

>  	__amt_source_gc_work();
>  	destroy_workqueue(amt_wq);
>  }