netdev - Re: [PATCH net v2 3/3] mISDN: hfcmulti: 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:48:35 -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
Subject: Re: [PATCH net v2 3/3] mISDN: hfcmulti: 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.
> 
> skb_queue_purge() is called under spin_lock_irqsave() in handle_dmsg()
> and hfcm_l1callback(), kfree_skb() is called in them, 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.
> 
> Fixes: af69fb3a8ffa ("Add mISDN HFC multiport driver")
> Signed-off-by: Yang Yingliang <yangyingliang@...wei.com>
> ---
>  drivers/isdn/hardware/mISDN/hfcmulti.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/isdn/hardware/mISDN/hfcmulti.c b/drivers/isdn/hardware/mISDN/hfcmulti.c
> index 4f7eaa17fb27..e840609c50eb 100644
> --- a/drivers/isdn/hardware/mISDN/hfcmulti.c
> +++ b/drivers/isdn/hardware/mISDN/hfcmulti.c
> @@ -3217,6 +3217,7 @@ static int
>  hfcm_l1callback(struct dchannel *dch, u_int cmd)
>  {
>  	struct hfc_multi	*hc = dch->hw;
> +	struct sk_buff_head	free_queue;
>  	u_long	flags;
>  
>  	switch (cmd) {
> @@ -3245,6 +3246,7 @@ hfcm_l1callback(struct dchannel *dch, u_int cmd)
>  		l1_event(dch->l1, HW_POWERUP_IND);
>  		break;
>  	case HW_DEACT_REQ:
> +		__skb_queue_head_init(&free_queue);
>  		/* start deactivation */
>  		spin_lock_irqsave(&hc->lock, flags);
>  		if (hc->ctype == HFC_TYPE_E1) {
> @@ -3264,20 +3266,21 @@ hfcm_l1callback(struct dchannel *dch, u_int cmd)
>  				plxsd_checksync(hc, 0);
>  			}
>  		}
> -		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);
>  		if (test_and_clear_bit(FLG_BUSY_TIMER, &dch->Flags))
>  			del_timer(&dch->timer);
>  		spin_unlock_irqrestore(&hc->lock, flags);
> +		__skb_queue_purge(&free_queue);
>  		break;
>  	case HW_POWERUP_REQ:
>  		spin_lock_irqsave(&hc->lock, flags);
> @@ -3384,6 +3387,9 @@ handle_dmsg(struct mISDNchannel *ch, struct sk_buff *skb)
>  	case PH_DEACTIVATE_REQ:
>  		test_and_clear_bit(FLG_L2_ACTIVATED, &dch->Flags);
>  		if (dch->dev.D.protocol != ISDN_P_TE_S0) {
> +			struct sk_buff_head free_queue;
> +
> +			__skb_queue_head_init(&free_queue);
>  			spin_lock_irqsave(&hc->lock, flags);
>  			if (debug & DEBUG_HFCMULTI_MSG)
>  				printk(KERN_DEBUG
> @@ -3405,14 +3411,14 @@ handle_dmsg(struct mISDNchannel *ch, struct sk_buff *skb)
>  				/* deactivate */
>  				dch->state = 1;
>  			}
> -			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);
> @@ -3424,6 +3430,7 @@ handle_dmsg(struct mISDNchannel *ch, struct sk_buff *skb)
>  #endif
>  			ret = 0;
>  			spin_unlock_irqrestore(&hc->lock, flags);
> +			__skb_queue_purge(&free_queue);
>  		} else
>  			ret = l1_event(dch->l1, hh->prim);
>  		break;

Looks good to me.

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