[PATCH] megaraid: fix use of delayed work

[PATCH] megaraid: fix use of delayed work

[Xiaotian Feng]

megaraid use INIT_WORK to declare a hotplug_work, but cast the hotplug_work
from work_struct to delayed_work and schedule_delayed_work on it. This is
very dangerous, as other part of delayed_work might be kernel memories allocated
by others.

With commit 8852aac, schedule_delayed_work() will check dwork->timer before
queue_work, this will cause megaraid code to hit the BUG_ON in workqueue code.
Change megaraid code to use delayed work.

Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Neela Syam Kolli <megaraidlinux@lsi.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/megaraid/megaraid_sas.h      |    2 +-
 drivers/scsi/megaraid/megaraid_sas_base.c |   14 ++++++--------
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas.h b/drivers/scsi/megaraid/megaraid_sas.h
index 16b7a72..3b2365c 100644
--- a/drivers/scsi/megaraid/megaraid_sas.h
+++ b/drivers/scsi/megaraid/megaraid_sas.h
@@ -1276,7 +1276,7 @@ struct megasas_evt_detail {
 } __attribute__ ((packed));
 
 struct megasas_aen_event {
-	struct work_struct hotplug_work;
+	struct delayed_work hotplug_work;
 	struct megasas_instance *instance;
 };
 
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index d2c5366..e4f2baa 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -2060,9 +2060,9 @@ megasas_service_aen(struct megasas_instance *instance, struct megasas_cmd *cmd)
 		} else {
 			ev->instance = instance;
 			instance->ev = ev;
-			INIT_WORK(&ev->hotplug_work, megasas_aen_polling);
-			schedule_delayed_work(
-				(struct delayed_work *)&ev->hotplug_work, 0);
+			INIT_DELAYED_WORK(&ev->hotplug_work,
+					  megasas_aen_polling);
+			schedule_delayed_work(&ev->hotplug_work, 0);
 		}
 	}
 }
@@ -4352,8 +4352,7 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state)
 	/* cancel the delayed work if this work still in queue */
 	if (instance->ev != NULL) {
 		struct megasas_aen_event *ev = instance->ev;
-		cancel_delayed_work_sync(
-			(struct delayed_work *)&ev->hotplug_work);
+		cancel_delayed_work_sync(&ev->hotplug_work);
 		instance->ev = NULL;
 	}
 
@@ -4545,8 +4544,7 @@ static void __devexit megasas_detach_one(struct pci_dev *pdev)
 	/* cancel the delayed work if this work still in queue*/
 	if (instance->ev != NULL) {
 		struct megasas_aen_event *ev = instance->ev;
-		cancel_delayed_work_sync(
-			(struct delayed_work *)&ev->hotplug_work);
+		cancel_delayed_work_sync(&ev->hotplug_work);
 		instance->ev = NULL;
 	}
 
@@ -5190,7 +5188,7 @@ static void
 megasas_aen_polling(struct work_struct *work)
 {
 	struct megasas_aen_event *ev =
-		container_of(work, struct megasas_aen_event, hotplug_work);
+		container_of(work, struct megasas_aen_event, hotplug_work.work);
 	struct megasas_instance *instance = ev->instance;
 	union megasas_evt_class_locale class_locale;
 	struct  Scsi_Host *host;
-- 
1.7.9.6 (Apple Git-31.1)

Re: [PATCH] megaraid: fix use of delayed work

[Tejun Heo]

On Tue, Dec 04, 2012 at 07:33:54PM +0800, Xiaotian Feng wrote:
> megaraid use INIT_WORK to declare a hotplug_work, but cast the hotplug_work
> from work_struct to delayed_work and schedule_delayed_work on it. This is
> very dangerous, as other part of delayed_work might be kernel memories allocated
> by others.
> 
> With commit 8852aac, schedule_delayed_work() will check dwork->timer before
> queue_work, this will cause megaraid code to hit the BUG_ON in workqueue code.
> Change megaraid code to use delayed work.
> 
> Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Neela Syam Kolli <megaraidlinux@lsi.com>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: linux-scsi@vger.kernel.org

Urgh... what the....  Didn't see that one coming.  I'm gonna push this
to Linus through the workqueue tree.

Thanks for the fix.

-- 
tejun

Re: [PATCH] megaraid: fix use of delayed work

[Tejun Heo]

Hello, again, Xiaotian.

On Tue, Dec 04, 2012 at 07:39:39AM -0800, Tejun Heo wrote:
> Urgh... what the....  Didn't see that one coming.  I'm gonna push this
> to Linus through the workqueue tree.
> 
> Thanks for the fix.

It seems like megaraid doesn't have any reason to use delayed_work at
all.  It can use a plain work and cancel_work_sync() instead.  Maybe
it was doing the container_of() thing because workqueue didn't use to
have cancel_work_sync()?  Anyways, please convert it to use plain
work_struct post 3.7.

Thanks.

-- 
tejun

[PATCH 1/2] megaraid: fix BUG_ON() from incorrect use of delayed work

[Tejun Heo]

>From c1d390d8e6128b050f0f66b1c33d390760deb3f4 Mon Sep 17 00:00:00 2001
From: Xiaotian Feng <xtfeng@gmail.com>
Date: Tue, 4 Dec 2012 19:33:54 +0800

megaraid use INIT_WORK to declare a hotplug_work, but cast the
hotplug_work from work_struct to delayed_work and
schedule_delayed_work on it.  This is very dangerous, as other part of
delayed_work might be kernel memories allocated by others.

With commit 8852aac ("workqueue: mod_delayed_work_on() shouldn't queue
timer on 0 delay"), schedule_delayed_work() will check dwork->timer
before queue_work even when @delay is 0, this causes megaraid code to
hit the BUG_ON() in workqueue code.  Change megaraid code to use
delayed work.

Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Neela Syam Kolli <megaraidlinux@lsi.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/megaraid/megaraid_sas.h      |  2 +-
 drivers/scsi/megaraid/megaraid_sas_base.c | 14 ++++++--------
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas.h b/drivers/scsi/megaraid/megaraid_sas.h
index 16b7a72..3b2365c 100644
--- a/drivers/scsi/megaraid/megaraid_sas.h
+++ b/drivers/scsi/megaraid/megaraid_sas.h
@@ -1276,7 +1276,7 @@ struct megasas_evt_detail {
 } __attribute__ ((packed));
 
 struct megasas_aen_event {
-	struct work_struct hotplug_work;
+	struct delayed_work hotplug_work;
 	struct megasas_instance *instance;
 };
 
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index d2c5366..e4f2baa 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -2060,9 +2060,9 @@ megasas_service_aen(struct megasas_instance *instance, struct megasas_cmd *cmd)
 		} else {
 			ev->instance = instance;
 			instance->ev = ev;
-			INIT_WORK(&ev->hotplug_work, megasas_aen_polling);
-			schedule_delayed_work(
-				(struct delayed_work *)&ev->hotplug_work, 0);
+			INIT_DELAYED_WORK(&ev->hotplug_work,
+					  megasas_aen_polling);
+			schedule_delayed_work(&ev->hotplug_work, 0);
 		}
 	}
 }
@@ -4352,8 +4352,7 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state)
 	/* cancel the delayed work if this work still in queue */
 	if (instance->ev != NULL) {
 		struct megasas_aen_event *ev = instance->ev;
-		cancel_delayed_work_sync(
-			(struct delayed_work *)&ev->hotplug_work);
+		cancel_delayed_work_sync(&ev->hotplug_work);
 		instance->ev = NULL;
 	}
 
@@ -4545,8 +4544,7 @@ static void __devexit megasas_detach_one(struct pci_dev *pdev)
 	/* cancel the delayed work if this work still in queue*/
 	if (instance->ev != NULL) {
 		struct megasas_aen_event *ev = instance->ev;
-		cancel_delayed_work_sync(
-			(struct delayed_work *)&ev->hotplug_work);
+		cancel_delayed_work_sync(&ev->hotplug_work);
 		instance->ev = NULL;
 	}
 
@@ -5190,7 +5188,7 @@ static void
 megasas_aen_polling(struct work_struct *work)
 {
 	struct megasas_aen_event *ev =
-		container_of(work, struct megasas_aen_event, hotplug_work);
+		container_of(work, struct megasas_aen_event, hotplug_work.work);
 	struct megasas_instance *instance = ev->instance;
 	union megasas_evt_class_locale class_locale;
 	struct  Scsi_Host *host;
-- 
1.7.11.7

[PATCH 2/2] workqueue: convert BUG_ON()s in __queue_delayed_work() to WARN_ON_ONCE()s

[Tejun Heo]

>From 87aa1e796ff6d491b5ed4e5663e5a4e449ac513b Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Tue, 4 Dec 2012 07:40:39 -0800

8852aac25e ("workqueue: mod_delayed_work_on() shouldn't queue timer on
0 delay") unexpectedly uncovered a very nasty abuse of delayed_work in
megaraid - it allocated work_struct, used container_of() to cast it to
delayed_work and then pass that into queue_delayed_work().

Previously, this was okay because 0 @delay short-circuited to
queue_work() before doing anything with delayed_work.  8852aac25e
moved 0 @delay test into __queue_delayed_work() after sanity check on
delayed_work making megaraid trigger BUG_ON().

Although megaraid is already fixed by c1d390d8e6 ("megaraid: fix
BUG_ON() from incorrect use of delayed work"), this patch converts
BUG_ON()s in __queue_delayed_work() to WARN_ON_ONCE()s so that such
abusers, if there are more, trigger warning but don't crash the
machine.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Xiaotian Feng <xtfeng@gmail.com>
---
 kernel/workqueue.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 084aa47..1dae900 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1361,8 +1361,8 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
 
 	WARN_ON_ONCE(timer->function != delayed_work_timer_fn ||
 		     timer->data != (unsigned long)dwork);
-	BUG_ON(timer_pending(timer));
-	BUG_ON(!list_empty(&work->entry));
+	WARN_ON_ONCE(timer_pending(timer));
+	WARN_ON_ONCE(!list_empty(&work->entry));
 
 	/*
 	 * If @delay is 0, queue @dwork->work immediately.  This is for
-- 
1.7.11.7

Re: [PATCH 1/2] megaraid: fix BUG_ON() from incorrect use of delayed work

[Daniel Vacek]

On Tue, Dec 4, 2012 at 4:54 PM, Tejun Heo <tj@kernel.org> wrote:
> @@ -5190,7 +5188,7 @@ static void
>  megasas_aen_polling(struct work_struct *work)
>  {
>         struct megasas_aen_event *ev =
> -               container_of(work, struct megasas_aen_event, hotplug_work);
> +               container_of(work, struct megasas_aen_event, hotplug_work.work);
>         struct megasas_instance *instance = ev->instance;
>         union megasas_evt_class_locale class_locale;
>         struct  Scsi_Host *host;

-megasas_aen_polling(struct work_struct *work)
+megasas_aen_polling(struct delayed_work *work)

Not really sure. Just asking?

--nX

Re: [PATCH 1/2] megaraid: fix BUG_ON() from incorrect use of delayed work

[Daniel Vacek]

On Wed, Dec 5, 2012 at 12:02 PM, Daniel Vacek <neelx.g@gmail.com> wrote:
> On Tue, Dec 4, 2012 at 4:54 PM, Tejun Heo <tj@kernel.org> wrote:
>> @@ -5190,7 +5188,7 @@ static void
>>  megasas_aen_polling(struct work_struct *work)
>>  {
>>         struct megasas_aen_event *ev =
>> -               container_of(work, struct megasas_aen_event, hotplug_work);
>> +               container_of(work, struct megasas_aen_event, hotplug_work.work);
>>         struct megasas_instance *instance = ev->instance;
>>         union megasas_evt_class_locale class_locale;
>>         struct  Scsi_Host *host;
>
> -megasas_aen_polling(struct work_struct *work)
> +megasas_aen_polling(struct delayed_work *work)
>
> Not really sure. Just asking?

Never mind, it's ok. Now I get it. Sorry for the buzz.

--nX