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().
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
v1->v2: Fix an overlooked error due to 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 11/1/21 09:27, 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().
>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
>
> v1->v2: Fix an overlooked error due to an incorrect copy-paste
> of the sizeof() operator.
I am happy that you caught the error before it destroyed every instance of
r8188eu. Incidentally, I disagree with checkpatch in that I think that
sizeof(struct foo) is more descriptive than sizeof(*bar). If I wanted to check
the resulting value of the sizeof(), the second form requires an additional
step. It probably does not matter much to the compiler, but when I have to do it
manually, the extra effort is not negligible.
Even though I disagree with the philosophy,
Acked-by: Larry Finger <[email protected]>
>
> 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;
>
On Mon, Nov 01, 2021 at 03:27:32PM +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().
>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
>
> v1->v2: Fix an overlooked error due to an incorrect copy-paste
> of the sizeof() operator.
What commit does this fix?
thanks,
greg k-h
On Monday, November 1, 2021 4:11:26 PM CET Larry Finger wrote:
> On 11/1/21 09:27, 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".
> >
> > []
> >
> I am happy that you caught the error before it destroyed every instance of
> r8188eu.
I don't think so, since we have run this driver with no problems at all :)
SAC bugs can potentially cause serious system hangs at runtime, but they do
not always cause problems in real execution as you have noticed here with
this driver. We have used and tested it hundreds of times with no problems.
> Incidentally, I disagree with checkpatch in that I think that
> sizeof(struct foo) is more descriptive than sizeof(*bar).
I agree with you in full, but I felt that I had to change it just because of
the warning output by that tool. I don't like to have my patches discarded
because they don't fix checkpatch warnings or introduce new ones.
> If I wanted to check
> the resulting value of the sizeof(), the second form requires an additional
> step. It probably does not matter much to the compiler, but when I have to
do it
> manually, the extra effort is not negligible.
>
> Even though I disagree with the philosophy,
>
> Acked-by: Larry Finger <[email protected]>
>
Thanks for your "Acked-by" tag.
Fabio
On Mon, 2021-11-01 at 17:30 +0100, Fabio M. De Francesco wrote:
> On Monday, November 1, 2021 4:11:26 PM CET Larry Finger wrote:
> > Incidentally, I disagree with checkpatch in that I think that
> > sizeof(struct foo) is more descriptive than sizeof(*bar).
> I agree with you in full
It's not checkpatch in particular, it's from coding-style
The preferred form for passing a size of a struct is the following:
.. code-block:: c
p = kmalloc(sizeof(*p), ...);
On Monday, November 1, 2021 4:18:03 PM CET Greg Kroah-Hartman wrote:
> On Mon, Nov 01, 2021 at 03:27:32PM +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().
> >
> > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > ---
> >
> > v1->v2: Fix an overlooked error due to an incorrect copy-paste
> > of the sizeof() operator.
>
> What commit does this fix?
>
> thanks,
>
> greg k-h
>
Sorry, Greg. Please let me know if I understand correctly what you are asking
for...
In v1 I introduced a silly error while copy-pasting "sizeof()" and then I
fixed it in v2.
I think that you mean that I should reword the list of changes from v1
because I'm not explaining properly why I submitted v2.
Is my understanding correct? If so, I have no problem in submitting v3.
Thank you in advance,
Fabio
P.S.: I had to resend this email and I want apologize for the noise. It seems
that it contained HTML parts and for this reasons it was rejected by the
relevant lists.
On Mon, Nov 01, 2021 at 05:43:08PM +0100, Fabio M. De Francesco wrote:
> On Monday, November 1, 2021 4:18:03 PM CET Greg Kroah-Hartman wrote:
> > On Mon, Nov 01, 2021 at 03:27:32PM +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().
> > >
> > > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > > ---
> > >
> > > v1->v2: Fix an overlooked error due to an incorrect copy-paste
> > > of the sizeof() operator.
> >
> > What commit does this fix?
> >
> > thanks,
> >
> > greg k-h
> >
> Sorry, Greg. Please let me know if I understand correctly what you are asking
> for...
>
> In v1 I introduced a silly error while copy-pasting "sizeof()" and then I
> fixed it in v2.
>
> I think that you mean that I should reword the list of changes from v1
> because I'm not explaining properly why I submitted v2.
>
> Is my understanding correct? If so, I have no problem in submitting v3.
Sorry, no, I mean what commit in the kernel tree is this patch "fixing"?
You should have a "Fixes: " tag in the signed-off-by area of the
changelog so that we know where the problem you are resolving here
originated.
thanks,
greg k-h