[PATCH net] net: sunhme: fix infinite loop in unmap_partial_tx_skb()

[PATCH net] net: sunhme: fix infinite loop in unmap_partial_tx_skb()

[Denis Arefev]

unmap_partial_tx_skb() walks the TX ring entries between
first_entry and entry to release the DMA mappings of the
fragments that were mapped successfully before a mapping error,
but it never advances first_entry inside the loop. Whenever the
failed fragment is not the first one, entry is ahead of
first_entry and the loop condition first_entry != entry never
becomes false, so the function spins forever.

The function is called from happy_meal_start_xmit() with
hp->happy_lock held and interrupts disabled, so the infinite
loop leads to a hard lockup.

Advance first_entry with NEXT_TX() at the end of the loop body.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: ec1f1276022e ("sunhme: Add DMA mapping error checks.")
Cc: stable@vger.kernel.org
Signed-off-by: Denis Arefev <arefev@swemel.ru>
---
 drivers/net/ethernet/sun/sunhme.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index a70f704eb9f3..544eb43af125 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -1965,6 +1965,7 @@ static void unmap_partial_tx_skb(struct happy_meal *hp, u32 first_mapping,
 		len = hme_read_desc32(hp, &this->tx_flags);
 		len &= TXFLAG_SIZE;
 		dma_unmap_page(hp->dma_dev, addr, len, DMA_TO_DEVICE);
+		first_entry = NEXT_TX(first_entry);
 	}
 }
 
-- 
2.43.0

Re: [PATCH net] net: sunhme: fix infinite loop in unmap_partial_tx_skb()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: sunhme: fix infinite loop in unmap_partial_tx_skb()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net: sunhme: MAC processes unmapped memory due to stale
  TXFLAG_OWN

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  makes it reachable.

--

net: sunhme: fix infinite loop in unmap_partial_tx_skb()

This commit fixes the loop in unmap_partial_tx_skb(), which never advanced
first_entry. Before the fix, a DMA mapping failure on any fragment after
the first one made happy_meal_start_xmit() spin forever with
hp->happy_lock held. The fix adds NEXT_TX() at the end of the loop body.

> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> index a70f704eb9f3f..544eb43af1258 100644
> --- a/drivers/net/ethernet/sun/sunhme.c
> +++ b/drivers/net/ethernet/sun/sunhme.c
> @@ -1965,6 +1965,7 @@ static void unmap_partial_tx_skb(struct happy_meal *hp, u32 first_mapping,
>  		len = hme_read_desc32(hp, &this->tx_flags);
>  		len &= TXFLAG_SIZE;
>  		dma_unmap_page(hp->dma_dev, addr, len, DMA_TO_DEVICE);
> +		first_entry = NEXT_TX(first_entry);

[Severity: High]
This isn't a bug introduced by this patch, but now that the error path
no longer locks up, it becomes reachable. It came in with ec1f1276022e
together with the missing NEXT_TX().

Once the fragment mappings are undone here, are those descriptors still
owned by the hardware?

happy_meal_start_xmit() sets TXFLAG_OWN in tx_flags and writes each
fragment descriptor with hme_write_txd() as soon as skb_frag_dma_map()
succeeds. Only the head descriptor at first_entry is held back. If a
later fragment fails to map, unmap_partial_tx_skb() calls
dma_unmap_page() on the fragment entries that were already staged, but
never rewrites them. Each one keeps TXFLAG_OWN, its length and a
tx_addr that is no longer mapped.

The out_dma_error path then leaves hp->tx_new at first_entry, sets
hp->tx_skbs[hp->tx_new] to NULL and frees the skb with
dev_kfree_skb_any(), which releases the fragment pages or swiotlb
slots.

If the next transmit is a linear skb such as a TCP ACK, it only writes
first_entry with OWN|SOP|EOP and kicks the engine with ETX_TP_DMAWAKEUP.
Could the MAC then move on to first_entry + 1, find OWN still set, and
DMA from the unmapped bus address? Depending on the platform that would
be an IOMMU fault, a TX DMA error and chip reset, or freed or reused
memory sent onto the wire. The stale chain also has no SOP, and no EOP
on its last descriptor.

After that the chip's ring position would be ahead of hp->tx_new.
Descriptors the driver later writes into those slots would sit behind
the hardware, and happy_meal_tx() stops at the first entry that still
has TXFLAG_OWN set. Would that stall TX completions until the ring
wraps or the TX watchdog resets the chip?

Should unmap_partial_tx_skb() also clear each descriptor after
unmapping it, for example with hme_write_txd(hp, this, 0, 0)? That
could also be a separate follow-up if you prefer to keep this fix
minimal for stable.

[Severity: High]

This isn't a bug introduced by this patch, but the patch makes it reachable
now that this error path returns instead of locking up. It came in with
ec1f1276022e together with the loop bug.

Once the fragment mappings are undone, are these descriptors still owned
by the hardware?

In happy_meal_start_xmit(), tx_flags always includes TXFLAG_OWN. Each
fragment is written to the ring as soon as it is mapped, and only the head
descriptor is held back:

drivers/net/ethernet/sun/sunhme.c:happy_meal_start_xmit() {
    ...
	tx_flags = TXFLAG_OWN;
    ...
			this_txflags = tx_flags;
			if (frag == skb_shinfo(skb)->nr_frags - 1)
				this_txflags |= TXFLAG_EOP;
			hme_write_txd(hp, &hp->happy_block->happy_meal_txd[entry],
				      (this_txflags | (len & TXFLAG_SIZE)),
				      mapping);
    ...
}

If fragment k+1 (with k >= 1) fails skb_frag_dma_map(),
unmap_partial_tx_skb() calls dma_unmap_page() on entries first_entry+1
through first_entry+k. It never rewrites those descriptors, so each one
keeps TXFLAG_OWN, its length and its now-unmapped tx_addr.

The error path then leaves hp->tx_new at first_entry and frees the skb,
which releases the fragment pages or swiotlb slots:

out_dma_error:
	hp->tx_skbs[hp->tx_new] = NULL;
	spin_unlock_irq(&hp->happy_lock);

	dev_kfree_skb_any(skb);

Suppose the next transmit is a linear skb, such as a TCP ACK. It writes
only first_entry with OWN|SOP|EOP and then kicks the DMA engine:

	hme_write32(hp, hp->etxregs + ETX_PENDING, ETX_TP_DMAWAKEUP);

Could the device then move on to first_entry+1, find OWN still set, and
DMA from the unmapped bus address? Depending on the platform, that would
mean an IOMMU fault, or freed or reused memory sent onto the wire. The
stale descriptors also form a chain with no SOP, and no EOP on the last
staged fragment.

After that, the device's ring position would be ahead of the driver's.
Descriptors the driver later writes into those slots would sit behind the
device. happy_meal_tx() stops at the first one with OWN still set:

		flags = hme_read_desc32(hp, &this->tx_flags);
		if (flags & TXFLAG_OWN)
			break;

Would that stall TX completions until the ring wraps or the TX watchdog
resets the chip?

Should unmap_partial_tx_skb() also clear each descriptor after unmapping
it, for example with hme_write_txd(hp, this, 0, 0)?

>  	}
>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013103228.37430-1-arefev%40swemel.ru