netdev - Re: [PATCH v2 1/1] net: macb: Avoid erroneously stopped TX ring.

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: <20230421195755.5e59a164@kernel.org>
Date:   Fri, 21 Apr 2023 19:57:55 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ingo Rohloff <ingo.rohloff@...terbach.com>
Cc:     Roman Gushchin <roman.gushchin@...ux.dev>,
        Lars-Peter Clausen <lars@...afoo.de>,
        robert.hancock@...ian.com, Nicolas.Ferre@...rochip.com,
        claudiu.beznea@...rochip.com, davem@...emloft.net,
        netdev@...r.kernel.org, tomas.melin@...sala.com
Subject: Re: [PATCH v2 1/1] net: macb: Avoid erroneously stopped TX ring.

On Fri, 21 Apr 2023 15:00:35 +0200 Ingo Rohloff wrote:
> The SW puts a frame to be transmitted into the TX descriptor ring with
> macb_start_xmit().
> The last step of this operation is, that the SW clears the TX_USED bit
> of the first descriptor it wrote.
> The HW already reached and read this descriptor with a set TX_USED bit.
> The SW sets the TSTART bit in the NCR register.
> This is a race condition:
> 1) Either the HW already has processed the descriptor and has stopped the
>    transmission, so the TGO bit in the TSR register is cleared.
> 2) The HW has read, but not yet processed the descriptor.
> In case 2) the HW ignores the TSTART trigger and stops the
> transmission a little bit later.
> 
> You now have got a TX descriptor in the TX ring which is ready (TX_USED
> bit cleared), but the hardware does not process the fresh descriptor,
> because it ignored the corresponding TSTART trigger.
> 
> This patch checks if the hardware is processing the same descriptor, where
> the TX_USED bit was just cleared.
> If this is the case this patch ensures that the TSTART trigger is repeated
> if needed.

Were you able to measure the performance impact of the workaround?
Doesn't name a difference?

> +static void macb_fix_tstart_race(unsigned int tx_head,
> +				 struct macb *bp, struct macb_queue *queue)
> +{
> +	u32 macb_tsr, macb_tbqp, macb_ncr;
> +
> +	/* Controller was (probably) active when we wrote TSTART.
> +	 * This might be a race condition.
> +	 * Ensure TSTART is not ignored.
> +	 */
> +	for (;;) {
> +		macb_tbqp = queue_readl(queue, TBQP);
> +		macb_tbqp = macb_tbqp - lower_32_bits(queue->tx_ring_dma);
> +		macb_tbqp = macb_tbqp / macb_dma_desc_get_size(bp);
> +		macb_tbqp = macb_tx_ring_wrap(bp, macb_tbqp);
> +		if (tx_head != macb_tbqp) {
> +			/* Controller is working on different descriptor.
> +			 * There should be no problem.
> +			 */
> +			break;
> +		}
> +
> +		/* Controller works on the descriptor we just wrote.
> +		 * TSTART might not have worked. Check for TGO again.
> +		 */
> +		macb_tsr = macb_readl(bp, TSR);
> +		if (!(macb_tsr & MACB_BIT(TGO))) {
> +			/* Controller stopped... write TSTART again.
> +			 */
> +			macb_ncr = macb_readl(bp, NCR);
> +			macb_ncr = macb_ncr | MACB_BIT(TSTART);
> +			macb_writel(bp, NCR, macb_ncr);
> +			break;
> +		}
> +		/* Controller might stop or process our descriptor.
> +		 * Check again.
> +		 */

We should add (1) cpu_relax(), (2) a statistic which would count that
the condition was encountered, and (3) some form of limit on the loop,
so that we don't hang the host with irqs off if the NIC goes bad.

> +	}
> +}

>  	spin_lock_irq(&bp->lock);
> +	macb_tsr = macb_readl(bp, TSR);

-- 
pw-bot: cr