null pointer risk of kernel workqueue

Hi Tejun & Jiangshan,

I find an null pointer risk in the code of workqueue. Here is description:

If draining, __queue_work() will call the function is_chained_work() to do some checks. In is_chained_work(), worker->current_pwq is used directly. It should be not safe.
http://elixir.free-electrons.com/linux/latest/source/kernel/workqueue.c#L1384

If you check the thread function of this worker, worker_thread(), you will find worker->current_pwq is null when one work is done or ready to be processed.
This issue may happen only if we queue work during executing drain_workqueue().
http://elixir.free-electrons.com/linux/latest/source/kernel/workqueue.c#L2173

Hello,

Quoted 8 lines · tanxiaofei

Hi Tejun & Jiangshan,

I find an null pointer risk in the code of workqueue. Here is description:

If draining, __queue_work() will call the function is_chained_work() to do some checks. In is_chained_work(), worker->current_pwq is used directly. It should be not safe.
http://elixir.free-electrons.com/linux/latest/source/kernel/workqueue.c#L1384

Quoted 4 lines

If you check the thread function of this worker, worker_thread(), you will find worker->current_pwq is null when one work is done or ready to be processed.
This issue may happen only if we queue work during executing drain_workqueue().
http://elixir.free-electrons.com/linux/latest/source/kernel/workqueue.c#L2173

Hmmm? I don't get it. worker->current_pwq is guaranteed to be set while a work function is being executed and the chained check can only get there iff the the worker is executing a work function.

Quoted 11 lines · Tejun Heo

Hello,

Quoted 8 lines · tanxiaofei

Hi Tejun & Jiangshan,

I find an null pointer risk in the code of workqueue. Here is description:

If draining, __queue_work() will call the function is_chained_work() to do some checks. In is_chained_work(), worker->current_pwq is used directly. It should be not safe.
http://elixir.free-electrons.com/linux/latest/source/kernel/workqueue.c#L1384

Quoted 9 lines

Quoted 4 lines

If you check the thread function of this worker, worker_thread(), you will find worker->current_pwq is null when one work is done or ready to be processed.
This issue may happen only if we queue work during executing drain_workqueue().
http://elixir.free-electrons.com/linux/latest/source/kernel/workqueue.c#L2173

Hmmm? I don't get it. worker->current_pwq is guaranteed to be set while a work function is being executed and the chained check can only get there iff the the worker is executing a work function.

Hi Tejun,

Thanks for your quick reply.
Hmm, but i think interrupt preemption could make this happen.
For example, the scenario that all following conditions are met.
1.The handler of an interrupt call queue_work() to queue some works, and the workqueue is draining. 2.An worker thread is interrupted by this interrupt.
3.The worker thread is being executed, and at the very moment,worker->current_pwq is set to null.(This is possible and please check function worker_thread() carefully).

Thanks.

.

Hi Tejun,

Any comments about this?

Quoted 3 lines · tanxiaofei

Hi Tejun,

Any comments about this?

I think you're confused, or at least can't understand what you're trying to say. Can you create a rero?

Quoted 3 lines · tanxiaofei

Hi Tejun,

Any comments about this?

I think you're confused, or at least can't understand what you're trying to say. Can you create a rero?

Thanks.

Quoted 8 lines · Tejun Heo

Quoted 3 lines · tanxiaofei

Hi Tejun,

Any comments about this?

I think you're confused, or at least can't understand what you're trying to say. Can you create a rero?

Hi Tejun,
The case is as following:

worker_thread()
|-spin_lock_irq()
|-process_one_work()

    |-worker->current_pwq = pwq
    |-spin_unlock_irq()
    |-worker->current_func(work)
    |-spin_lock_irq()
    |-worker->current_pwq = NULL
|-spin_unlock_irq()
                                                                    //interrupt here
                                                                    |-irq_handler
                                                                        |-__queue_work()
                                                                            //assuming that the wq is draining
                                                                            |-if (unlikely(wq->flags & __WQ_DRAINING) &&WARN_ON_ONCE(!is_chained_work(wq)))
                                                                                |-is_chained_work(wq)
                                                                                    |-current_wq_worker() // Here, 'current' is the interrupted worker!
                                                                                        |-current->current_pwq is NULL here!

|-schedule()

And I think the following patch can solve the bug, right?

kernel/workqueue_internal.h +1 -1

index 8635417..650680c 100644
@@ -59,7 +59,7 @@ struct worker {
  */
 static inline struct worker *current_wq_worker(void)
 {
-       if (current->flags & PF_WQ_WORKER)
+       if (!in_irq() && (current->flags & PF_WQ_WORKER))
                return kthread_data(current);
        return NULL;
 }

Thanks,
Li Bin

Quoted 2 lines

Thanks.

.

Quoted 8 lines · Tejun Heo

Quoted 3 lines · tanxiaofei

Hi Tejun,

Any comments about this?

I think you're confused, or at least can't understand what you're trying to say. Can you create a rero?

Hi Tejun,
The case is as following:

worker_thread()
|-spin_lock_irq()
|-process_one_work()

    |-worker->current_pwq = pwq
    |-spin_unlock_irq()
    |-worker->current_func(work)
    |-spin_lock_irq()
    |-worker->current_pwq = NULL
|-spin_unlock_irq()
                                                                    //interrupt here
                                                                    |-irq_handler
                                                                        |-__queue_work()
                                                                            //assuming that the wq is draining
                                                                            |-if (unlikely(wq->flags & __WQ_DRAINING) &&WARN_ON_ONCE(!is_chained_work(wq)))
                                                                                |-is_chained_work(wq)
                                                                                    |-current_wq_worker() // Here, 'current' is the interrupted worker!
                                                                                        |-current->current_pwq is NULL here!

|-schedule()

And I think the following patch can solve the bug, right?

kernel/workqueue_internal.h +0 -0

index 8635417..650680c 100644

Quoted 8 lines

And I think the following patch can solve the bug, right?

kernel/workqueue_internal.h +0 -0

index 8635417..650680c 100644

kernel/workqueue_internal.h struct worker { · line 65

Quoted 8 lines

kernel/workqueue_internal.h +1 -1

@@ -59,7 +59,7 @@ struct worker {
  */
 static inline struct worker *current_wq_worker(void)
 {
-       if (current->flags & PF_WQ_WORKER)
+       if (!in_irq() && (current->flags & PF_WQ_WORKER))
                return kthread_data(current);
        return NULL;
 }

Yeah, that makes sense to me. Can you please resend the patch with patch description and SOB?

Thanks.