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

[netdev-bot+sashiko@kernel.org]

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