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

[Fabio M. De Francesco]

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