[PATCH net] net: 8139cp: close race between NAPI completion and Rx unmask

[PATCH net] net: 8139cp: close race between NAPI completion and Rx unmask

[Colton Pawielski via B4 Relay]

From: Colton Pawielski <colton.pawielski@gmail.com>

cp_rx_poll() calls napi_complete_done() and only then takes cp->lock
with interrupts disabled to unmask the Rx interrupts.  An interrupt
that is not masked while polling (TxOK, TxErr, ...) can arrive in
between.  cp_interrupt() then sees the Rx status bits, which are still
set, finds NAPI no longer scheduled, masks Rx and schedules NAPI again.
cp_rx_poll() then carries on and unmasks Rx anyway.

From that point Rx interrupts are enabled while NAPI is scheduled:
cp_interrupt() leaves the Rx bits for the poll routine, does not mask
them because napi_schedule_prep() fails, and the level-triggered
interrupt fires again as soon as it returns.  On an SH7751 LANDISK
under combined Tx/Rx load this interrupt storm was captured with a RAM
trace right before the board locked up hard.

Do the completion and the unmask under the lock, as 8139too does.

Fixes: ab1e7e1d2666 ("8139cp: use napi_complete_done()")
Signed-off-by: Colton Pawielski <colton.pawielski@gmail.com>
---
An AI coding assistant (Claude Opus 5.5, in Claude Code) did most of the
debugging: it instrumented the driver with a RAM-backed trace of register
accesses and IRQ entry/exit that survives the power cycle, and decoded
the trace after the hang.

Tested on three I-O DATA LANDISK boards (SH7751R, RTL8139C+ rev 0x20).
Without this patch, pulling 300 MB over ssh while reading the disk
(concurrent IDE and network DMA) hard-hung the board in 7 of 8 runs.
With it, that test passed 8/8, 3/3 and 3/3 on the three boards, and
copying the full rootfs and multi-GB test files over ssh also completed
without problems.
---
 drivers/net/ethernet/realtek/8139cp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 9016527e229a..a9334d3a0f7d 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -554,11 +554,12 @@ static int cp_rx_poll(struct napi_struct *napi, int budget)
 	/* if we did not reach work limit, then we're done with
 	 * this round of polling
 	 */
-	if (rx < budget && napi_complete_done(napi, rx)) {
+	if (rx < budget) {
 		unsigned long flags;
 
 		spin_lock_irqsave(&cp->lock, flags);
-		cpw16_f(IntrMask, cp_intr_mask);
+		if (napi_complete_done(napi, rx))
+			cpw16_f(IntrMask, cp_intr_mask);
 		spin_unlock_irqrestore(&cp->lock, flags);
 	}
 

---
base-commit: 0df412012a26389582b120844d098e1e961f10a6
change-id: 20261021-b4-8139cp-napi-8e139f232736

Best regards,
--  
Colton Pawielski <colton.pawielski@gmail.com>

Re: [PATCH net] net: 8139cp: close race between NAPI completion and Rx unmask

[Daniel Palmer]

Hi Colton,

On Thu, 22 Oct 2026 at 08:12, Colton Pawielski via B4 Relay
<devnull+colton.pawielski.gmail.com@kernel.org> wrote:
>
> From: Colton Pawielski <colton.pawielski@gmail.com>
>
> cp_rx_poll() calls napi_complete_done() and only then takes cp->lock
> with interrupts disabled to unmask the Rx interrupts.  An interrupt
> that is not masked while polling (TxOK, TxErr, ...) can arrive in
> between.  cp_interrupt() then sees the Rx status bits, which are still
> set, finds NAPI no longer scheduled, masks Rx and schedules NAPI again.
> cp_rx_poll() then carries on and unmasks Rx anyway.
>
> From that point Rx interrupts are enabled while NAPI is scheduled:
> cp_interrupt() leaves the Rx bits for the poll routine, does not mask
> them because napi_schedule_prep() fails, and the level-triggered
> interrupt fires again as soon as it returns.  On an SH7751 LANDISK
> under combined Tx/Rx load this interrupt storm was captured with a RAM
> trace right before the board locked up hard.
>
> Do the completion and the unmask under the lock, as 8139too does.
>
> Fixes: ab1e7e1d2666 ("8139cp: use napi_complete_done()")
> Signed-off-by: Colton Pawielski <colton.pawielski@gmail.com>

I tried this out on my landisks and it seems to be fine.
So for as far as "I put this on my hardware and it didn't break my
ethernet or make it explode":

Tested-by: Daniel Palmer <daniel@thingy.jp>

I don't know enough about the APIs involved to review this properly
but for FWIW I had my clanker try to reproduce this in QEMU with an
emulation of the landisk and it said it could get into the state
described in the patch so it seems valid.

Cheers,

Daniel

Re: [PATCH net] net: 8139cp: close race between NAPI completion and Rx unmask

[netdev-bot+sashiko]

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

Critical: 1 · High: 0 · 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).

- [Critical] The patch moves napi_complete_done() inside
  spin_lock_irqsave(&cp->lock) in cp_rx_poll()…

--

net: 8139cp: close race between NAPI completion and Rx unmask

This commit moves napi_complete_done() in cp_rx_poll() so that it runs
under cp->lock with interrupts disabled. NAPI completion and the Rx
IntrMask unmask then happen atomically with respect to cp_interrupt().
The goal is to fix an Rx interrupt storm that hard-hung SH7751 LANDISK
boards.

> diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
> index 9016527e229a..a9334d3a0f7d 100644
> --- a/drivers/net/ethernet/realtek/8139cp.c
> +++ b/drivers/net/ethernet/realtek/8139cp.c
> @@ -554,11 +554,12 @@ static int cp_rx_poll(struct napi_struct *napi, int budget)
>  	/* if we did not reach work limit, then we're done with
>  	 * this round of polling
>  	 */
> -	if (rx < budget && napi_complete_done(napi, rx)) {
> +	if (rx < budget) {
>  		unsigned long flags;
>  
>  		spin_lock_irqsave(&cp->lock, flags);
> -		cpw16_f(IntrMask, cp_intr_mask);
> +		if (napi_complete_done(napi, rx))
> +			cpw16_f(IntrMask, cp_intr_mask);

[Severity: Critical]
Can this deadlock on cp->lock?

8139cp passes every received frame to GRO in cp_rx_skb():

    napi_gro_receive(&cp->napi, skb);

Frames that GRO cannot merge wait on gro->rx_list until gro_normal_batch
is reached, and TCP flows can sit in the GRO hash. All of them are
delivered by napi_complete_done() when the poll ends with rx < budget:

napi_complete_done()
  gro_flush_normal()
    gro_flush()
    gro_normal_list()
      netif_receive_skb_list_internal()

With this change, that receive processing runs with cp->lock held and
hard IRQs off.

Suppose the stack replies synchronously on the same interface, for
example with an ARP reply, an ICMP echo reply, a TCP ACK, or a forwarded
or bridged frame. The transmit path then seems to come back into the
driver:

netif_receive_skb_list_internal()
  arp_process() / icmp_reply() / tcp_send_ack()
    dev_queue_xmit()
      sch_direct_xmit()
        dev_hard_start_xmit()
          cp_start_xmit()
            spin_lock_irqsave(&cp->lock, intr_flags);

Wouldn't this CPU then spin forever on a cp->lock it already holds? On
SMP, one ARP request or ping from a peer on the link looks like enough to
trigger it. That includes QEMU's emulated rtl8139, which reports revision
0x20 and binds to 8139cp.

There may also be a second path. __dev_queue_xmit() uses
rcu_read_lock_bh()/rcu_read_unlock_bh(), and netfilter uses
local_bh_disable()/local_bh_enable(). Both reach __local_bh_enable_ip().
That function does lockdep_assert_irqs_enabled(), and with
CONFIG_TRACE_IRQFLAGS it also calls local_irq_enable().

Could hard IRQs come back on while cp->lock is still held? Then
cp_interrupt() could run on the same CPU and spin on:

    spin_lock(&cp->lock);

The commit message says:

    Do the completion and the unmask under the lock, as 8139too does.

Does that comparison carry over? rtl8139_rx() in 8139too calls
netif_receive_skb() directly, so its GRO lists stay empty. As a result,
napi_complete_done() under tp->lock in rtl8139_poll() never delivers
packets.

Before commit ab1e7e1d2666 ("8139cp: use napi_complete_done()"),
cp_rx_poll() called napi_gro_flush(napi, false) before taking cp->lock.
Only __napi_complete() and the IntrMask write were done under the lock.

The SH7751 test boards are UP. There, spin_lock() without
DEBUG_SPINLOCK reduces to preempt_disable(), which may explain why this
didn't show up in testing.

Would it work to keep GRO flushing and stack delivery outside cp->lock?
Only the NAPI state change and the IntrMask unmask would then run under
the lock. One way is to flush GRO before taking the lock. Another is to
keep napi_complete_done() outside the lock and close the race some
other way, for example by re-checking IntrStatus or handling the case in
cp_interrupt().

>  		spin_unlock_irqrestore(&cp->lock, flags);
>  	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-b4-8139cp-napi-v1-1-c9b683d9564b%40gmail.com