Re: [PATCH] net: atl1c: fix soft lockup on out-of-range tpd_cons read

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: atl1c: fix soft lockup on out-of-range tpd_cons read

This adds a bounds check on the TX consumer index read from the hardware
in atl1c_clean_tx(), so that an out-of-range value (reported as 0xffff)
is treated as "nothing to clean" rather than driving the cleanup loop
forever.

> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index 7efa3fc257b39..e58f1d2c26bdf 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -1602,6 +1602,9 @@ static int atl1c_clean_tx(struct napi_struct *napi, int budget)
>  	AT_READ_REGW(&adapter->hw, atl1c_qregs[tpd_ring->num].tpd_cons,
>  		     &hw_next_to_clean);
>  

[Severity: Medium]
Should this carry a Fixes: tag and a Cc: stable@vger.kernel.org?

git blame puts the unbounded loop in atl1c_clean_tx() at commit
43250ddd75a35d ("atl1c: Atheros L1C Gigabit Ethernet driver"), and the
per-queue register form of the read at commit 057f4af2b1716f, so all
stable trees appear to be affected:

Fixes: 43250ddd75a35d ("atl1c: Atheros L1C Gigabit Ethernet driver")

The loop really cannot terminate on its own: tpd_ring->count is fixed at
1024 in atl1c_setup_ring_resources() and the driver has no ethtool
ringparam callback, so a 0xffff read never matches any wrapped
next_to_clean value, and atl1c_clean_buffer() returns immediately once
the entries carry ATL1C_BUFFER_FREE.

Could the changelog also say how the 0xffff value was observed - kernel
version, hardware, and the log or reproducer? That makes the hardware
claim easier to confirm.

[Severity: High]
This isn't a bug introduced by this patch, but the sibling Atheros
drivers have the same loop and are left untouched here. Were they
audited?

atl1e_clean_tx_irq() in drivers/net/ethernet/atheros/atl1e/atl1e_main.c
uses the same accessor and the same modulo-count loop:

	u16 hw_next_to_clean = AT_READ_REGW(&adapter->hw, REG_TPD_CONS_IDX);
	u16 next_to_clean = atomic_read(&tx_ring->next_to_clean);

	while (next_to_clean != hw_next_to_clean) {

An all-ones read from a dead or resetting PCIe link is generic bus
behaviour, so this spins forever in softirq context on atl1e too.

atl1_intr_tx() in drivers/net/ethernet/atheros/atlx/atl1.c has the same
shape, driven by the device-written CMB field:

	cmb_tpd_next_to_clean = le16_to_cpu(adapter->cmb.cmb->tpd_cons_idx);

	while (cmb_tpd_next_to_clean != sw_tpd_next_to_clean) {

Would a note in the changelog about these two, or matching patches, make
sense?

> +	if (unlikely(hw_next_to_clean >= tpd_ring->count))
> +		hw_next_to_clean = next_to_clean;
> +
>  	while (next_to_clean != hw_next_to_clean) {
>  		buffer_info = &tpd_ring->buffer_info[next_to_clean];
>  		if (buffer_info->skb) {

[Severity: High]
This is a pre-existing issue and not introduced by this patch, but the
"while the PCIe link/MAC is resetting" scenario named in the changelog
looks like it also has an unsynchronized reset-versus-poll window that
this clamp does not close.

atl1c_common_task() handles the link change by masking interrupts and
then resetting the rings from process context:

	if (test_and_clear_bit(ATL1C_WORK_EVENT_LINK_CHANGE,
		&adapter->work_event)) {
		atl1c_irq_disable(adapter);
		atl1c_check_link_status(adapter);
		atl1c_irq_enable(adapter);
	}

atl1c_irq_disable() only masks IMR and waits for the hard IRQ handler:

	atomic_inc(&adapter->irq_sem);
	AT_WRITE_REG(&adapter->hw, REG_IMR, 0);
	AT_WRITE_REG(&adapter->hw, REG_ISR, ISR_DIS_INT);
	AT_WRITE_FLUSH(&adapter->hw);
	synchronize_irq(adapter->pdev->irq);

There is no napi_disable() or napi_synchronize() on this path, unlike
atl1c_down(). On link down, atl1c_check_link_status() then goes on to
atl1c_reset_mac(), atl1c_reset_dma_ring() and atl1c_configure() with all
TX NAPI instances still enabled.

atl1c_reset_dma_ring() -> atl1c_clean_tx_ring() walks every entry and
resets the index:

	for (index = 0; index < ring_count; index++) {
		buffer_info = &tpd_ring->buffer_info[index];
		atl1c_clean_buffer(pdev, buffer_info, 0);
	}
	...
	atomic_set(&tpd_ring->next_to_clean, 0);

Meanwhile atl1c_clean_tx() calls atl1c_clean_buffer() on the same
entries and writes its entry-time snapshot back:

	atl1c_clean_buffer(pdev, buffer_info, budget);
	if (++next_to_clean == tpd_ring->count)
		next_to_clean = 0;
	atomic_set(&tpd_ring->next_to_clean, next_to_clean);

The only guard against releasing a buffer twice is a plain read of a u16
field in atl1c_clean_buffer():

	if (buffer_info->flags & ATL1C_BUFFER_FREE)
		return;

Can both contexts pass that test for the same buffer_info and end up
doing two dma_unmap_single() and two napi_consume_skb() calls on it?
And can the poll's atomic_set() overwrite the reset path's
next_to_clean = 0, leaving the software index out of sync with the
freshly reset hardware?

Would adding napi_disable()/napi_synchronize() around the reset in the
link-change path be the right complement to this clamp? The clamp still
looks needed on its own, since surprise removal produces all-ones reads
that NAPI synchronization cannot prevent.
-- 
pw-bot: cr