netdev - Re: [PATCH net v2 1/3] mISDN: hfcsusb: don't call dev_kfree_skb/kfree_skb() under spin_lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 12 Dec 2022 13:45:57 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Yang Yingliang <yangyingliang@...wei.com>, netdev@...r.kernel.org
Cc:     isdn@...ux-pingi.de, davem@...emloft.net, kuba@...nel.org,
        jiri@...nulli.us, Martin Bachem <m.bachem@....de>
Subject: Re: [PATCH net v2 1/3] mISDN: hfcsusb: don't call
 dev_kfree_skb/kfree_skb() under spin_lock_irqsave()

On Mon, 2022-12-12 at 16:41 +0800, Yang Yingliang wrote:
> It is not allowed to call kfree_skb() or consume_skb() from hardware
> interrupt context or with hardware interrupts being disabled.
> 
> It should use dev_kfree_skb_irq() or dev_consume_skb_irq() instead.
> The difference between them is free reason, dev_kfree_skb_irq() means
> the SKB is dropped in error and dev_consume_skb_irq() means the SKB
> is consumed in normal.
> 
> skb_queue_purge() is called under spin_lock_irqsave() in hfcusb_l2l1D(),
> kfree_skb() is called in it, to fix this, use skb_queue_splice_init()
> to move the dch->squeue to a free queue, also enqueue the tx_skb and
> rx_skb, at last calling __skb_queue_purge() to free the SKBs afer unlock.
> 
> In tx_iso_complete(), dev_kfree_skb() is called to consume the transmitted
> SKB, so replace it with dev_consume_skb_irq().
> 
> Fixes: 69f52adb2d53 ("mISDN: Add HFC USB driver")
> Signed-off-by: Yang Yingliang <yangyingliang@...wei.com>
> ---
>  drivers/isdn/hardware/mISDN/hfcsusb.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/isdn/hardware/mISDN/hfcsusb.c b/drivers/isdn/hardware/mISDN/hfcsusb.c
> index 651f2f8f685b..1efd17979f24 100644
> --- a/drivers/isdn/hardware/mISDN/hfcsusb.c
> +++ b/drivers/isdn/hardware/mISDN/hfcsusb.c
> @@ -326,20 +326,24 @@ hfcusb_l2l1D(struct mISDNchannel *ch, struct sk_buff *skb)
>  		test_and_clear_bit(FLG_L2_ACTIVATED, &dch->Flags);
>  
>  		if (hw->protocol == ISDN_P_NT_S0) {
> +			struct sk_buff_head free_queue;
> +
> +			__skb_queue_head_init(&free_queue);
>  			hfcsusb_ph_command(hw, HFC_L1_DEACTIVATE_NT);
>  			spin_lock_irqsave(&hw->lock, flags);
> -			skb_queue_purge(&dch->squeue);
> +			skb_queue_splice_init(&dch->squeue, &free_queue);
>  			if (dch->tx_skb) {
> -				dev_kfree_skb(dch->tx_skb);
> +				__skb_queue_tail(&free_queue, dch->tx_skb);
>  				dch->tx_skb = NULL;
>  			}
>  			dch->tx_idx = 0;
>  			if (dch->rx_skb) {
> -				dev_kfree_skb(dch->rx_skb);
> +				__skb_queue_tail(&free_queue, dch->rx_skb);
>  				dch->rx_skb = NULL;
>  			}
>  			test_and_clear_bit(FLG_TX_BUSY, &dch->Flags);
>  			spin_unlock_irqrestore(&hw->lock, flags);
> +			__skb_queue_purge(&free_queue);
>  #ifdef FIXME
>  			if (test_and_clear_bit(FLG_L1_BUSY, &dch->Flags))
>  				dchannel_sched_event(&hc->dch, D_CLEARBUSY);
> @@ -1330,7 +1334,7 @@ tx_iso_complete(struct urb *urb)
>  					printk("\n");
>  				}
>  
> -				dev_kfree_skb(tx_skb);
> +				dev_consume_skb_irq(tx_skb);
>  				tx_skb = NULL;
>  				if (fifo->dch && get_next_dframe(fifo->dch))
>  					tx_skb = fifo->dch->tx_skb;

Looks good to me.

Reviewed-by: Alexander Duyck <alexanderduyck@...com>