linux-kernel - Re: work item still be scheduled to execute after destroy

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:   Tue, 6 Dec 2022 12:13:18 +0800
From:   Lai Jiangshan <jiangshanlai@...il.com>
To:     richard clark <richard.xnu.clark@...il.com>
Cc:     tj@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: work item still be scheduled to execute after destroy_workqueue?

On Mon, Dec 5, 2022 at 2:18 PM richard clark
<richard.xnu.clark@...il.com> wrote:
>
> Hi Lai and Tejun,
>
> Why can the work still be queued to and executed when queueing it to a
> wq has been destroyed, for instance, the below code snippet in a
> kernel module:
> ---------------------->8---------------------
>
> struct workqueue_struct *wq0;
> #define MAX_ACTIVE_WORKS        (3)
>
> static void work0_func(struct work_struct *work);
> static void work1_func(struct work_struct *work);
> static void work2_func(struct work_struct *work);
>
> static DECLARE_WORK(w0, work0_func);
> static DECLARE_WORK(w1, work1_func);
> static DECLARE_WORK(w2, work2_func);
>
> /* work->func */
> static void work0_func(struct work_struct *work)
> {
>         pr_info("+%s begins to sleep\n", __func__);
>         /* sleep for 10s */
>         schedule_timeout_interruptible(msecs_to_jiffies(10000));
>         pr_info("+%s after sleep, begin to queue another work\n", __func__);
>         queue_work_on(1, wq0, &w1);
> }
>
> /* work->func */
> static void work1_func(struct work_struct *work)
> {
>         pr_info("+%s scheduled\n", __func__);
> }
>
> /* work->func */
> static void work2_func(struct work_struct *work)
> {
>         pr_info("+%s scheduled\n", __func__);
> }
>
> static int destroy_init(void)
> {
>         wq0 = alloc_workqueue("percpu_wq0", 0, MAX_ACTIVE_WORKS);
>         if (!wq0) {
>                 pr_err("alloc_workqueue failed\n");
>                 return -1;
>         }
>         queue_work_on(1, wq0, &w0);
>         pr_info("Begin to destroy wq0...\n");
>         destroy_workqueue(wq0);
>         pr_info("queue w2 to the wq0 after destroyed...\n");
>         queue_work_on(1, wq0, &w2);

Hello, Richard.

Nice spot.

It is illegal to use a destroyed structure in the view of any API.

A destroyed workqueue might be directly freed or kept for a while,
which is up to the code of workqueue.c

Before e2dca7adff8f(workqueue: make the workqueues list RCU walkable),
the workqueue is directly totally freed when destroyed.
After the said commit, the workqueue is held for an RCU grace before
totally freed.  And it is a per-cpu workqueue, and the base ref is
never dropped on per-cpu pwqs, which means it is referencable and
able to be queued items during the period by accident.

Albeit it is illegal to use a destroyed workqueue, it is definitely bad
for workqueue code not to complain noisily about the behavior, so I am
going to set __WQ_DRAINING permanently for the destroyed workqueue, so
the illegal usage of the destroyed workqueue can result WARN().

Thank you for the report.
Lai

>
>         return 0;
> }
>
> The output on my x86_64 box is:
>
> [344702.734480] +destroy_init+
> [344702.734499] Begin to destroy wq0...
> [344702.734516] +work0_func begins to sleep
> [344712.791607] +work0_func after sleep, begin to queue another work
> [344712.791620] +work1_func scheduled
> [344712.791649] queue w2 to the wq0 after destroyed...
> [344712.791663] +work2_func scheduled  <------------- work 2 still be scheduled?