| Message ID | 20220519101656.44513-1-duoming@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: wireless: marvell: mwifiex: fix sleep in atomic context bugs | expand |
Duoming Zhou <duoming@zju.edu.cn> writes: > 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 <duoming@zju.edu.cn> Have you tested this on real hardware? Or is this just a theoretical fix?
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 <duoming@zju.edu.cn> > > 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
Hi Duoming,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
url: https://github.com/intel-lab-lkp/linux/commits/Duoming-Zhou/net-wireless-marvell-mwifiex-fix-sleep-in-atomic-context-bugs/20220519-181826
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git fbb3abdf2223cd0dfc07de85fe5a43ba7f435bdf
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220519/202205191932.qrHJI7FT-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d11bfae513f24308f5315efe8ca56471eff8e76c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Duoming-Zhou/net-wireless-marvell-mwifiex-fix-sleep-in-atomic-context-bugs/20220519-181826
git checkout d11bfae513f24308f5315efe8ca56471eff8e76c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from include/linux/container_of.h:5,
from include/linux/list.h:5,
from include/linux/wait.h:7,
from drivers/net/wireless/marvell/mwifiex/decl.h:26,
from drivers/net/wireless/marvell/mwifiex/init.c:20:
drivers/net/wireless/marvell/mwifiex/init.c: In function 'fw_dump_work':
>> include/linux/container_of.h:19:54: error: invalid use of undefined type 'struct mwfiex_adapter'
19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~
include/linux/build_bug.h:78:56: note: in definition of macro '__static_assert'
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~~~
include/linux/container_of.h:19:9: note: in expansion of macro 'static_assert'
19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~~~
include/linux/container_of.h:19:23: note: in expansion of macro '__same_type'
19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~
drivers/net/wireless/marvell/mwifiex/init.c:69:17: note: in expansion of macro 'container_of'
69 | container_of(work, struct mwfiex_adapter, devdump_work);
| ^~~~~~~~~~~~
include/linux/compiler_types.h:293:27: error: expression in static assertion is not an integer
293 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:78:56: note: in definition of macro '__static_assert'
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~~~
include/linux/container_of.h:19:9: note: in expansion of macro 'static_assert'
19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~~~
include/linux/container_of.h:19:23: note: in expansion of macro '__same_type'
19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~
drivers/net/wireless/marvell/mwifiex/init.c:69:17: note: in expansion of macro 'container_of'
69 | container_of(work, struct mwfiex_adapter, devdump_work);
| ^~~~~~~~~~~~
In file included from include/uapi/linux/posix_types.h:5,
from include/uapi/linux/types.h:14,
from include/linux/types.h:6,
from include/linux/kasan-checks.h:5,
from include/asm-generic/rwonce.h:26,
from ./arch/m68k/include/generated/asm/rwonce.h:1,
from include/linux/compiler.h:248,
from include/linux/build_bug.h:5,
from include/linux/container_of.h:5,
from include/linux/list.h:5,
from include/linux/wait.h:7,
from drivers/net/wireless/marvell/mwifiex/decl.h:26,
from drivers/net/wireless/marvell/mwifiex/init.c:20:
>> include/linux/stddef.h:16:33: error: invalid use of undefined type 'struct mwfiex_adapter'
16 | #define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER)
| ^~~~~~~~~~~~~~~~~~
include/linux/container_of.h:22:28: note: in expansion of macro 'offsetof'
22 | ((type *)(__mptr - offsetof(type, member))); })
| ^~~~~~~~
drivers/net/wireless/marvell/mwifiex/init.c:69:17: note: in expansion of macro 'container_of'
69 | container_of(work, struct mwfiex_adapter, devdump_work);
| ^~~~~~~~~~~~
>> drivers/net/wireless/marvell/mwifiex/init.c:71:36: error: passing argument 1 of 'mwifiex_upload_device_dump' from incompatible pointer type [-Werror=incompatible-pointer-types]
71 | mwifiex_upload_device_dump(adapter);
| ^~~~~~~
| |
| struct mwfiex_adapter *
In file included from drivers/net/wireless/marvell/mwifiex/init.c:24:
drivers/net/wireless/marvell/mwifiex/main.h:1688:57: note: expected 'struct mwifiex_adapter *' but argument is of type 'struct mwfiex_adapter *'
1688 | void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter);
| ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
cc1: some warnings being treated as errors
vim +19 include/linux/container_of.h
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 9
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 10 /**
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 11 * container_of - cast a member of a structure out to the containing structure
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 12 * @ptr: the pointer to the member.
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 13 * @type: the type of the container struct this is embedded in.
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 14 * @member: the name of the member within the struct.
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 15 *
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 16 */
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 17 #define container_of(ptr, type, member) ({ \
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 18 void *__mptr = (void *)(ptr); \
e1edc277e6f6df Rasmus Villemoes 2021-11-08 @19 static_assert(__same_type(*(ptr), ((type *)0)->member) || \
e1edc277e6f6df Rasmus Villemoes 2021-11-08 20 __same_type(*(ptr), void), \
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 21 "pointer type mismatch in container_of()"); \
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 22 ((type *)(__mptr - offsetof(type, member))); })
d2a8ebbf8192b8 Andy Shevchenko 2021-11-08 23
duoming@zju.edu.cn 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 <duoming@zju.edu.cn> >> >> 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.
On Thu, May 19, 2022 at 5:44 PM Kalle Valo <kvalo@kernel.org> wrote: > duoming@zju.edu.cn 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 <duoming@zju.edu.cn> > >> > >> 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... :-(
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); }
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 <duoming@zju.edu.cn> --- 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(-)