[Question] null pointer risk of kernel workqueue

[Question] null pointer risk of kernel workqueue

[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

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

There are very few places to call drain_workqueue() in the whole linux kernel.
I think that's why no one noticed this risk.

Xiaofei Tan
_______________________________________________
linuxarm mailing list
linuxarm@huawei.com
http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm

.

Re: [Question] null pointer risk of kernel workqueue

[Tejun Heo]

Hello,

On Sat, Sep 23, 2017 at 05:04:24PM +0800, tanxiaofei wrote:
> 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

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.

Thanks.

-- 
tejun

Re: [Question] null pointer risk of kernel workqueue

[tanxiaofei]

On 2017/9/25 23:25, Tejun Heo wrote:
> Hello,
> 
> On Sat, Sep 23, 2017 at 05:04:24PM +0800, tanxiaofei wrote:
>> 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
> 
> 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.

Re: [Question] null pointer risk of kernel workqueue

[tanxiaofei]

Hi Tejun,

Any comments about this?

Thanks

On 2017/9/27 17:17, tanxiaofei wrote:
> 
> 
> On 2017/9/25 23:25, Tejun Heo wrote:
>> Hello,
>>
>> On Sat, Sep 23, 2017 at 05:04:24PM +0800, tanxiaofei wrote:
>>> 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
>>
>> 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.
> 
> 
> 
> 
> 
> .
> 

Re: [Question] null pointer risk of kernel workqueue

[Tejun Heo]

On Fri, Oct 20, 2017 at 02:57:18PM +0800, tanxiaofei wrote:
> 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.

-- 
tejun

Re: [Question] null pointer risk of kernel workqueue

[Tejun Heo]

On Sat, Oct 21, 2017 at 08:35:22AM -0700, Tejun Heo wrote:
> On Fri, Oct 20, 2017 at 02:57:18PM +0800, tanxiaofei wrote:
> > 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?

Sorry, I meant to write repro - a reproduction case.

-- 
tejun

Re: [Question] null pointer risk of kernel workqueue

[Li Bin]

on 2017/10/21 23:35, Tejun Heo wrote:
> On Fri, Oct 20, 2017 at 02:57:18PM +0800, tanxiaofei wrote:
>> 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?

diff --git a/kernel/workqueue_internal.h b/kernel/workqueue_internal.h
index 8635417..650680c 100644
--- a/kernel/workqueue_internal.h
+++ b/kernel/workqueue_internal.h
@@ -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

> Thanks.
> 

Re: [Question] null pointer risk of kernel workqueue

[tanxiaofei]

Hi Bin,

Yes, that's it. thanks.

--
tanxiaofei

On 2017/10/23 9:34, Li Bin wrote:
> 
> 
> on 2017/10/21 23:35, Tejun Heo wrote:
>> On Fri, Oct 20, 2017 at 02:57:18PM +0800, tanxiaofei wrote:
>>> 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?
> 
> diff --git a/kernel/workqueue_internal.h b/kernel/workqueue_internal.h
> index 8635417..650680c 100644
> --- a/kernel/workqueue_internal.h
> +++ b/kernel/workqueue_internal.h
> @@ -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
> 
>> Thanks.
>>
> 
> 
> .
> 

Re: [Question] null pointer risk of kernel workqueue

[Tejun Heo]

Hello,

On Mon, Oct 23, 2017 at 09:34:11AM +0800, Li Bin wrote:
> 
> 
> on 2017/10/21 23:35, Tejun Heo wrote:
> > On Fri, Oct 20, 2017 at 02:57:18PM +0800, tanxiaofei wrote:
> >> 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?
> 
> diff --git a/kernel/workqueue_internal.h b/kernel/workqueue_internal.h
> index 8635417..650680c 100644
> --- a/kernel/workqueue_internal.h
> +++ b/kernel/workqueue_internal.h
> @@ -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.

-- 
tejun

Re: [Question] null pointer risk of kernel workqueue

[Li Bin]

on 2017/10/23 22:03, Tejun Heo wrote:
>>
>> And I think the following patch can solve the bug, right?
>>
>> diff --git a/kernel/workqueue_internal.h b/kernel/workqueue_internal.h
>> index 8635417..650680c 100644
>> --- a/kernel/workqueue_internal.h
>> +++ b/kernel/workqueue_internal.h
>> @@ -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?

Ok, I will resend the patch soon.

Thanks,
Li Bin

> 
> Thanks.
>