netdev - Re: [PATCH net] net: airoha: Take into account out-of-order tx completions in airoha_dev

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]

Message-ID: <aOo0woPiMxjABFv2@horms.kernel.org>
Date: Sat, 11 Oct 2025 11:43:14 +0100
From: Simon Horman <horms@...nel.org>
To: Lorenzo Bianconi <lorenzo@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: airoha: Take into account out-of-order tx
 completions in airoha_dev_xmit()

On Fri, Oct 10, 2025 at 07:21:43PM +0200, Lorenzo Bianconi wrote:
> Completion napi can free out-of-order tx descriptors if hw QoS is
> enabled and packets with different priority are queued to same DMA ring.
> Take into account possible out-of-order reports checking if the tx queue
> is full using circular buffer head/tail pointer instead of the number of
> queued packets.
> 
> Fixes: 23020f0493270 ("net: airoha: Introduce ethernet support for EN7581 SoC")
> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 833dd911980b3f698bd7e5f9fd9e2ce131dd5222..5e2ff52dba03a7323141fe9860fba52806279bd0 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -1873,6 +1873,19 @@ static u32 airoha_get_dsa_tag(struct sk_buff *skb, struct net_device *dev)
>  #endif
>  }
>  
> +static bool airoha_dev_is_tx_busy(struct airoha_queue *q, u32 nr_frags)
> +{
> +	u16 index = (q->head + nr_frags) % q->ndesc;
> +
> +	/* completion napi can free out-of-order tx descriptors if hw QoS is
> +	 * enabled and packets with different priorities are queued to the same
> +	 * DMA ring. Take into account possible out-of-order reports checking
> +	 * if the tx queue is full using circular buffer head/tail pointers
> +	 * instead of the number of queued packets.
> +	 */
> +	return index >= q->tail && (q->head < q->tail || q->head > index);

Hi Lorenzo,

I think there is a corner case here.
Perhaps they can't occur, but here goes.

Let us suppose that head is 1.
And the ring is completely full, so tail is 2.

Now, suppose nr_frags is ndesc - 1.
In this case the function above will return false. But the ring is full.

Ok, ndesc is actually 1024 and nfrags should never be close to that.
But the problem is general. And a perhaps more realistic example is:

  ndesc is 1024
  head is 1008
  The ring is full so tail is 1009
  (Or head is any other value that leaves less than 16 slots free)
  nr_frags is 16

airoha_dev_is_tx_busy() returns false, even though the ring is full.

Probably this has it's own problems. But if my reasoning above is correct
(is it?) then the following seems to address it by flattening and extending
the ring. Because what we are about is the relative value of head, index
and tail. Not the slots they occupy in the ring.

N.B: I tetsed the algorirthm with a quick implementation in user-space.
The following is, however, completely untested.

static bool airoha_dev_is_tx_busy(struct airoha_queue *q, u32 nr_frags)
{
	unsigned int tail = q->tail < q->head ? q->tail + q->ndesc : q->tail;
	unsigned int index = q->head + nr_frags;

	return index >= tail;
}

...