On Mon, Dec 02, 2024 at 06:21:02PM +0000, Joe Damato wrote:
> Make napi_hash_lock IRQ safe. It is used during the control path, and is
> taken and released in napi_hash_add and napi_hash_del, which will
> typically be called by calls to napi_enable and napi_disable.
>
> This change avoids a deadlock in pcnet32 (and other any other drivers
> which follow the same pattern):
>
> CPU 0:
> pcnet32_open
> spin_lock_irqsave(&lp->lock, ...)
> napi_enable
> napi_hash_add <- before this executes, CPU 1 proceeds
> spin_lock(napi_hash_lock)
> [...]
> spin_unlock_irqrestore(&lp->lock, flags);
>
> CPU 1:
> pcnet32_close
> napi_disable
> napi_hash_del
> spin_lock(napi_hash_lock)
> < INTERRUPT >
> pcnet32_interrupt
> spin_lock(lp->lock) <- DEADLOCK
>
> Changing the napi_hash_lock to be IRQ safe prevents the IRQ from firing
> on CPU 1 until napi_hash_lock is released, preventing the deadlock.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 86e25f40aa1e ("net: napi: Add napi_config")
> Reported-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> Closes: https://lore.kernel.org/netdev/85dd4590-ea6b-427d-876a-1d8559c7ad82@xxxxxxxxxxxx/
> Suggested-by: Jakub Kicinski <kuba@xxxxxxxxxx>
> Signed-off-by: Joe Damato <jdamato@xxxxxxxxxx>
Tested-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> net/core/dev.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 13d00fc10f55..45a8c3dd4a64 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6557,18 +6557,22 @@ static void __napi_hash_add_with_id(struct napi_struct *napi,
> static void napi_hash_add_with_id(struct napi_struct *napi,
> unsigned int napi_id)
> {
> - spin_lock(&napi_hash_lock);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&napi_hash_lock, flags);
> WARN_ON_ONCE(napi_by_id(napi_id));
> __napi_hash_add_with_id(napi, napi_id);
> - spin_unlock(&napi_hash_lock);
> + spin_unlock_irqrestore(&napi_hash_lock, flags);
> }
>
> static void napi_hash_add(struct napi_struct *napi)
> {
> + unsigned long flags;
> +
> if (test_bit(NAPI_STATE_NO_BUSY_POLL, &napi->state))
> return;
>
> - spin_lock(&napi_hash_lock);
> + spin_lock_irqsave(&napi_hash_lock, flags);
>
> /* 0..NR_CPUS range is reserved for sender_cpu use */
> do {
> @@ -6578,7 +6582,7 @@ static void napi_hash_add(struct napi_struct *napi)
>
> __napi_hash_add_with_id(napi, napi_gen_id);
>
> - spin_unlock(&napi_hash_lock);
> + spin_unlock_irqrestore(&napi_hash_lock, flags);
> }
>
> /* Warning : caller is responsible to make sure rcu grace period
> @@ -6586,11 +6590,13 @@ static void napi_hash_add(struct napi_struct *napi)
> */
> static void napi_hash_del(struct napi_struct *napi)
> {
> - spin_lock(&napi_hash_lock);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&napi_hash_lock, flags);
>
> hlist_del_init_rcu(&napi->napi_hash_node);
>
> - spin_unlock(&napi_hash_lock);
> + spin_unlock_irqrestore(&napi_hash_lock, flags);
> }
>
> static enum hrtimer_restart napi_watchdog(struct hrtimer *timer)
> --
> 2.25.1
>
