There are sleep in atomic context bugs when uploading device dump
data on usb interface. The root cause is that the operations that
may sleep are called in fw_dump_timer_fn which is a timer handler.
The call tree shows the execution paths that could lead to bugs:
(Interrupt context)
fw_dump_timer_fn
mwifiex_upload_device_dump
dev_coredumpv(..., GFP_KERNEL)
dev_coredumpm()
kzalloc(sizeof(*devcd), gfp); //may sleep
dev_set_name
kobject_set_name_vargs
kvasprintf_const(GFP_KERNEL, ...); //may sleep
kstrdup(s, GFP_KERNEL); //may sleep
This patch moves the operations that may sleep into a work item.
The work item will run in another kernel thread which is in
process context to execute the bottom half of the interrupt.
So it could prevent atomic context from sleeping.
Fixes: f5ecd02a8b20 ("mwifiex: device dump support for usb interface")
Signed-off-by: Duoming Zhou <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/init.c | 12 +++++++++++-
drivers/net/wireless/marvell/mwifiex/main.h | 1 +
drivers/net/wireless/marvell/mwifiex/sta_event.c | 1 +
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index 88c72d1827a..727963d0c82 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -63,11 +63,19 @@ static void wakeup_timer_fn(struct timer_list *t)
adapter->if_ops.card_reset(adapter);
}
+static void fw_dump_work(struct work_struct *work)
+{
+ struct mwfiex_adapter *adapter =
+ container_of(work, struct mwfiex_adapter, devdump_work);
+
+ mwifiex_upload_device_dump(adapter);
+}
+
static void fw_dump_timer_fn(struct timer_list *t)
{
struct mwifiex_adapter *adapter = from_timer(adapter, t, devdump_timer);
- mwifiex_upload_device_dump(adapter);
+ schedule_work(&adapter->devdump_work);
}
/*
@@ -321,6 +329,7 @@ static void mwifiex_init_adapter(struct mwifiex_adapter *adapter)
adapter->active_scan_triggered = false;
timer_setup(&adapter->wakeup_timer, wakeup_timer_fn, 0);
adapter->devdump_len = 0;
+ INIT_WORK(&adapter->devdump_work, fw_dump_work);
timer_setup(&adapter->devdump_timer, fw_dump_timer_fn, 0);
}
@@ -401,6 +410,7 @@ mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
{
del_timer(&adapter->wakeup_timer);
del_timer_sync(&adapter->devdump_timer);
+ cancel_work_sync(&adapter->devdump_work);
mwifiex_cancel_all_pending_cmd(adapter);
wake_up_interruptible(&adapter->cmd_wait_q.wait);
wake_up_interruptible(&adapter->hs_activate_wait_q);
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 332dd1c8db3..c8ac2f57f18 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -900,6 +900,7 @@ struct mwifiex_adapter {
struct work_struct rx_work;
struct workqueue_struct *dfs_workqueue;
struct work_struct dfs_work;
+ struct work_struct devdump_work;
bool rx_work_enabled;
bool rx_processing;
bool delay_main_work;
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_event.c b/drivers/net/wireless/marvell/mwifiex/sta_event.c
index 7d42c5d2dbf..8e28d0107d7 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
@@ -644,6 +644,7 @@ mwifiex_fw_dump_info_event(struct mwifiex_private *priv,
upload_dump:
del_timer_sync(&adapter->devdump_timer);
+ cancel_work_sync(&adapter->devdump_work);
mwifiex_upload_device_dump(adapter);
}
--
2.17.1
Hello,
On Thu, 19 May 2022 13:27:07 +0300 Kalle Valo wrote:
> > There are sleep in atomic context bugs when uploading device dump
> > data on usb interface. The root cause is that the operations that
> > may sleep are called in fw_dump_timer_fn which is a timer handler.
> > The call tree shows the execution paths that could lead to bugs:
> >
> > (Interrupt context)
> > fw_dump_timer_fn
> > mwifiex_upload_device_dump
> > dev_coredumpv(..., GFP_KERNEL)
> > dev_coredumpm()
> > kzalloc(sizeof(*devcd), gfp); //may sleep
> > dev_set_name
> > kobject_set_name_vargs
> > kvasprintf_const(GFP_KERNEL, ...); //may sleep
> > kstrdup(s, GFP_KERNEL); //may sleep
> >
> > This patch moves the operations that may sleep into a work item.
> > The work item will run in another kernel thread which is in
> > process context to execute the bottom half of the interrupt.
> > So it could prevent atomic context from sleeping.
> >
> > Fixes: f5ecd02a8b20 ("mwifiex: device dump support for usb interface")
> > Signed-off-by: Duoming Zhou <[email protected]>
>
> Have you tested this on real hardware? Or is this just a theoretical
> fix?
This is a theoretical fix. I don't have the real hardware.
Best regards,
Duoming Zhou
[email protected] writes:
> Hello,
>
> On Thu, 19 May 2022 13:27:07 +0300 Kalle Valo wrote:
>
>> > There are sleep in atomic context bugs when uploading device dump
>> > data on usb interface. The root cause is that the operations that
>> > may sleep are called in fw_dump_timer_fn which is a timer handler.
>> > The call tree shows the execution paths that could lead to bugs:
>> >
>> > (Interrupt context)
>> > fw_dump_timer_fn
>> > mwifiex_upload_device_dump
>> > dev_coredumpv(..., GFP_KERNEL)
>> > dev_coredumpm()
>> > kzalloc(sizeof(*devcd), gfp); //may sleep
>> > dev_set_name
>> > kobject_set_name_vargs
>> > kvasprintf_const(GFP_KERNEL, ...); //may sleep
>> > kstrdup(s, GFP_KERNEL); //may sleep
>> >
>> > This patch moves the operations that may sleep into a work item.
>> > The work item will run in another kernel thread which is in
>> > process context to execute the bottom half of the interrupt.
>> > So it could prevent atomic context from sleeping.
>> >
>> > Fixes: f5ecd02a8b20 ("mwifiex: device dump support for usb interface")
>> > Signed-off-by: Duoming Zhou <[email protected]>
>>
>> Have you tested this on real hardware? Or is this just a theoretical
>> fix?
>
> This is a theoretical fix. I don't have the real hardware.
For such patches clearly document that in the commit log, for example
something like "Compile tested only." or similar. But do take into
account that I'm wary about non-trivial fixes which have not been tested
on a real device, it's easy to do more harm than good.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Thu, May 19, 2022 at 5:44 PM Kalle Valo <[email protected]> wrote:
> [email protected] writes:
> > On Thu, 19 May 2022 13:27:07 +0300 Kalle Valo wrote:
...
> >> > Fixes: f5ecd02a8b20 ("mwifiex: device dump support for usb interface")
> >> > Signed-off-by: Duoming Zhou <[email protected]>
> >>
> >> Have you tested this on real hardware? Or is this just a theoretical
> >> fix?
> >
> > This is a theoretical fix. I don't have the real hardware.
>
> For such patches clearly document that in the commit log, for example
> something like "Compile tested only."
It seems I even missed this part... :-(
--
With Best Regards,
Andy Shevchenko