Re: [PATCH net] pcnet32: stop holding device spin lock during napi_complete_done

[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.
---
pcnet32: stop holding device spin lock during napi_complete_done

This commit removes the lp->lock spinlock hold around napi_complete_done()
to avoid a potential deadlock with pcnet32_start_xmit() if a GRO flush
were to occur.

> diff --git a/drivers/net/ethernet/amd/pcnet32.c b/drivers/net/ethernet/amd/pcnet32.c
> index 911808ab13a79..4f3076d4ea34e 100644
> --- a/drivers/net/ethernet/amd/pcnet32.c
> +++ b/drivers/net/ethernet/amd/pcnet32.c
> @@ -1407,8 +1407,10 @@ static int pcnet32_poll(struct napi_struct *napi, int budget)
>  		pcnet32_restart(dev, CSR0_START);
>  		netif_wake_queue(dev);
>  	}
> +	spin_unlock_irqrestore(&lp->lock, flags);
>  
>  	if (work_done < budget && napi_complete_done(napi, work_done)) {
> +		spin_lock_irqsave(&lp->lock, flags);

[Severity: High]
Does dropping the spinlock before napi_complete_done() introduce a race
condition with shared IRQs that breaks NAPI interrupt mitigation?

If napi_complete_done() clears the NAPIF_STATE_SCHED bit, marking the
NAPI poll as complete, a shared IRQ could fire on another CPU.

If pcnet32_interrupt() then acquires lp->lock and checks csr0 without
checking the INTR bit, could it process pending events and successfully
call napi_schedule_prep()?

If pcnet32_interrupt() schedules NAPI and releases the lock, this code
would then resume, re-acquire lp->lock, and unconditionally unmask
device interrupts.

This could leave the system in a state where NAPI is scheduled but
hardware interrupts are enabled, resulting in an interrupt storm on
every subsequent packet.

>  		/* clear interrupt masks */
>  		val = lp->a->read_csr(ioaddr, CSR3);
>  		val &= 0x00ff;
> @@ -1416,9 +1418,9 @@ static int pcnet32_poll(struct napi_struct *napi, int budget)
>  
>  		/* Set interrupt enable. */
>  		lp->a->write_csr(ioaddr, CSR0, CSR0_INTEN);
> +		spin_unlock_irqrestore(&lp->lock, flags);
>  	}
>  
> -	spin_unlock_irqrestore(&lp->lock, flags);
>  	return work_done;
>  }
-- 
pw-bot: cr