[net] netlink: disable IRQs for netlink_lock_table()

Commit Message

Johannes Berg May 17, 2021, 2:38 p.m. UTC

From: Johannes Berg <[e-mail]>

Syzbot reports that in mac80211 we have a potential deadlock
between our "local->stop_queue_reasons_lock" (spinlock) and
netlink's nl_table_lock (rwlock). This is because there's at
least one situation in which we might try to send a netlink
message with this spinlock held while it is also possible to
take the spinlock from a hardirq context, resulting in the
following deadlock scenario reported by lockdep:

       CPU0                    CPU1
       ----                    ----
  lock(nl_table_lock);
                               local_irq_disable();
                               lock(&local->queue_stop_reason_lock);
                               lock(nl_table_lock);
  <Interrupt>
    lock(&local->queue_stop_reason_lock);

This seems valid, we can take the queue_stop_reason_lock in
any kind of context ("CPU0"), and call ieee80211_report_ack_skb()
with the spinlock held and IRQs disabled ("CPU1") in some
code path (ieee80211_do_stop() via ieee80211_free_txskb()).

Short of disallowing netlink use in scenarios like these
(which would be rather complex in mac80211's case due to
the deep callchain), it seems the only fix for this is to
disable IRQs while nl_table_lock is held to avoid hitting
this scenario, this disallows the "CPU0" portion of the
reported deadlock.

Note that the writer side (netlink_table_grab()) already
disables IRQs for this lock.

Unfortunately though, this seems like a huge hammer, and
maybe the whole netlink table locking should be reworked.

Reported-by: [e-mail]
Signed-off-by: Johannes Berg <[e-mail]>
---
 net/netlink/af_netlink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

[e-mail] May 17, 2021, 10:40 p.m. UTC | #1

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Mon, 17 May 2021 16:38:09 +0200 you wrote:
> From: Johannes Berg <[e-mail]>
> 
> Syzbot reports that in mac80211 we have a potential deadlock
> between our "local->stop_queue_reasons_lock" (spinlock) and
> netlink's nl_table_lock (rwlock). This is because there's at
> least one situation in which we might try to send a netlink
> message with this spinlock held while it is also possible to
> take the spinlock from a hardirq context, resulting in the
> following deadlock scenario reported by lockdep:
> 
> [...]

Here is the summary with links:
  - [net] netlink: disable IRQs for netlink_lock_table()
    https://git.kernel.org/netdev/net/c/1d482e666b8e

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Patch

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 3a62f97acf39..6133e412b948 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -461,11 +461,13 @@  void netlink_table_ungrab(void)
 static inline void
 netlink_lock_table(void)
 {
+	unsigned long flags;
+
 	/* read_lock() synchronizes us to netlink_table_grab */
 
-	read_lock(&nl_table_lock);
+	read_lock_irqsave(&nl_table_lock, flags);
 	atomic_inc(&nl_table_users);
-	read_unlock(&nl_table_lock);
+	read_unlock_irqrestore(&nl_table_lock, flags);
 }
 
 static inline void