[PATCH v2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context

[PATCH v2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context

[Fabio M. De Francesco]

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 <fmdefrancesco@gmail.com>
---

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

Re: [PATCH v2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context

[Larry Finger]

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 <fmdefrancesco@gmail.com>
> ---
> 
> 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 <Larry.Finger@lwfinger.net>

> 
>   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;
> 

Re: [PATCH v2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context

[Fabio M. De Francesco]

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 <Larry.Finger@lwfinger.net>
> 

Thanks for your "Acked-by" tag.

Fabio

Re: [PATCH v2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context

[Joe Perches]

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), ...);

Re: [PATCH v2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context

[Greg Kroah-Hartman]

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 <fmdefrancesco@gmail.com>
> ---
> 
> 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

Re: [PATCH v2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context

[Fabio M. De Francesco]

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 <fmdefrancesco@gmail.com>
> > ---
> > 
> > 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.

Re: [PATCH v2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context

[Greg Kroah-Hartman]

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 <fmdefrancesco@gmail.com>
> > > ---
> > > 
> > > 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