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

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

[Jinjie Ruan]

It is not allowed to call kfree_skb() from hardware interrupt
context or with interrupts being disabled. This patchset is
trying to use dev_kfree_skb_irq() or add all skb to a free list, then
free them after spin_unlock_irqrestore() at once.

Changes in v2:
- Combine the three patches into one patch set.
- Update the commit message and subject prefix.

Jinjie Ruan (3):
  staging: rtl8192e: rtl_core: Call dev_kfree_skb_irq() instead of
    kfree_skb() under spin_lock_irqsave()
  staging: rtl8192e: rtl_core: Do not call kfree_skb() under
    spin_lock_irqsave() for _rtl92e_irq()
  Staging: rtl8192e: rtllib_softmac: Do not call kfree_skb() under
    spin_lock_irqsave()

 drivers/staging/rtl8192e/rtl8192e/rtl_core.c | 24 ++++++++++++--------
 drivers/staging/rtl8192e/rtllib_softmac.c    |  6 ++++-
 2 files changed, 19 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH -next v2 1/3] staging: rtl8192e: rtl_core: Call dev_kfree_skb_irq() instead of kfree_skb() under spin_lock_irqsave()

[Jinjie Ruan]

It is not allowed to call kfree_skb() from hardware interrupt
context or with hardware interrupts being disabled.
So replace kfree_skb() with dev_kfree_skb_irq() under
spin_lock_irqsave(). Compile tested only.

Fixes: 94a799425eee ("From: wlanfae <wlanfae@realtek.com> [PATCH 1/8] rtl8192e: Import new version of driver from realtek")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: Larry Finger <Larry.Finger@lwfinger.net>
---
v2:
- Update the commit title and subject prefix.
---
 drivers/staging/rtl8192e/rtl8192e/rtl_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_core.c b/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
index 50eb8f3494ec..5a613b116925 100644
--- a/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
+++ b/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
@@ -1663,7 +1663,7 @@ void rtl92e_reset_desc_ring(struct net_device *dev)
 				dma_unmap_single(&priv->pdev->dev,
 						 entry->TxBuffAddr, skb->len,
 						 DMA_TO_DEVICE);
-				kfree_skb(skb);
+				dev_kfree_skb_irq(skb);
 				ring->idx = (ring->idx + 1) % ring->entries;
 			}
 			ring->idx = 0;
-- 
2.34.1

Re: [PATCH -next v2 1/3] staging: rtl8192e: rtl_core: Call dev_kfree_skb_irq() instead of kfree_skb() under spin_lock_irqsave()

[Greg KH]

On Fri, Aug 25, 2023 at 09:52:11AM +0800, Jinjie Ruan wrote:
> It is not allowed to call kfree_skb() from hardware interrupt
> context or with hardware interrupts being disabled.

Why not?  Seems to work so far :)

Are you fixing up all of the normal network drivers for this first?

thanks,

greg k-h

Re: [PATCH -next v2 1/3] staging: rtl8192e: rtl_core: Call dev_kfree_skb_irq() instead of kfree_skb() under spin_lock_irqsave()

[Ruan Jinjie]

On 2023/9/13 16:34, Greg KH wrote:
> On Fri, Aug 25, 2023 at 09:52:11AM +0800, Jinjie Ruan wrote:
>> It is not allowed to call kfree_skb() from hardware interrupt
>> context or with hardware interrupts being disabled.
> 
> Why not?  Seems to work so far :)

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  */


> 
> Are you fixing up all of the normal network drivers for this first?
> 
> thanks,
> 
> greg k-h

[PATCH -next v2 2/3] staging: rtl8192e: rtl_core: Do not call kfree_skb() under spin_lock_irqsave() for _rtl92e_irq()

[Jinjie Ruan]

It is not allowed to call kfree_skb() from hardware interrupt
context or with hardware interrupts being disabled. All the SKBs have
been dequeued from the old queue, so it's safe to enqueue these
SKBs to a free queue, then free them after spin_unlock_irqrestore()
at once. Compile tested only.

Fixes: 3d461c912462 ("rtl8192e: Split into two directories")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Update the commit title and subject prefix.
---
 drivers/staging/rtl8192e/rtl8192e/rtl_core.c | 22 ++++++++++++--------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_core.c b/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
index 5a613b116925..953a4faa9a49 100644
--- a/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
+++ b/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
@@ -1431,7 +1431,7 @@ static int _rtl92e_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	return ret;
 }
 
-static void _rtl92e_tx_isr(struct net_device *dev, int prio)
+static void _rtl92e_tx_isr(struct net_device *dev, struct sk_buff_head *free_list, int prio)
 {
 	struct r8192_priv *priv = rtllib_priv(dev);
 
@@ -1451,7 +1451,7 @@ static void _rtl92e_tx_isr(struct net_device *dev, int prio)
 		dma_unmap_single(&priv->pdev->dev, entry->TxBuffAddr,
 				 skb->len, DMA_TO_DEVICE);
 
-		kfree_skb(skb);
+		__skb_queue_tail(free_list, skb);
 	}
 	if (prio != BEACON_QUEUE)
 		tasklet_schedule(&priv->irq_tx_tasklet);
@@ -1971,12 +1971,14 @@ static irqreturn_t _rtl92e_irq(int irq, void *netdev)
 {
 	struct net_device *dev = netdev;
 	struct r8192_priv *priv = rtllib_priv(dev);
+	struct sk_buff_head free_list;
 	unsigned long flags;
 	u32 inta;
 
 	if (priv->irq_enabled == 0)
 		goto done;
 
+	skb_queue_head_init(&free_list);
 	spin_lock_irqsave(&priv->irq_th_lock, flags);
 
 	rtl92e_ack_irq(dev, &inta);
@@ -1997,7 +1999,7 @@ static irqreturn_t _rtl92e_irq(int irq, void *netdev)
 	}
 
 	if (inta  & IMR_MGNTDOK) {
-		_rtl92e_tx_isr(dev, MGNT_QUEUE);
+		_rtl92e_tx_isr(dev, &free_list, MGNT_QUEUE);
 		spin_unlock_irqrestore(&priv->irq_th_lock, flags);
 		if (priv->rtllib->ack_tx_to_ieee) {
 			if (_rtl92e_is_tx_queue_empty(dev)) {
@@ -2009,10 +2011,10 @@ static irqreturn_t _rtl92e_irq(int irq, void *netdev)
 	}
 
 	if (inta & IMR_COMDOK)
-		_rtl92e_tx_isr(dev, TXCMD_QUEUE);
+		_rtl92e_tx_isr(dev, &free_list, TXCMD_QUEUE);
 
 	if (inta & IMR_HIGHDOK)
-		_rtl92e_tx_isr(dev, HIGH_QUEUE);
+		_rtl92e_tx_isr(dev, &free_list, HIGH_QUEUE);
 
 	if (inta & IMR_ROK)
 		tasklet_schedule(&priv->irq_rx_tasklet);
@@ -2031,26 +2033,28 @@ static irqreturn_t _rtl92e_irq(int irq, void *netdev)
 
 	if (inta & IMR_BKDOK) {
 		priv->rtllib->link_detect_info.NumTxOkInPeriod++;
-		_rtl92e_tx_isr(dev, BK_QUEUE);
+		_rtl92e_tx_isr(dev, &free_list, BK_QUEUE);
 	}
 
 	if (inta & IMR_BEDOK) {
 		priv->rtllib->link_detect_info.NumTxOkInPeriod++;
-		_rtl92e_tx_isr(dev, BE_QUEUE);
+		_rtl92e_tx_isr(dev, &free_list, BE_QUEUE);
 	}
 
 	if (inta & IMR_VIDOK) {
 		priv->rtllib->link_detect_info.NumTxOkInPeriod++;
-		_rtl92e_tx_isr(dev, VI_QUEUE);
+		_rtl92e_tx_isr(dev, &free_list, VI_QUEUE);
 	}
 
 	if (inta & IMR_VODOK) {
 		priv->rtllib->link_detect_info.NumTxOkInPeriod++;
-		_rtl92e_tx_isr(dev, VO_QUEUE);
+		_rtl92e_tx_isr(dev, &free_list, VO_QUEUE);
 	}
 
 	spin_unlock_irqrestore(&priv->irq_th_lock, flags);
 
+	__skb_queue_purge(&free_list);
+
 done:
 
 	return IRQ_HANDLED;
-- 
2.34.1

[PATCH -next v2 3/3] Staging: rtl8192e: rtllib_softmac: Do not call kfree_skb() under spin_lock_irqsave()

[Jinjie Ruan]

It is not allowed to call kfree_skb() from hardware interrupt
context or with hardware interrupts being disabled. All the SKBs have
been dequeued from the old queue, so it's safe to enqueue these
SKBs to a free queue, then free them after spin_unlock_irqrestore()
at once. Compile tested only.

Fixes: 94a799425eee ("From: wlanfae <wlanfae@realtek.com> [PATCH 1/8] rtl8192e: Import new version of driver from realtek")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Update the commit title and subject prefix.
---
 drivers/staging/rtl8192e/rtllib_softmac.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
index 0e52b207942d..398fb354d342 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -2353,8 +2353,10 @@ void rtllib_softmac_xmit(struct rtllib_txb *txb, struct rtllib_device *ieee)
 	unsigned long flags;
 	int  i;
 	struct cb_desc *tcb_desc = NULL;
+	struct sk_buff_head free_list;
 	unsigned long queue_len = 0;
 
+	skb_queue_head_init(&free_list);
 	spin_lock_irqsave(&ieee->lock, flags);
 
 	/* called with 2nd parm 0, no tx mgmt lock required */
@@ -2382,7 +2384,7 @@ void rtllib_softmac_xmit(struct rtllib_txb *txb, struct rtllib_device *ieee)
 				skb_queue_tail(&ieee->skb_waitQ[queue_index],
 					       txb->fragments[i]);
 			else
-				kfree_skb(txb->fragments[i]);
+				__skb_queue_tail(&free_list, txb->fragments[i]);
 		} else {
 			ieee->softmac_data_hard_start_xmit(
 					txb->fragments[i],
@@ -2393,6 +2395,8 @@ void rtllib_softmac_xmit(struct rtllib_txb *txb, struct rtllib_device *ieee)
 	rtllib_txb_free(txb);
 
 	spin_unlock_irqrestore(&ieee->lock, flags);
+
+	__skb_queue_purge(&free_list);
 }
 
 void rtllib_reset_queue(struct rtllib_device *ieee)
-- 
2.34.1

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

[Philipp Hortmann]

On 8/25/23 03:52, Jinjie Ruan wrote:
> It is not allowed to call kfree_skb() from hardware interrupt
> context or with interrupts being disabled. This patchset is
> trying to use dev_kfree_skb_irq() or add all skb to a free list, then
> free them after spin_unlock_irqrestore() at once.
> 
> Changes in v2:
> - Combine the three patches into one patch set.
> - Update the commit message and subject prefix.
> 
> Jinjie Ruan (3):
>    staging: rtl8192e: rtl_core: Call dev_kfree_skb_irq() instead of
>      kfree_skb() under spin_lock_irqsave()
>    staging: rtl8192e: rtl_core: Do not call kfree_skb() under
>      spin_lock_irqsave() for _rtl92e_irq()
>    Staging: rtl8192e: rtllib_softmac: Do not call kfree_skb() under
>      spin_lock_irqsave()
> 
>   drivers/staging/rtl8192e/rtl8192e/rtl_core.c | 24 ++++++++++++--------
>   drivers/staging/rtl8192e/rtllib_softmac.c    |  6 ++++-
>   2 files changed, 19 insertions(+), 11 deletions(-)
> 

Patch number 01 is missing. Patch number 2 and 3 are OK
Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com>

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

[Dan Carpenter]

On Fri, Aug 25, 2023 at 07:47:44AM +0200, Philipp Hortmann wrote:
> On 8/25/23 03:52, Jinjie Ruan wrote:
> > It is not allowed to call kfree_skb() from hardware interrupt
> > context or with interrupts being disabled. This patchset is
> > trying to use dev_kfree_skb_irq() or add all skb to a free list, then
> > free them after spin_unlock_irqrestore() at once.
> > 
> > Changes in v2:
> > - Combine the three patches into one patch set.
> > - Update the commit message and subject prefix.
> > 
> > Jinjie Ruan (3):
> >    staging: rtl8192e: rtl_core: Call dev_kfree_skb_irq() instead of
> >      kfree_skb() under spin_lock_irqsave()
> >    staging: rtl8192e: rtl_core: Do not call kfree_skb() under
> >      spin_lock_irqsave() for _rtl92e_irq()
> >    Staging: rtl8192e: rtllib_softmac: Do not call kfree_skb() under
> >      spin_lock_irqsave()
> > 
> >   drivers/staging/rtl8192e/rtl8192e/rtl_core.c | 24 ++++++++++++--------
> >   drivers/staging/rtl8192e/rtllib_softmac.c    |  6 ++++-
> >   2 files changed, 19 insertions(+), 11 deletions(-)
> > 
> 
> Patch number 01 is missing. Patch number 2 and 3 are OK

I recieved it.  It's there on lore.  But I too am having some issues
with a bunch of dropped patches recently.  I don't know what caused it.

https://lore.kernel.org/all/20230825015213.2697347-2-ruanjinjie@huawei.com/

regards,
dan carpenter

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

[Dan Carpenter]

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()?

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

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

[Ruan Jinjie]

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