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
