From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Daniel Jordan <daniel.m.jordan@oracle.com>,
Tejun Heo <tj@kernel.org>, Lai Jiangshan <jiangshanlai@gmail.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] workqueue: Try to catch flush_work() without INIT_WORK().
Date: Wed, 23 Jan 2019 09:44:12 +0900 [thread overview]
Message-ID: <201901230044.x0N0iCdk051613@www262.sakura.ne.jp> (raw)
In-Reply-To: <20190122151819.qjzihyyd4h2ukou2@ca-dmjordan1.us.oracle.com>
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);
next prev parent reply other threads:[~2019-01-23 0:44 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-18 17:04 [PATCH] workqueue: Try to catch flush_work() without INIT_WORK() Tetsuo Handa
2019-01-18 19:48 ` Daniel Jordan
2019-01-19 2:41 ` Tetsuo Handa
2019-01-22 15:18 ` Daniel Jordan
2019-01-23 0:44 ` Tetsuo Handa [this message]
2019-01-23 2:05 ` Daniel Jordan
2019-01-25 15:31 ` Tejun Heo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201901230044.x0N0iCdk051613@www262.sakura.ne.jp \
--to=penguin-kernel@i-love.sakura.ne.jp \
--cc=daniel.m.jordan@oracle.com \
--cc=jiangshanlai@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.