* [PATCH] megaraid: fix use of delayed work
@ 2012-12-04 11:33 Xiaotian Feng
2012-12-04 15:39 ` Tejun Heo
2012-12-04 15:54 ` [PATCH 1/2] megaraid: fix BUG_ON() from incorrect " Tejun Heo
0 siblings, 2 replies; 7+ messages in thread
From: Xiaotian Feng @ 2012-12-04 11:33 UTC (permalink / raw)
To: linux-kernel, tj
Cc: Xiaotian Feng, Xiaotian Feng, Neela Syam Kolli,
James E.J. Bottomley, linux-scsi
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)
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] megaraid: fix use of delayed work
2012-12-04 11:33 [PATCH] megaraid: fix use of delayed work Xiaotian Feng
@ 2012-12-04 15:39 ` Tejun Heo
2012-12-04 15:57 ` Tejun Heo
2012-12-04 15:54 ` [PATCH 1/2] megaraid: fix BUG_ON() from incorrect " Tejun Heo
1 sibling, 1 reply; 7+ messages in thread
From: Tejun Heo @ 2012-12-04 15:39 UTC (permalink / raw)
To: Xiaotian Feng
Cc: linux-kernel, Xiaotian Feng, Neela Syam Kolli,
James E.J. Bottomley, linux-scsi
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] megaraid: fix BUG_ON() from incorrect use of delayed work
2012-12-04 11:33 [PATCH] megaraid: fix use of delayed work Xiaotian Feng
2012-12-04 15:39 ` Tejun Heo
@ 2012-12-04 15:54 ` Tejun Heo
2012-12-04 15:55 ` [PATCH 2/2] workqueue: convert BUG_ON()s in __queue_delayed_work() to WARN_ON_ONCE()s Tejun Heo
2012-12-05 11:02 ` [PATCH 1/2] megaraid: fix BUG_ON() from incorrect use of delayed work Daniel Vacek
1 sibling, 2 replies; 7+ messages in thread
From: Tejun Heo @ 2012-12-04 15:54 UTC (permalink / raw)
To: Xiaotian Feng
Cc: linux-kernel, Xiaotian Feng, Neela Syam Kolli,
James E.J. Bottomley, linux-scsi
>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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] workqueue: convert BUG_ON()s in __queue_delayed_work() to WARN_ON_ONCE()s
2012-12-04 15:54 ` [PATCH 1/2] megaraid: fix BUG_ON() from incorrect " Tejun Heo
@ 2012-12-04 15:55 ` Tejun Heo
2012-12-05 11:02 ` [PATCH 1/2] megaraid: fix BUG_ON() from incorrect use of delayed work Daniel Vacek
1 sibling, 0 replies; 7+ messages in thread
From: Tejun Heo @ 2012-12-04 15:55 UTC (permalink / raw)
To: Xiaotian Feng
Cc: linux-kernel, Xiaotian Feng, Neela Syam Kolli,
James E.J. Bottomley, linux-scsi
>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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] megaraid: fix use of delayed work
2012-12-04 15:39 ` Tejun Heo
@ 2012-12-04 15:57 ` Tejun Heo
0 siblings, 0 replies; 7+ messages in thread
From: Tejun Heo @ 2012-12-04 15:57 UTC (permalink / raw)
To: Xiaotian Feng
Cc: linux-kernel, Xiaotian Feng, Neela Syam Kolli,
James E.J. Bottomley, linux-scsi
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] megaraid: fix BUG_ON() from incorrect use of delayed work
2012-12-04 15:54 ` [PATCH 1/2] megaraid: fix BUG_ON() from incorrect " Tejun Heo
2012-12-04 15:55 ` [PATCH 2/2] workqueue: convert BUG_ON()s in __queue_delayed_work() to WARN_ON_ONCE()s Tejun Heo
@ 2012-12-05 11:02 ` Daniel Vacek
2012-12-05 11:16 ` Daniel Vacek
1 sibling, 1 reply; 7+ messages in thread
From: Daniel Vacek @ 2012-12-05 11:02 UTC (permalink / raw)
To: Tejun Heo
Cc: Xiaotian Feng, linux-kernel, Xiaotian Feng, Neela Syam Kolli,
James E.J. Bottomley, linux-scsi
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] megaraid: fix BUG_ON() from incorrect use of delayed work
2012-12-05 11:02 ` [PATCH 1/2] megaraid: fix BUG_ON() from incorrect use of delayed work Daniel Vacek
@ 2012-12-05 11:16 ` Daniel Vacek
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vacek @ 2012-12-05 11:16 UTC (permalink / raw)
To: Tejun Heo
Cc: Xiaotian Feng, linux-kernel, Xiaotian Feng, Neela Syam Kolli,
James E.J. Bottomley, linux-scsi
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-12-05 11:16 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-04 11:33 [PATCH] megaraid: fix use of delayed work Xiaotian Feng
2012-12-04 15:39 ` Tejun Heo
2012-12-04 15:57 ` Tejun Heo
2012-12-04 15:54 ` [PATCH 1/2] megaraid: fix BUG_ON() from incorrect " Tejun Heo
2012-12-04 15:55 ` [PATCH 2/2] workqueue: convert BUG_ON()s in __queue_delayed_work() to WARN_ON_ONCE()s Tejun Heo
2012-12-05 11:02 ` [PATCH 1/2] megaraid: fix BUG_ON() from incorrect use of delayed work Daniel Vacek
2012-12-05 11:16 ` Daniel Vacek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).