linux-kernel - Re: [net] net: Make napi_hash

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ddeca293-5938-42f3-9722-748050ab0aa0@roeck-us.net>
Date: Mon, 2 Dec 2024 12:55:27 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Joe Damato <jdamato@...tly.com>
Cc: netdev@...r.kernel.org, pabeni@...hat.com, edumazet@...gle.com,
	kuba@...nel.org, mkarsten@...terloo.ca, stable@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Simon Horman <horms@...nel.org>, David Ahern <dsahern@...nel.org>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Lorenzo Bianconi <lorenzo@...nel.org>,
	Alexander Lobakin <aleksander.lobakin@...el.com>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [net] net: Make napi_hash_lock irq safe

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@...r.kernel.org
> Fixes: 86e25f40aa1e ("net: napi: Add napi_config")
> Reported-by: Guenter Roeck <linux@...ck-us.net>
> Closes: https://lore.kernel.org/netdev/85dd4590-ea6b-427d-876a-1d8559c7ad82@roeck-us.net/
> Suggested-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Joe Damato <jdamato@...tly.com>

Tested-by: Guenter Roeck <linux@...ck-us.net>

> ---
>  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
>