From: duoming@zju.edu.cn
To: "Kalle Valo" <kvalo@kernel.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>,
linux-wireless@vger.kernel.org, amitkarwar@gmail.com,
ganapathi017@gmail.com, sharvari.harisangam@nxp.com,
huxinming820@gmail.com, davem@davemloft.net, edumazet@google.com,
kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
rafael@kernel.org
Subject: Re: [PATCH v3] mwifiex: fix sleep in atomic context bugs caused by dev_coredumpv
Date: Tue, 24 May 2022 10:08:39 +0800 (GMT+08:00) [thread overview]
Message-ID: <50aec6d7.2d054.180f3d2cbc0.Coremail.duoming@zju.edu.cn> (raw)
In-Reply-To: <87ee0kyzvc.fsf@kernel.org>
Hello,
On Mon, 23 May 2022 19:31:35 +0300 Kalle Valo wrote:
> >> > 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.
> >
> > The following solution removes the gfp_t parameter of dev_coredumpv(),
> > dev_coredumpm() and dev_coredumpsg() and change the gfp_t parameter of
> > kzalloc() in dev_coredumpm() to GFP_KERNEL, in order to show that these
> > functions can not be used in atomic context.
> >
> > What's more, I move the operations that may sleep into a work item and use
> > schedule_work() to call a kernel thread to do the operations that may sleep.
> >
>
> [...]
>
> > --- 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 mwifiex_adapter *adapter =
> > + container_of(work, struct mwifiex_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);
>
> In this patch please only do the API change in mwifiex. The change to
> using a workqueue needs to be in separate patch so it can be properly
> tested. I don't want a change like that going to the kernel without
> testing on a real device.
Thank you for your suggestions! I will only do the API change in mwifies and
keep other places that call dev_coredumpv remain unchanged. I will test the
new separate patches on real Marvell wifi chip these days.
Best regards,
Duoming Zhou
next prev parent reply other threads:[~2022-05-24 2:09 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-23 5:28 [PATCH v3] mwifiex: fix sleep in atomic context bugs caused by dev_coredumpv Duoming Zhou
2022-05-23 6:31 ` Greg KH
[not found] ` <41a266af.2abb6.180efa8594d.Coremail.duoming@zju.edu.cn>
2022-05-23 11:31 ` Kalle Valo
2022-05-23 13:41 ` Greg KH
2022-05-23 16:27 ` Kalle Valo
2022-05-23 13:43 ` Johannes Berg
2022-05-23 14:20 ` Eric W. Biederman
2022-05-23 15:58 ` duoming
2022-05-23 16:31 ` Kalle Valo
2022-05-24 2:08 ` duoming [this message]
2022-05-23 19:42 ` Brian Norris
2022-05-24 1:54 ` duoming
2022-05-23 17:55 ` Brian Norris
2022-05-23 8:03 duoming
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50aec6d7.2d054.180f3d2cbc0.Coremail.duoming@zju.edu.cn \
--to=duoming@zju.edu.cn \
--cc=amitkarwar@gmail.com \
--cc=davem@davemloft.net \
--cc=ebiederm@xmission.com \
--cc=edumazet@google.com \
--cc=ganapathi017@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=huxinming820@gmail.com \
--cc=kuba@kernel.org \
--cc=kvalo@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=rafael@kernel.org \
--cc=sharvari.harisangam@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).