Messages in this thread | | | From | "Eric W. Biederman" <> | Date | Mon, 23 May 2022 09:20:28 -0500 | Subject | Re: [PATCH v3] mwifiex: fix sleep in atomic context bugs caused by dev_coredumpv |
| |
Duoming Zhou <duoming@zju.edu.cn> writes:
> There are sleep in atomic context bugs when uploading device dump > data in mwifiex. The root cause is that dev_coredumpv could not > be used in atomic contexts, because it calls dev_set_name which > include operations that may sleep. The call tree shows 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 > > In order to let dev_coredumpv support atomic contexts, this patch > changes the gfp_t parameter of kvasprintf_const and kstrdup in > kobject_set_name_vargs from GFP_KERNEL to GFP_ATOMIC. What's more, > In order to mitigate bug, this patch changes the gfp_t parameter > of dev_coredumpv from GFP_KERNEL to GFP_ATOMIC.
vmalloc in atomic context?
Not only does dev_coredumpm set a device name dev_coredumpm creates an entire device to hold the device dump.
My sense is that either dev_coredumpm needs to be rebuilt on a completely different principle that does not need a device to hold the coredump (so that it can be called from interrupt context) or that dev_coredumpm should never be called in an context that can not sleep.
Looking at fw_dump_timer_fn the only purpose of the timer is to trigger a device dump after a certain amount of time. So I suspect all that is needed to fix this issue is to change the type of devdump_timer to struct delayed_work and use scheduled_delayed_work instead of mod_timer.
Eric
p.s. I looked at this because there was coredump in the infrastructure name, and I do some of the work to keep coredumps working. Device dump seems like a much better term, and I wished the designer of the api had used that instead.
| |