Re: [PATCH net-next v7 14/17] net: macb: move printk() calls out of bp->lock critical section

[Théo Lebrun]

Replying to sashiko
https://sashiko.dev/#/patchset/20260803-macb-context-v7-0-4d7d4af04849%40bootlin.com

> commit 236ca85ec182e174ae2b672ce495a29216642196
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>
> net: macb: move printk() calls out of bp->lock critical section
>
> This patch moves printk() calls outside of the bp->lock critical section in
> the macb driver to prevent deadlocks with netconsole. It defers the error
> reporting for TX buffer exhaustion, halt timeouts, and HRESP errors.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 5a4eb87f5a97d..7e77c24da8a63 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -1303,6 +1303,7 @@ static void macb_tx_error_task(struct work_struct *work)
> >  	struct macb_tx_skb *tx_skb;
> >  	struct macb_dma_desc *desc;
> >  	bool halt_timeout = false;
>
> [Severity: High]
> Does the driver ensure that this work item is cancelled during teardown?
>
> If macb_close() frees bp->ctx and sets it to NULL without calling
> cancel_work_sync(), this work item might execute after the interface is
> brought down.
>
> Since this function starts with:
>
> macb_tx_error_task() {
>     ...
>     struct macb_context *ctx = queue->bp->ctx;
>     ...
> }
>
> Could this result in a use-after-free or NULL pointer dereference if an
> error interrupt fires shortly before or during interface teardown? The
> same question applies to macb_hresp_error_task() and bp->ctx.

Ignoring because this is a pre-existing issue.

It complains about the close codepath which we don't modify here. BTW
the fix to the close codepath would probably to copy most ideas from
the swap we are implementing. Said another way, swap is more thoughtful
than close.

> > +	bool buggy_driver = false;
> >  	struct sk_buff *skb;
> >  	unsigned long flags;
> >  	unsigned int tail;
>
> [ ... ]
>
> > @@ -1412,6 +1411,13 @@ static void macb_tx_error_task(struct work_struct *work)
> >  	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
> >  
> >  	spin_unlock_irqrestore(&bp->lock, flags);
> > +
> > +	if (halt_timeout)
> > +		netdev_err(bp->netdev, "BUG: halt tx timed out, we ignored it\n");
> > +
> > +	if (buggy_driver)
> > +		netdev_err(bp->netdev, "BUG: TX buffers exhausted mid-frame\n");
> > +
> >  	napi_enable(&queue->napi_tx);
>
> [Severity: High]
> Could moving these netdev_err() calls here create a race condition that
> stalls the TX queue?
>
> Earlier in this function, the hardware is restarted and interrupts are
> unmasked. When the spinlock is dropped, an interrupt can fire while
> napi_tx is still disabled.
>
> If a TCOMP interrupt fires during these netdev_err() calls, the
> interrupt handler will mask the TCOMP interrupt in IDR and attempt
> to schedule NAPI:
>
> macb_interrupt() {
>     ...
>     if (status & (MACB_BIT(TCOMP) | MACB_BIT(TXUBR))) {
>         queue_writel(queue, IDR, MACB_BIT(TCOMP));
>         ...
>         napi_schedule_irqoff(&queue->napi_tx);
>     }
>     ...
> }
>
> Since napi_enable() hasn't been called yet, the schedule request will be
> dropped. When napi_enable() finally runs, it clears the disabled state
> but does not unmask the interrupt or schedule NAPI, which seems like it
> could leave the queue permanently stalled. Should napi_enable() be
> called before these netdev_err() calls?

So this complains about a race inbetween macb_tx_error_task() and our
IRQ handler. Here is the two contexts side-by-side (sorry for 80 char
overflow, I'm counterbalancing that with shiny box drawing unicode):

─ PROCESS CONTEXT ─────────────────┐
                                   │
macb_tx_error_task() {             │
  napi_disable(queue->napi_tx)     │
  spin_lock_irqsave(bp->lock)      │
  ...                              │
  spin_unlock_irqrestore(bp->lock) │
                                   ├─ INTERRUPT CONTEXT ────────────────
                                   │
                                   │ macb_interrupt() {
                                   │   spin_lock(bp->lock)
                                   │   status = queue_readl(queue, ISR)
                                   │   if (status & TCOMP) {
                                   │     queue_writel(queue, IDR, TCOMP)
                                   │     macb_queue_isr_clear(queue, TCOMP)
                                   │     napi_schedule_irqoff(&queue->napi_tx)
                                   │   }
                                   │   ...
                                   │ }
                                   ├────────────────────────────────────
  napi_enable(queue->napi_tx)      │
}                                  │
───────────────────────────────────┘

(I've schematised it but expect macb_interrupt() to be stuck on
spin_lock() waiting for spin_unlock() from the process context.)

The race is that the IRQ might call napi_schedule_irqoff() before
napi_enable(). But what we've done with our deferred netdev_err() calls
is only to grow the race window, we haven't introduced it.

The LLM missed one key point in that the window was already present, and
probably as important or more than the printk I'm adding:
spin_unlock_irqrestore() might trigger kernel-side pre-emption.

So I'll move my two netdev_err() after napi_enable(), but this is a
pre-existing issue unrelated to this swap series.

Thanks,

-- 
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com