Re: [PATCH -next v2 0/3] staging: rtl8192e: Do not call kfree_skb() under spin_lock_irqsave()

[Ruan Jinjie <ruanjinjie@huawei.com>]

On 2023/9/5 19:59, Dan Carpenter wrote:
> Added netdev because they're really the experts.
> 
> On Fri, Aug 25, 2023 at 09:52:10AM +0800, Jinjie Ruan wrote:
>> It is not allowed to call kfree_skb() from hardware interrupt
>> context or with interrupts being disabled.
> 
> There are no comments which say that this is not allowed.  I have
> reviewed the code to see why it's not allowed.  The only thing I can
> see is that maybe the skb->destructor(skb); in skb_release_head_state()
> sleeps?  Or possibly the uarg->callback() in skb_zcopy_clear()?

The commit e6247027e517 ("net: introduce dev_consume_skb_any()") has the
below comment:

3830 /*
3831  * It is not allowed to call kfree_skb() or consume_skb() from hardware
3832  * interrupt context or with hardware interrupts being disabled.
3833  * (in_hardirq() || irqs_disabled())
3834  *
3835  * We provide four helpers that can be used in following contexts :
3836  *
3837  * dev_kfree_skb_irq(skb) when caller drops a packet from irq context,
3838  *  replacing kfree_skb(skb)
3839  *
3840  * dev_consume_skb_irq(skb) when caller consumes a packet from irq
context.
3841  *  Typically used in place of consume_skb(skb) in TX completion path
3842  *
3843  * dev_kfree_skb_any(skb) when caller doesn't know its current irq
context,
3844  *  replacing kfree_skb(skb)
3845  *
3846  * dev_consume_skb_any(skb) when caller doesn't know its current
irq context,
3847  *  and consumed a packet. Used in place of consume_skb(skb)
3848  */

> 
> Can you comment more on why this isn't allowed?  Was this detected at
> runtime?  Do you have a stack trace?
> 
> Once I know more I can add this to Smatch so that it is detected
> automatically using static analysis.
> 
> regards,
> dan carpenter
> 
>