[PATCH] workqueue: Try to catch flush_work() without INIT_WORK().

[PATCH] workqueue: Try to catch flush_work() without INIT_WORK().

[Tetsuo Handa]

syzbot found a flush_work() caller who forgot to call INIT_WORK()
because that work_struct was allocated by kzalloc(). But the message

  INFO: trying to register non-static key.
  the code is fine but needs lockdep annotation.
  turning off the locking correctness validator.

by lock_map_acquire() is failing to tell that INIT_WORK() is missing.

Since flush_work() without INIT_WORK() is a bug, and INIT_WORK() should
set ->func field to non-zero, let's warn if ->func field is zero.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 kernel/workqueue.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 392be4b..a503ad9 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2908,6 +2908,9 @@ static bool __flush_work(struct work_struct *work, bool from_cancel)
 	if (WARN_ON(!wq_online))
 		return false;
 
+	if (WARN_ON(!work->func))
+		return false;
+
 	if (!from_cancel) {
 		lock_map_acquire(&work->lockdep_map);
 		lock_map_release(&work->lockdep_map);
-- 
1.8.3.1

Re: [PATCH] workqueue: Try to catch flush_work() without INIT_WORK().

[Daniel Jordan]

On Sat, Jan 19, 2019 at 02:04:58AM +0900, Tetsuo Handa wrote:
> syzbot found a flush_work() caller who forgot to call INIT_WORK()
> because that work_struct was allocated by kzalloc(). But the message
> 
>   INFO: trying to register non-static key.
>   the code is fine but needs lockdep annotation.
>   turning off the locking correctness validator.
> 
> by lock_map_acquire() is failing to tell that INIT_WORK() is missing.
> 
> Since flush_work() without INIT_WORK() is a bug, and INIT_WORK() should
> set ->func field to non-zero, let's warn if ->func field is zero.

Agree that it's a good idea to catch this.  So the caller did flush_work
without queueing it beforehand?  Out of curiosity, what situation leads to
this?  Link to the report might be helpful.

> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 392be4b..a503ad9 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2908,6 +2908,9 @@ static bool __flush_work(struct work_struct *work, bool from_cancel)
>  	if (WARN_ON(!wq_online))
>  		return false;
>  
> +	if (WARN_ON(!work->func))
> +		return false;
> +

__queue_work has a sanity check already for work, but using list_empty.  Seems
slightly better to be consistent?

Re: [PATCH] workqueue: Try to catch flush_work() without INIT_WORK().

[Tetsuo Handa]

On 2019/01/19 4:48, Daniel Jordan wrote:
> On Sat, Jan 19, 2019 at 02:04:58AM +0900, Tetsuo Handa wrote:
>> syzbot found a flush_work() caller who forgot to call INIT_WORK()
>> because that work_struct was allocated by kzalloc(). But the message
>>
>>   INFO: trying to register non-static key.
>>   the code is fine but needs lockdep annotation.
>>   turning off the locking correctness validator.
>>
>> by lock_map_acquire() is failing to tell that INIT_WORK() is missing.
>>
>> Since flush_work() without INIT_WORK() is a bug, and INIT_WORK() should
>> set ->func field to non-zero, let's warn if ->func field is zero.
> 
> Agree that it's a good idea to catch this.  So the caller did flush_work
> without queueing it beforehand?  Out of curiosity, what situation leads to
> this?  Link to the report might be helpful.

I quoted the patch below.

> 
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index 392be4b..a503ad9 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -2908,6 +2908,9 @@ static bool __flush_work(struct work_struct *work, bool from_cancel)
>>  	if (WARN_ON(!wq_online))
>>  		return false;
>>  
>> +	if (WARN_ON(!work->func))
>> +		return false;
>> +
> 
> __queue_work has a sanity check already for work, but using list_empty.  Seems
> slightly better to be consistent?
> 

list_empty() won't work, for "struct work_struct" is embedded into a struct
which is allocated by kzalloc().



From 1bbf8d9c7aaef78d7f483d2131261162485c6f72 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 19 Jan 2019 01:38:53 +0900
Subject: [PATCH] drm/vkms: Fix flush_work() without INIT_WORK().

syzbot is hitting a lockdep warning [1] because flush_work() is called
without INIT_WORK() after kzalloc() at vkms_atomic_crtc_reset().

Commit 6c234fe37c57627a ("drm/vkms: Implement CRC debugfs API") added
INIT_WORK() to only vkms_atomic_crtc_duplicate_state() side. Assuming
that lifecycle of crc_work is appropriately managed, fix this problem
by adding INIT_WORK() to vkms_atomic_crtc_reset() side.

[1] https://syzkaller.appspot.com/bug?id=a5954455fcfa51c29ca2ab55b203076337e1c770

Reported-and-tested-by: syzbot <syzbot+12f1b031b6da017e34f8@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 177bbcb..3c37d8c 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -104,6 +104,7 @@ static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
 	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
 	if (!vkms_state)
 		return;
+	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
 
 	crtc->state = &vkms_state->base;
 	crtc->state->crtc = crtc;
-- 
1.8.3.1

Re: [PATCH] workqueue: Try to catch flush_work() without INIT_WORK().

[Daniel Jordan]

On Sat, Jan 19, 2019 at 11:41:22AM +0900, Tetsuo Handa wrote:
> On 2019/01/19 4:48, Daniel Jordan wrote:
> > On Sat, Jan 19, 2019 at 02:04:58AM +0900, Tetsuo Handa wrote:
> > __queue_work has a sanity check already for work, but using list_empty.  Seems
> > slightly better to be consistent?
> > 
> 
> list_empty() won't work, for "struct work_struct" is embedded into a struct
> which is allocated by kzalloc().

Please check list_empty's definition again, it compares the address of the node
to its next pointer, so it should work for a zeroed node.  I'll reiterate that
it seems slightly better to be consistent in "is work_struct initialized?"
checks, but it's not a big deal and I'm fine either way.

> From 1bbf8d9c7aaef78d7f483d2131261162485c6f72 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 19 Jan 2019 01:38:53 +0900
> Subject: [PATCH] drm/vkms: Fix flush_work() without INIT_WORK().
> 
> syzbot is hitting a lockdep warning [1] because flush_work() is called
> without INIT_WORK() after kzalloc() at vkms_atomic_crtc_reset().
> 
> Commit 6c234fe37c57627a ("drm/vkms: Implement CRC debugfs API") added
> INIT_WORK() to only vkms_atomic_crtc_duplicate_state() side. Assuming
> that lifecycle of crc_work is appropriately managed, fix this problem
> by adding INIT_WORK() to vkms_atomic_crtc_reset() side.
> 
> [1] https://syzkaller.appspot.com/bug?id=a5954455fcfa51c29ca2ab55b203076337e1c770
> 
> Reported-and-tested-by: syzbot <syzbot+12f1b031b6da017e34f8@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 177bbcb..3c37d8c 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -104,6 +104,7 @@ static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
>  	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
>  	if (!vkms_state)
>  		return;
> +	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
>  
>  	crtc->state = &vkms_state->base;
>  	crtc->state->crtc = crtc;

Great, thanks, I think mentioning this path would improve the changelog.

Re: [PATCH] workqueue: Try to catch flush_work() without INIT_WORK().

[Tetsuo Handa]

Daniel Jordan wrote:
> On Sat, Jan 19, 2019 at 11:41:22AM +0900, Tetsuo Handa wrote:
> > On 2019/01/19 4:48, Daniel Jordan wrote:
> > > On Sat, Jan 19, 2019 at 02:04:58AM +0900, Tetsuo Handa wrote:
> > > __queue_work has a sanity check already for work, but using list_empty.  Seems
> > > slightly better to be consistent?
> > > 
> > 
> > list_empty() won't work, for "struct work_struct" is embedded into a struct
> > which is allocated by kzalloc().
> 
> Please check list_empty's definition again, it compares the address of the node
> to its next pointer, so it should work for a zeroed node.  I'll reiterate that
> it seems slightly better to be consistent in "is work_struct initialized?"
> checks, but it's not a big deal and I'm fine either way.

You are talking about

	if (WARN_ON(!list_empty(&work->entry))) {
		spin_unlock(&pwq->pool->lock);
		return;
	}

part in __queue_work(), aren't you? But since flush_work() is used for waiting for
a work to complete, that work can be either queued state (list_empty() == false) or
not queued state (list_empty() == true). Thus, I don't think that flush_work() can
use list_empty() for checking whether that work was initialized.



[PATCH v2] workqueue: Try to catch flush_work() without INIT_WORK().

syzbot found a flush_work() caller who forgot to call INIT_WORK()
because that work_struct was allocated by kzalloc() [1]. But the message

  INFO: trying to register non-static key.
  the code is fine but needs lockdep annotation.
  turning off the locking correctness validator.

by lock_map_acquire() is failing to tell that INIT_WORK() is missing.

Since flush_work() without INIT_WORK() is a bug, and INIT_WORK() should
set ->func field to non-zero, let's warn if ->func field is zero.

[1] https://syzkaller.appspot.com/bug?id=a5954455fcfa51c29ca2ab55b203076337e1c770

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 kernel/workqueue.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 392be4b..a503ad9 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2908,6 +2908,9 @@ static bool __flush_work(struct work_struct *work, bool from_cancel)
 	if (WARN_ON(!wq_online))
 		return false;
 
+	if (WARN_ON(!work->func))
+		return false;
+
 	if (!from_cancel) {
 		lock_map_acquire(&work->lockdep_map);
 		lock_map_release(&work->lockdep_map);

Re: [PATCH] workqueue: Try to catch flush_work() without INIT_WORK().

[Daniel Jordan]

On Wed, Jan 23, 2019 at 09:44:12AM +0900, Tetsuo Handa wrote:
> Daniel Jordan wrote:
> > On Sat, Jan 19, 2019 at 11:41:22AM +0900, Tetsuo Handa wrote:
> > > On 2019/01/19 4:48, Daniel Jordan wrote:
> > > > On Sat, Jan 19, 2019 at 02:04:58AM +0900, Tetsuo Handa wrote:
> > > > __queue_work has a sanity check already for work, but using list_empty.  Seems
> > > > slightly better to be consistent?
> > > > 
> > > 
> > > list_empty() won't work, for "struct work_struct" is embedded into a struct
> > > which is allocated by kzalloc().
> > 
> > Please check list_empty's definition again, it compares the address of the node
> > to its next pointer, so it should work for a zeroed node.  I'll reiterate that
> > it seems slightly better to be consistent in "is work_struct initialized?"
> > checks, but it's not a big deal and I'm fine either way.
> 
> You are talking about
> 
> 	if (WARN_ON(!list_empty(&work->entry))) {
> 		spin_unlock(&pwq->pool->lock);
> 		return;
> 	}
> 
> part in __queue_work(), aren't you? But since flush_work() is used for waiting for
> a work to complete, that work can be either queued state (list_empty() == false) or
> not queued state (list_empty() == true). Thus, I don't think that flush_work() can
> use list_empty() for checking whether that work was initialized.

Oh, you're right, sorry for the noise!

> [PATCH v2] workqueue: Try to catch flush_work() without INIT_WORK().
> 
> syzbot found a flush_work() caller who forgot to call INIT_WORK()
> because that work_struct was allocated by kzalloc() [1]. But the message
> 
>   INFO: trying to register non-static key.
>   the code is fine but needs lockdep annotation.
>   turning off the locking correctness validator.
> 
> by lock_map_acquire() is failing to tell that INIT_WORK() is missing.
> 
> Since flush_work() without INIT_WORK() is a bug, and INIT_WORK() should
> set ->func field to non-zero, let's warn if ->func field is zero.
> 
> [1] https://syzkaller.appspot.com/bug?id=a5954455fcfa51c29ca2ab55b203076337e1c770
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  kernel/workqueue.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 392be4b..a503ad9 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2908,6 +2908,9 @@ static bool __flush_work(struct work_struct *work, bool from_cancel)
>  	if (WARN_ON(!wq_online))
>  		return false;
>  
> +	if (WARN_ON(!work->func))
> +		return false;
> +
>  	if (!from_cancel) {
>  		lock_map_acquire(&work->lockdep_map);
>  		lock_map_release(&work->lockdep_map);

Thanks for updating the changelog.  FWIW, you can add

Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>

Re: [PATCH] workqueue: Try to catch flush_work() without INIT_WORK().

[Tejun Heo]

On Wed, Jan 23, 2019 at 09:44:12AM +0900, Tetsuo Handa wrote:
> Daniel Jordan wrote:
> > On Sat, Jan 19, 2019 at 11:41:22AM +0900, Tetsuo Handa wrote:
> > > On 2019/01/19 4:48, Daniel Jordan wrote:
> > > > On Sat, Jan 19, 2019 at 02:04:58AM +0900, Tetsuo Handa wrote:
> > > > __queue_work has a sanity check already for work, but using list_empty.  Seems
> > > > slightly better to be consistent?
> > > > 
> > > 
> > > list_empty() won't work, for "struct work_struct" is embedded into a struct
> > > which is allocated by kzalloc().
> > 
> > Please check list_empty's definition again, it compares the address of the node
> > to its next pointer, so it should work for a zeroed node.  I'll reiterate that
> > it seems slightly better to be consistent in "is work_struct initialized?"
> > checks, but it's not a big deal and I'm fine either way.
> 
> You are talking about
> 
> 	if (WARN_ON(!list_empty(&work->entry))) {
> 		spin_unlock(&pwq->pool->lock);
> 		return;
> 	}
> 
> part in __queue_work(), aren't you? But since flush_work() is used for waiting for
> a work to complete, that work can be either queued state (list_empty() == false) or
> not queued state (list_empty() == true). Thus, I don't think that flush_work() can
> use list_empty() for checking whether that work was initialized.
> 
> 
> 
> [PATCH v2] workqueue: Try to catch flush_work() without INIT_WORK().
> 
> syzbot found a flush_work() caller who forgot to call INIT_WORK()
> because that work_struct was allocated by kzalloc() [1]. But the message
> 
>   INFO: trying to register non-static key.
>   the code is fine but needs lockdep annotation.
>   turning off the locking correctness validator.
> 
> by lock_map_acquire() is failing to tell that INIT_WORK() is missing.
> 
> Since flush_work() without INIT_WORK() is a bug, and INIT_WORK() should
> set ->func field to non-zero, let's warn if ->func field is zero.
> 
> [1] https://syzkaller.appspot.com/bug?id=a5954455fcfa51c29ca2ab55b203076337e1c770
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Applied to wq/for-5.0.

Thanks.

-- 
tejun