[RFC] workqueue: do a sanity check on new work

[RFC] workqueue: do a sanity check on new work

[Dan Carpenter]

There may be a better way to do this.  If someone tries to call
shedule_work() on a work_struck before doing an INIT_WORK() then we
hit the BUG_ON(!list_empty(&work->entry)) in __queue_work() and hang.
Instead of that, we could just print a stack dump and return.

It only works if the work->func is NULL at the start but a lot of these
things get initialized with kzalloc() so it probably catches most of
them.

Reported-by: Matt Renzelmann <mjr@cs.wisc.edu>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 5abf42f..45be34f 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1088,6 +1088,11 @@ queue_work_on(int cpu, struct workqueue_struct *wq, struct work_struct *work)
 {
 	int ret = 0;
 
+	if (!work->func) {
+		WARN_ON(1);
+		return 1;
+	}
+
 	if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))) {
 		__queue_work(cpu, wq, work);
 		ret = 1;

Re: [RFC] workqueue: do a sanity check on new work

[Tejun Heo]

Hello, Dan.

On Fri, Apr 13, 2012 at 7:35 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> There may be a better way to do this.  If someone tries to call
> shedule_work() on a work_struck before doing an INIT_WORK() then we
> hit the BUG_ON(!list_empty(&work->entry)) in __queue_work() and hang.
> Instead of that, we could just print a stack dump and return.

Hmmm... why not just change the BUG_ON() in __queue_work() to "if
(WARN_ON()) return;"?

Thanks.

-- 
tejun

Re: [RFC] workqueue: do a sanity check on new work

[Dan Carpenter]

On Fri, Apr 13, 2012 at 09:39:13AM -0700, Tejun Heo wrote:
> Hello, Dan.
> 
> On Fri, Apr 13, 2012 at 7:35 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > There may be a better way to do this.  If someone tries to call
> > shedule_work() on a work_struck before doing an INIT_WORK() then we
> > hit the BUG_ON(!list_empty(&work->entry)) in __queue_work() and hang.
> > Instead of that, we could just print a stack dump and return.
> 
> Hmmm... why not just change the BUG_ON() in __queue_work() to "if
> (WARN_ON()) return;"?
> 

Good point.  I'll do that.

regards,
dan carpenter

[patch v2] workqueue: change BUG_ON() to WARN_ON()

[Dan Carpenter]

This BUG_ON() can be triggered if you call schedule_work() before
calling INIT_WORK().  It is a bug definitely, but it's nicer to just
print a stack trace and return.

Reported-by: Matt Renzelmann <mjr@cs.wisc.edu>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: the first version added a second check before the BUG_ON()

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 5abf42f..66ec08d 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1032,7 +1032,10 @@ static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
 	cwq = get_cwq(gcwq->cpu, wq);
 	trace_workqueue_queue_work(cpu, cwq, work);
 
-	BUG_ON(!list_empty(&work->entry));
+	if (WARN_ON(!list_empty(&work->entry))) {
+		spin_unlock_irqrestore(&gcwq->lock, flags);
+		return;
+	}
 
 	cwq->nr_in_flight[cwq->work_color]++;
 	work_flags = work_color_to_flags(cwq->work_color);

Re: [patch v2] workqueue: change BUG_ON() to WARN_ON()

[Tejun Heo]

On Fri, Apr 13, 2012 at 10:06:58PM +0300, Dan Carpenter wrote:
> This BUG_ON() can be triggered if you call schedule_work() before
> calling INIT_WORK().  It is a bug definitely, but it's nicer to just
> print a stack trace and return.
> 
> Reported-by: Matt Renzelmann <mjr@cs.wisc.edu>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied to wq/for-3.5.  Thanks.

-- 
tejun