| Message ID | 20241202182103.363038-1-jdamato@fastly.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cecc1555a8c2acd65f9d36182c28ae463db0ad7e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: Make napi_hash_lock irq safe | expand |
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@vger.kernel.org > Fixes: 86e25f40aa1e ("net: napi: Add napi_config") > Reported-by: Guenter Roeck <linux@roeck-us.net> > Closes: https://lore.kernel.org/netdev/85dd4590-ea6b-427d-876a-1d8559c7ad82@roeck-us.net/ > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Joe Damato <jdamato@fastly.com> Tested-by: Guenter Roeck <linux@roeck-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 >
On Mon, Dec 2, 2024 at 7:21 PM Joe Damato <jdamato@fastly.com> 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@vger.kernel.org > Fixes: 86e25f40aa1e ("net: napi: Add napi_config") > Reported-by: Guenter Roeck <linux@roeck-us.net> > Closes: https://lore.kernel.org/netdev/85dd4590-ea6b-427d-876a-1d8559c7ad82@roeck-us.net/ > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Joe Damato <jdamato@fastly.com> Reviewed-by: Eric Dumazet <edumazet@google.com>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Mon, 2 Dec 2024 18:21:02 +0000 you 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): > > [...] Here is the summary with links: - [net] net: Make napi_hash_lock irq safe https://git.kernel.org/netdev/net/c/cecc1555a8c2 You are awesome, thank you!
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)
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@vger.kernel.org Fixes: 86e25f40aa1e ("net: napi: Add napi_config") Reported-by: Guenter Roeck <linux@roeck-us.net> Closes: https://lore.kernel.org/netdev/85dd4590-ea6b-427d-876a-1d8559c7ad82@roeck-us.net/ Suggested-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Joe Damato <jdamato@fastly.com> --- net/core/dev.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)