netdev - Re: [PATCH] mctp-i2c: increase the MCTP_I2C_TX_WORK

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 for Android: free password hash cracker in your pocket

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

Message-ID: <39c213c17712fdc8877b4277b430bfded71c3cca.camel@codeconstruct.com.au>
Date: Fri, 17 Nov 2023 08:50:18 -0600
From: Matt Johnston <matt@...econstruct.com.au>
To: Jeremy Kerr <jk@...econstruct.com.au>, Jinliang Wang
 <jinliangw@...gle.com>
Cc: William Kennington <wak@...gle.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mctp-i2c: increase the MCTP_I2C_TX_WORK_LEN to 500

Hi,

On Fri, 2023-11-17 at 15:29 +0800, Jeremy Kerr wrote:
> 	spin_lock_irqsave(&midev->tx_queue.lock, flags);
> 	if (skb_queue_len(&midev->tx_queue) >= MCTP_I2C_TX_WORK_LEN) {
> 		netif_stop_queue(dev);
> 		spin_unlock_irqrestore(&midev->tx_queue.lock, flags);
> 		netdev_err(dev, "BUG! Tx Ring full when queue awake!\n");
> 		return NETDEV_TX_BUSY;
> 	}
> 
> 	__skb_queue_tail(&midev->tx_queue, skb);
> 	if (skb_queue_len(&midev->tx_queue) == MCTP_I2C_TX_WORK_LEN)  // normal stop
> 		netif_stop_queue(dev);
> 	spin_unlock_irqrestore(&midev->tx_queue.lock, flags);
> 
> What looks like has happened here:
> 
>  1) we have TX_WORK_LEN-1 packets queued
>  2) we release a flow, which queues the "marker" skb. the tx_queue now
>     has TX_WORK_LEN items
>  3) we queue another packet, ending up with TX_WORK_LEN+1 in the queue
>  4) the == TX_WORK_LEN test fails, so we dont do a netif_stop_queue()
> 
> A couple of potential fixes:
> 
>  * We do the check and conditional netif_stop_queue() in (2)
>  * We change the check there to be `>= MCTP_I2C_TX_WORK_LEN`

My inclination would be to change the second comparison (the normal stop
condition) to 

    /* -1 to allow space for an additional unlock_marker skb */
    if (skb_queue_len(&midev->tx_queue) >= MCTP_I2C_TX_WORK_LEN-1)

Cheers,
Matt