Use the GFP_ATOMIC flag of kzalloc() with two memory allocation in
report_del_sta_event(). This function is called while holding spinlocks,
therefore it is not allowed to sleep. With the GFP_ATOMIC type flag, the
allocation is high priority and must not sleep.
This issue is detected by Smatch which emits the following warning:
"drivers/staging/r8188eu/core/rtw_mlme_ext.c:6848 report_del_sta_event()
warn: sleeping in atomic context".
After the change, the post-commit hook output the following message:
"CHECK: Prefer kzalloc(sizeof(*pcmd_obj)...) over
kzalloc(sizeof(struct cmd_obj)...)".
According to the above "CHECK", use the preferred style in the first
kzalloc().
Fixes: 79f712ea994d ("staging: r8188eu: Remove wrappers for kalloc() and kzalloc()")
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
v2->v3: Add the "Fixes:" tag, as requested by Greg Kroah-Hartman.
v1->v2: Fix an error that I introduced with an incorrect copy-paste
of the sizeof() operator.
drivers/staging/r8188eu/core/rtw_mlme_ext.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
index 55c3d4a6faeb..315902682292 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
@@ -6845,12 +6845,12 @@ void report_del_sta_event(struct adapter *padapter, unsigned char *MacAddr, unsi
struct mlme_ext_priv *pmlmeext = &padapter->mlmeextpriv;
struct cmd_priv *pcmdpriv = &padapter->cmdpriv;
- pcmd_obj = kzalloc(sizeof(struct cmd_obj), GFP_KERNEL);
+ pcmd_obj = kzalloc(sizeof(*pcmd_obj), GFP_ATOMIC);
if (!pcmd_obj)
return;
cmdsz = (sizeof(struct stadel_event) + sizeof(struct C2HEvent_Header));
- pevtcmd = kzalloc(cmdsz, GFP_KERNEL);
+ pevtcmd = kzalloc(cmdsz, GFP_ATOMIC);
if (!pevtcmd) {
kfree(pcmd_obj);
return;
--
2.33.1
On Friday, November 5, 2021 2:25:52 PM CET Dan Carpenter wrote:
> On Mon, Nov 01, 2021 at 08:18:47PM +0100, Fabio M. De Francesco wrote:
> > Use the GFP_ATOMIC flag of kzalloc() with two memory allocation in
> > report_del_sta_event(). This function is called while holding spinlocks,
> > therefore it is not allowed to sleep. With the GFP_ATOMIC type flag, the
> > allocation is high priority and must not sleep.
> >
> > This issue is detected by Smatch which emits the following warning:
> > "drivers/staging/r8188eu/core/rtw_mlme_ext.c:6848 report_del_sta_event()
> > warn: sleeping in atomic context".
> >
> > After the change, the post-commit hook output the following message:
> > "CHECK: Prefer kzalloc(sizeof(*pcmd_obj)...) over
> > kzalloc(sizeof(struct cmd_obj)...)".
> >
> > According to the above "CHECK", use the preferred style in the first
> > kzalloc().
> >
> > Fixes: 79f712ea994d ("staging: r8188eu: Remove wrappers for kalloc() and
kzalloc()")
>
> This is not the correct Fixes tag. The original allocation wrappers
> checked in_interrupt() they did not check in_atomic() so they had same
> bug. The correct tag is:
>
> Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for
RTL8188eu driver")
>
> regards,
> dan carpenter
Hello Dan,
I'm sorry but I surely missing something, therefore, before making changes I
need to understand this subject a little better. Let me explain what I am
missing...
The two kzalloc() in report_del_sta_event() are called while spinlocks are
held and bottom halves are disabled by spin_lock_bh(). If I remember it
correctly spin_lock_bh() finally calls __local_bh_disable_ip() to disable
bottom halves on local CPU before actually acquiring the lock.
This is the code and inline documentation of in_interrupt():
/* in_interrupt() - We're in NMI,IRQ,SoftIRQ context or have BH disabled" */
#define irq_count() (nmi_count() | hardirq_count() | softirq_count())
#define in_interrupt() (irq_count())
And this is the code and inline documentation of in_atomic():
"/*
* Are we running in atomic context? WARNING: this macro cannot
* always detect atomic context; in particular, it cannot know about
* held spinlocks in non-preemptible kernels. Thus it should not be
* used in the general case to determine whether sleeping is possible.
* Do not use in_atomic() in driver code.
*/
#define in_atomic() (preempt_count() != 0)
To summarize, I think that using in_interrupt() in the old wrappers was the
wiser choice. Therefore this patch fixes 79f712ea994d ("staging: r8188eu:
Remove wrappers for kalloc() and kzalloc()").
I know that I have so little experience that I shouldn't even discuss this
topics. However, I would appreciate if you may explain with some more details
why in_atomic() should have been preferred over in_interrupt() in the old
wrappers that were removed with commit 79f712ea994d.
Thank you very much in advance,
Fabio M. De Francesco
Oh yeah, you're right. It never *just* does spinlocks (as stated in the
commit message btw), it does spin_lock_bh() which bumps the soft IRQ
count.
> To summarize, I think that using in_interrupt() in the old wrappers was the
> wiser choice.
"Wiser" is not the right word. The wrappers were always stupid, but I
guess they did work in this case so the fixes tag is correct.
regards,
dan carpenter
On Friday, November 5, 2021 4:36:33 PM CET Dan Carpenter wrote:
> Oh yeah, you're right. It never *just* does spinlocks (as stated in the
> commit message btw), it does spin_lock_bh() which bumps the soft IRQ
> count.
Thank you very much for checking and confirming.
> > To summarize, I think that using in_interrupt() in the old wrappers was
the
> > wiser choice.
>
> "Wiser" is not the right word. The wrappers were always stupid, but I
> guess they did work in this case so the fixes tag is correct.
Ah, sorry. I was not able to express my thought properly :(
I agree with you that the wrappers were a not a good idea and Larry did well
in removing them. Furthermore, I think that delegating the choice to use
GFP_KERNEL vs. GFP_ATOMIC depending on the return from in_interrupt() is very
bad design and it adds sensible overhead.
I used "wiser" is a stricter sense. I meant that, if wrappers were needed
(but they were not), in_interrupt() is "wiser" than "in_atomic()".
Regards,
Fabio M. De Francesco
On Mon, Nov 01, 2021 at 08:18:47PM +0100, Fabio M. De Francesco wrote:
> Use the GFP_ATOMIC flag of kzalloc() with two memory allocation in
> report_del_sta_event(). This function is called while holding spinlocks,
> therefore it is not allowed to sleep. With the GFP_ATOMIC type flag, the
> allocation is high priority and must not sleep.
>
> This issue is detected by Smatch which emits the following warning:
> "drivers/staging/r8188eu/core/rtw_mlme_ext.c:6848 report_del_sta_event()
> warn: sleeping in atomic context".
>
> After the change, the post-commit hook output the following message:
> "CHECK: Prefer kzalloc(sizeof(*pcmd_obj)...) over
> kzalloc(sizeof(struct cmd_obj)...)".
>
> According to the above "CHECK", use the preferred style in the first
> kzalloc().
>
> Fixes: 79f712ea994d ("staging: r8188eu: Remove wrappers for kalloc() and kzalloc()")
This is not the correct Fixes tag. The original allocation wrappers
checked in_interrupt() they did not check in_atomic() so they had same
bug. The correct tag is:
Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
regards,
dan carpenter
On Monday, November 1, 2021 8:18:47 PM CET Fabio M. De Francesco wrote:
> Use the GFP_ATOMIC flag of kzalloc() with two memory allocation in
> report_del_sta_event(). This function is called while holding spinlocks,
> therefore it is not allowed to sleep. With the GFP_ATOMIC type flag, the
> allocation is high priority and must not sleep.
>
> This issue is detected by Smatch which emits the following warning:
> "drivers/staging/r8188eu/core/rtw_mlme_ext.c:6848 report_del_sta_event()
> warn: sleeping in atomic context".
>
> After the change, the post-commit hook output the following message:
> "CHECK: Prefer kzalloc(sizeof(*pcmd_obj)...) over
> kzalloc(sizeof(struct cmd_obj)...)".
>
> According to the above "CHECK", use the preferred style in the first
> kzalloc().
>
> Fixes: 79f712ea994d ("staging: r8188eu: Remove wrappers for kalloc() and
kzalloc()")
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
>
> v2->v3: Add the "Fixes:" tag, as requested by Greg Kroah-Hartman.
>
> v1->v2: Fix an error that I introduced with an incorrect copy-paste
> of the sizeof() operator.
>
> drivers/staging/r8188eu/core/rtw_mlme_ext.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Hello Greg,
I've noticed that you have already applied recent changes to drivers/staging
up to the patches of November 6th, but my patch is not among them.
This patch has already been acked by Larry and I'm not sure if I should send
a v4 with his "Acked-by" tag or if you can add it by yourself when applying
to your tree.
Please let me know if there is something that prevents this patch to be
applied. I have no problem in changing / adding whatever it is needed.
Thanks,
Fabio M. De Francesco
On Sunday, November 7, 2021 1:38:35 PM CET Greg Kroah-Hartman wrote:
> On Sun, Nov 07, 2021 at 12:43:51PM +0100, Fabio M. De Francesco wrote:
> > On Monday, November 1, 2021 8:18:47 PM CET Fabio M. De Francesco wrote:
> > > Use the GFP_ATOMIC flag of kzalloc() with two memory allocation in
> > > report_del_sta_event(). This function is called while holding
spinlocks,
> > > therefore it is not allowed to sleep. With the GFP_ATOMIC type flag,
the
> > > allocation is high priority and must not sleep.
> > >
> > > This issue is detected by Smatch which emits the following warning:
> > > "drivers/staging/r8188eu/core/rtw_mlme_ext.c:6848
report_del_sta_event()
> > > warn: sleeping in atomic context".
> > >
> > > After the change, the post-commit hook output the following message:
> > > "CHECK: Prefer kzalloc(sizeof(*pcmd_obj)...) over
> > > kzalloc(sizeof(struct cmd_obj)...)".
> > >
> > > According to the above "CHECK", use the preferred style in the first
> > > kzalloc().
> > >
> > > Fixes: 79f712ea994d ("staging: r8188eu: Remove wrappers for kalloc()
and
> > kzalloc()")
> > > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > > ---
> > > [...]
> > Please let me know if there is something that prevents this patch to be
> > applied. I have no problem in changing / adding whatever it is needed.
>
> Nothing needs to be done, I am waiting for 5.16-rc1 to be released
> before I pick up this patch, and others that will be targeted for
> 5.16-final. Only then will I queue them up, as the automated email you
> should have gotten when you submitted the patch said would happen.
>
> Just relax, there is no rush here :)
>
Oh, sorry Greg. There must be something that I haven't understand about the
development process... :(
Obviously I agree that there is no rush here :)
As I said, this morning I read git log and saw patches that seemed more
recent; thus I thought that was the case to ask. I just (wrongly) thought
that the v3 of the patch got unnoticed or dropped because of some requests
that I had missed.
Thanks for the explanation,
Fabio
> thanks,
>
> greg k-h
>
On Sunday, November 7, 2021 2:29:47 PM CET Greg Kroah-Hartman wrote:
> On Sun, Nov 07, 2021 at 02:15:59PM +0100, Fabio M. De Francesco wrote:
> > On Sunday, November 7, 2021 1:38:35 PM CET Greg Kroah-Hartman wrote:
> > > On Sun, Nov 07, 2021 at 12:43:51PM +0100, Fabio M. De Francesco wrote:
> > > > On Monday, November 1, 2021 8:18:47 PM CET Fabio M. De Francesco
wrote:
> > > > > Use the GFP_ATOMIC flag of kzalloc() with two memory allocation in
> > > > > report_del_sta_event(). This function is called while holding
> > spinlocks,
> > > > > therefore it is not allowed to sleep. With the GFP_ATOMIC type
flag,
> > the
> > > > > allocation is high priority and must not sleep.
> > > > >
> > > > > This issue is detected by Smatch which emits the following warning:
> > > > > "drivers/staging/r8188eu/core/rtw_mlme_ext.c:6848
> > report_del_sta_event()
> > > > > warn: sleeping in atomic context".
> > > > >
> > > > > After the change, the post-commit hook output the following
message:
> > > > > "CHECK: Prefer kzalloc(sizeof(*pcmd_obj)...) over
> > > > > kzalloc(sizeof(struct cmd_obj)...)".
> > > > >
> > > > > According to the above "CHECK", use the preferred style in the
first
> > > > > kzalloc().
> > > > >
> > > > > Fixes: 79f712ea994d ("staging: r8188eu: Remove wrappers for
kalloc()
> > and
> > > > kzalloc()")
> > > > > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > > > > ---
> >
> > > > > [...]
> >
> > > > Please let me know if there is something that prevents this patch to
be
> > > > applied. I have no problem in changing / adding whatever it is
needed.
> > >
> > > Nothing needs to be done, I am waiting for 5.16-rc1 to be released
> > > before I pick up this patch, and others that will be targeted for
> > > 5.16-final. Only then will I queue them up, as the automated email you
> > > should have gotten when you submitted the patch said would happen.
> > >
> > > Just relax, there is no rush here :)
> > >
> >
> > Oh, sorry Greg. There must be something that I haven't understand about
the
> > development process... :(
> >
> > Obviously I agree that there is no rush here :)
> >
> > As I said, this morning I read git log and saw patches that seemed more
> > recent; thus I thought that was the case to ask. I just (wrongly) thought
> > that the v3 of the patch got unnoticed or dropped because of some
requests
> > that I had missed.
>
> Be sure to notice what branch commits are being applied to. There are
> different branches for a reason :)
>
This is what confuses me:
--- git-log output ---
commit 8a893759d0075ea9556abcf86a4826d9865ba4bf (origin/staging-testing)
Author: Phillip Potter <[email protected]>
Date: Sat Nov 6 23:16:36 2021 +0000
staging: r8188eu: remove MSG_88E macro
--- end of git-log output ---
Aside from the "Date" field, I know that this patch has been sent to the list
during the last night and that it goes to the same branch (staging-testing)
to which my patch should go. I know I'm still missing something, but I cannot
understand what it is... :(
Anyway, never mind. I don't want to bother you with this silly questions :)
Again, thanks,
Fabio
On Sun, Nov 07, 2021 at 02:15:59PM +0100, Fabio M. De Francesco wrote:
> On Sunday, November 7, 2021 1:38:35 PM CET Greg Kroah-Hartman wrote:
> > On Sun, Nov 07, 2021 at 12:43:51PM +0100, Fabio M. De Francesco wrote:
> > > On Monday, November 1, 2021 8:18:47 PM CET Fabio M. De Francesco wrote:
> > > > Use the GFP_ATOMIC flag of kzalloc() with two memory allocation in
> > > > report_del_sta_event(). This function is called while holding
> spinlocks,
> > > > therefore it is not allowed to sleep. With the GFP_ATOMIC type flag,
> the
> > > > allocation is high priority and must not sleep.
> > > >
> > > > This issue is detected by Smatch which emits the following warning:
> > > > "drivers/staging/r8188eu/core/rtw_mlme_ext.c:6848
> report_del_sta_event()
> > > > warn: sleeping in atomic context".
> > > >
> > > > After the change, the post-commit hook output the following message:
> > > > "CHECK: Prefer kzalloc(sizeof(*pcmd_obj)...) over
> > > > kzalloc(sizeof(struct cmd_obj)...)".
> > > >
> > > > According to the above "CHECK", use the preferred style in the first
> > > > kzalloc().
> > > >
> > > > Fixes: 79f712ea994d ("staging: r8188eu: Remove wrappers for kalloc()
> and
> > > kzalloc()")
> > > > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > > > ---
>
> > > > [...]
>
> > > Please let me know if there is something that prevents this patch to be
> > > applied. I have no problem in changing / adding whatever it is needed.
> >
> > Nothing needs to be done, I am waiting for 5.16-rc1 to be released
> > before I pick up this patch, and others that will be targeted for
> > 5.16-final. Only then will I queue them up, as the automated email you
> > should have gotten when you submitted the patch said would happen.
> >
> > Just relax, there is no rush here :)
> >
>
> Oh, sorry Greg. There must be something that I haven't understand about the
> development process... :(
>
> Obviously I agree that there is no rush here :)
>
> As I said, this morning I read git log and saw patches that seemed more
> recent; thus I thought that was the case to ask. I just (wrongly) thought
> that the v3 of the patch got unnoticed or dropped because of some requests
> that I had missed.
Be sure to notice what branch commits are being applied to. There are
different branches for a reason :)
thanks,
greg k-h
On Sun, Nov 07, 2021 at 03:03:18PM +0100, Fabio M. De Francesco wrote:
> On Sunday, November 7, 2021 2:29:47 PM CET Greg Kroah-Hartman wrote:
> > On Sun, Nov 07, 2021 at 02:15:59PM +0100, Fabio M. De Francesco wrote:
> > > On Sunday, November 7, 2021 1:38:35 PM CET Greg Kroah-Hartman wrote:
> > > > On Sun, Nov 07, 2021 at 12:43:51PM +0100, Fabio M. De Francesco wrote:
> > > > > On Monday, November 1, 2021 8:18:47 PM CET Fabio M. De Francesco
> wrote:
> > > > > > Use the GFP_ATOMIC flag of kzalloc() with two memory allocation in
> > > > > > report_del_sta_event(). This function is called while holding
> > > spinlocks,
> > > > > > therefore it is not allowed to sleep. With the GFP_ATOMIC type
> flag,
> > > the
> > > > > > allocation is high priority and must not sleep.
> > > > > >
> > > > > > This issue is detected by Smatch which emits the following warning:
> > > > > > "drivers/staging/r8188eu/core/rtw_mlme_ext.c:6848
> > > report_del_sta_event()
> > > > > > warn: sleeping in atomic context".
> > > > > >
> > > > > > After the change, the post-commit hook output the following
> message:
> > > > > > "CHECK: Prefer kzalloc(sizeof(*pcmd_obj)...) over
> > > > > > kzalloc(sizeof(struct cmd_obj)...)".
> > > > > >
> > > > > > According to the above "CHECK", use the preferred style in the
> first
> > > > > > kzalloc().
> > > > > >
> > > > > > Fixes: 79f712ea994d ("staging: r8188eu: Remove wrappers for
> kalloc()
> > > and
> > > > > kzalloc()")
> > > > > > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > > > > > ---
> > >
> > > > > > [...]
> > >
> > > > > Please let me know if there is something that prevents this patch to
> be
> > > > > applied. I have no problem in changing / adding whatever it is
> needed.
> > > >
> > > > Nothing needs to be done, I am waiting for 5.16-rc1 to be released
> > > > before I pick up this patch, and others that will be targeted for
> > > > 5.16-final. Only then will I queue them up, as the automated email you
> > > > should have gotten when you submitted the patch said would happen.
> > > >
> > > > Just relax, there is no rush here :)
> > > >
> > >
> > > Oh, sorry Greg. There must be something that I haven't understand about
> the
> > > development process... :(
> > >
> > > Obviously I agree that there is no rush here :)
> > >
> > > As I said, this morning I read git log and saw patches that seemed more
> > > recent; thus I thought that was the case to ask. I just (wrongly) thought
> > > that the v3 of the patch got unnoticed or dropped because of some
> requests
> > > that I had missed.
> >
> > Be sure to notice what branch commits are being applied to. There are
> > different branches for a reason :)
> >
> This is what confuses me:
>
> --- git-log output ---
>
> commit 8a893759d0075ea9556abcf86a4826d9865ba4bf (origin/staging-testing)
> Author: Phillip Potter <[email protected]>
> Date: Sat Nov 6 23:16:36 2021 +0000
>
> staging: r8188eu: remove MSG_88E macro
>
> --- end of git-log output ---
>
> Aside from the "Date" field, I know that this patch has been sent to the list
> during the last night and that it goes to the same branch (staging-testing)
> to which my patch should go. I know I'm still missing something, but I cannot
> understand what it is... :(
No, your change will go to the staging-linus branch, as it needs to go
into 5.16-final and get sent to Linus much sooner than 5.17-rc1, which
is where things are being queued up in the staging-testing branch at the
moment.
hope this helps,
greg k-h
On Sun, Nov 07, 2021 at 12:43:51PM +0100, Fabio M. De Francesco wrote:
> On Monday, November 1, 2021 8:18:47 PM CET Fabio M. De Francesco wrote:
> > Use the GFP_ATOMIC flag of kzalloc() with two memory allocation in
> > report_del_sta_event(). This function is called while holding spinlocks,
> > therefore it is not allowed to sleep. With the GFP_ATOMIC type flag, the
> > allocation is high priority and must not sleep.
> >
> > This issue is detected by Smatch which emits the following warning:
> > "drivers/staging/r8188eu/core/rtw_mlme_ext.c:6848 report_del_sta_event()
> > warn: sleeping in atomic context".
> >
> > After the change, the post-commit hook output the following message:
> > "CHECK: Prefer kzalloc(sizeof(*pcmd_obj)...) over
> > kzalloc(sizeof(struct cmd_obj)...)".
> >
> > According to the above "CHECK", use the preferred style in the first
> > kzalloc().
> >
> > Fixes: 79f712ea994d ("staging: r8188eu: Remove wrappers for kalloc() and
> kzalloc()")
> > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > ---
> >
> > v2->v3: Add the "Fixes:" tag, as requested by Greg Kroah-Hartman.
> >
> > v1->v2: Fix an error that I introduced with an incorrect copy-paste
> > of the sizeof() operator.
> >
> > drivers/staging/r8188eu/core/rtw_mlme_ext.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
>
> Hello Greg,
>
> I've noticed that you have already applied recent changes to drivers/staging
> up to the patches of November 6th, but my patch is not among them.
I have applied patches that are not targeted for 5.16-final, yes.
> This patch has already been acked by Larry and I'm not sure if I should send
> a v4 with his "Acked-by" tag or if you can add it by yourself when applying
> to your tree.
>
> Please let me know if there is something that prevents this patch to be
> applied. I have no problem in changing / adding whatever it is needed.
Nothing needs to be done, I am waiting for 5.16-rc1 to be released
before I pick up this patch, and others that will be targeted for
5.16-final. Only then will I queue them up, as the automated email you
should have gotten when you submitted the patch said would happen.
Just relax, there is no rush here :)
thanks,
greg k-h
On Sunday, November 7, 2021 3:17:19 PM CET Greg Kroah-Hartman wrote:
> No, your change will go to the staging-linus branch, as it needs to go
> into 5.16-final and get sent to Linus much sooner than 5.17-rc1, which
> is where things are being queued up in the staging-testing branch at the
> moment.
Oh, I didn't even remotely guess that this kinds of patches usually go to the
staging-linus branch so they get sent to Linus much sooner.
Thank you so much for your patience and for taking the time to explain the
workflow to me.
Fabio
>
> hope this helps,
>
> greg k-h
>