netdev - Re: [PATCH net v2] net: ks8851: Fix TX stall caused by TX buffer overrun

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] [day] [month] [year] [list]

Message-ID: <20231213162924.GH5817@kernel.org>
Date: Wed, 13 Dec 2023 16:29:24 +0000
From: Simon Horman <horms@...nel.org>
To: Ronald Wahl <rwahl@....de>
Cc: Ronald Wahl <ronald.wahl@...itan.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Ben Dooks <ben.dooks@...ethink.co.uk>,
	Tristram Ha <Tristram.Ha@...rochip.com>, netdev@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH net v2] net: ks8851: Fix TX stall caused by TX buffer
 overrun

On Tue, Dec 12, 2023 at 08:16:32PM +0100, Ronald Wahl wrote:
> From: Ronald Wahl <ronald.wahl@...itan.com>
> 
> There is a bug in the ks8851 Ethernet driver that more data is written
> to the hardware TX buffer than actually available. This is caused by
> wrong accounting of the free TX buffer space.
> 
> The driver maintains a tx_space variable that represents the TX buffer
> space that is deemed to be free. The ks8851_start_xmit_spi() function
> adds an SKB to a queue if tx_space is large enough and reduces tx_space
> by the amount of buffer space it will later need in the TX buffer and
> then schedules a work item. If there is not enough space then the TX
> queue is stopped.
> 
> The worker function ks8851_tx_work() dequeues all the SKBs and writes
> the data into the hardware TX buffer. The last packet will trigger an
> interrupt after it was send. Here it is assumed that all data fits into
> the TX buffer.
> 
> In the interrupt routine (which runs asynchronously because it is a
> threaded interrupt) tx_space is updated with the current value from the
> hardware. Also the TX queue is woken up again.
> 
> Now it could happen that after data was sent to the hardware and before
> handling the TX interrupt new data is queued in ks8851_start_xmit_spi()
> when the TX buffer space had still some space left. When the interrupt
> is actually handled tx_space is updated from the hardware but now we
> already have new SKBs queued that have not been written to the hardware
> TX buffer yet. Since tx_space has been overwritten by the value from the
> hardware the space is not accounted for.
> 
> Now we have more data queued then buffer space available in the hardware
> and ks8851_tx_work() will potentially overrun the hardware TX buffer. In
> many cases it will still work because often the buffer is written out
> fast enough so that no overrun occurs but for example if the peer
> throttles us via flow control then an overrun may happen.
> 
> This can be fixed in different ways. The most simple way would be to set
> tx_space to 0 before writing data to the hardware TX buffer preventing
> the queuing of more SKBs until the TX interrupt has been handled. I have
> chosen a slightly more efficient (and still rather simple) way and
> track the amount of data that is already queued and not yet written to
> the hardware. When new SKBs are to be queued the already queued amount
> of data is honoured when checking free TX buffer space.
> 
> I tested this with a setup of two linked KS8851 running iperf3 between
> the two in bidirectional mode. Before the fix I got a stall after some
> minutes. With the fix I saw now issues anymore after hours.
> 
> Fixes: 3ba81f3ece3c ("net: Micrel KS8851 SPI network driver")
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Paolo Abeni <pabeni@...hat.com>
> Cc: Ben Dooks <ben.dooks@...ethink.co.uk>
> Cc: Tristram Ha <Tristram.Ha@...rochip.com>
> Cc: netdev@...r.kernel.org
> Cc: stable@...r.kernel.org # 5.10+
> Signed-off-by: Ronald Wahl <ronald.wahl@...itan.com>

...

> diff --git a/drivers/net/ethernet/micrel/ks8851_spi.c b/drivers/net/ethernet/micrel/ks8851_spi.c

...

> @@ -310,6 +322,8 @@ static void ks8851_tx_work(struct work_struct *work)
>  	unsigned long flags;
>  	struct sk_buff *txb;
>  	bool last;
> +	unsigned short tx_space;
> +	unsigned int dequeued_len = 0;

Hi Ronald,

Please consider using reverse xmas tree - longest line to shortest -
for local variable declarations in Networking code.

> 
>  	kss = container_of(work, struct ks8851_net_spi, tx_work);
>  	ks = &kss->ks8851;
> @@ -320,6 +334,7 @@ static void ks8851_tx_work(struct work_struct *work)
>  	while (!last) {
>  		txb = skb_dequeue(&ks->txq);
>  		last = skb_queue_empty(&ks->txq);
> +		dequeued_len += calc_txlen(txb->len);

On the line below it is assumed that txb may be NULL.
But on the line above it is dereferenced unconditionally.
This seems inconsistent.

Flagged by Smatch.

> 
>  		if (txb) {
>  			ks8851_wrreg16_spi(ks, KS_RXQCR,

...