Re: [PATCH net] netpoll: run NAPI poll in softirq context to avoid rq->lock self-deadlock

[sashiko-bot@kernel.org]

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

New issues:
- [High] On PREEMPT_RT, `local_bh_enable()` triggers a lockdep splat and self-deadlocks because IRQs are disabled.
- [High] The addition of `local_bh_disable()` and `_local_bh_enable()` triggers a `WARN_ON_ONCE(in_hardirq())` splat when netconsole is used from hardirq context.

Pre-existing issues:
- [Critical] The patch fails to resolve the self-deadlock because it shifts the ksoftirqd wakeup to `zap_completion_queue()`.
--

commit 5559ca23d7e6dcba4ded6d9783c2b7954c8e659f
Author: Vlad Poenaru <vlad.wing@gmail.com>

netpoll: run NAPI poll in softirq context to avoid rq->lock self-deadlock

This commit aims to prevent a self-deadlock caused by netconsole flushing
messages while holding a runqueue lock. It wraps the netpoll NAPI callback
in local_bh_disable() to ensure softirqs are not raised out of band,
deferring them instead of waking ksoftirqd inline.

> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 3f4a17fa5713a..18da97eff5322 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -194,11 +194,56 @@ void netpoll_poll_dev(struct net_device *dev)
[ ... ]
> +	 * Disabling BH makes in_interrupt() true for the duration of the poll,
> +	 * so the TX completion only sets the softirq-pending bit and never wakes
> +	 * ksoftirqd. The raised softirq is harmless and benign: netpoll reaps
> +	 * the freed skbs itself via zap_completion_queue() below, and the
> +	 * pending NET_TX softirq is serviced at the next irq_exit().
> +	 */
> +	local_bh_disable();

[Severity: High]
Does adding local_bh_disable() here trigger a warning when netpoll is called
from hardirq context?

Netconsole is frequently used to flush dmesg synchronously during kernel
panics or sysrq, which occur in hardware interrupt handlers. Both
local_bh_disable() (via __local_bh_disable_ip()) and _local_bh_enable()
explicitly assert WARN_ON_ONCE(in_hardirq()).

Could this cause recursive warnings or disrupt the logging path during a
panic sequence?

>  	if (ops->ndo_poll_controller)
>  		ops->ndo_poll_controller(dev);
>  
>  	poll_napi(dev);
>  
> +#ifndef CONFIG_PREEMPT_RT
[ ... ]
> +#else
> +	/*
> +	 * On PREEMPT_RT this path runs with IRQs enabled and softirqs are
> +	 * threaded, so there is no IRQ-disabled, lock-holding context to
> +	 * protect. _local_bh_enable() is not available on RT, and local_bh_disable()
> +	 * there takes the per-CPU softirq_ctrl local_lock that only the full
> +	 * local_bh_enable() releases -- so use it.
> +	 */
> +	local_bh_enable();

[Severity: High]
The commit message states that on PREEMPT_RT this path runs with IRQs
enabled. Is this accurate?

Looking at netpoll_send_skb(), it unconditionally calls local_irq_save(),
meaning hardware interrupts are disabled here. Calling local_bh_enable()
maps to __local_bh_enable_ip(), which enforces lockdep_assert_irqs_enabled().
Will this trigger a lockdep splat?

Additionally, because IRQs are disabled, preemptible() evaluates to false.
Does the RT implementation then skip inline softirq processing and instead
fall back to wakeup_softirqd(), resulting in the exact same rq->lock
self-deadlock?

> +#endif
> +
>  	up(&ni->dev_lock);
>  
>  	zap_completion_queue();

[Severity: Critical]
This isn't a bug introduced by this patch, but does the patch fail
to fix the self-deadlock it intends to resolve?

The fix wraps poll_napi() to ensure in_interrupt() is true, preventing
the ksoftirqd wakeup. However, zap_completion_queue() is called after
_local_bh_enable(), meaning in_interrupt() is false again.

When zap_completion_queue() processes unfreeable SKBs, it calls:

zap_completion_queue()
  dev_kfree_skb_any()
    dev_kfree_skb_irq_reason()
      raise_softirq_irqoff(NET_TX_SOFTIRQ)

Because in_interrupt() is now false, this immediately invokes
wakeup_softirqd(). Won't this attempt to acquire the CPU's rq->lock
that the caller (e.g., __schedule()) already holds, resulting in the
exact same self-deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610183621.3915271-1-vlad.wing@gmail.com?part=1