All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sumit Garg <sumit.garg@linaro.org>
To: linux-wireless@vger.kernel.org
Cc: johannes@sipsolutions.net, davem@davemloft.net, kuba@kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	matthias.schoepfer@ithinx.io, Philipp.Berg@liebherr.com,
	Michael.Weitner@liebherr.com, daniel.thompson@linaro.org,
	loic.poulain@linaro.org, Sumit Garg <sumit.garg@linaro.org>,
	stable@vger.kernel.org
Subject: [PATCH] mac80211: fix race in ieee80211_register_hw()
Date: Mon,  6 Apr 2020 17:51:17 +0530	[thread overview]
Message-ID: <1586175677-3061-1-git-send-email-sumit.garg@linaro.org> (raw)

A race condition leading to a kernel crash is observed during invocation
of ieee80211_register_hw() on a dragonboard410c device having wcn36xx
driver built as a loadable module along with a wifi manager in user-space
waiting for a wifi device (wlanX) to be active.

Sequence diagram for a particular kernel crash scenario:

    user-space  ieee80211_register_hw()  RX IRQ
    +++++++++++++++++++++++++++++++++++++++++++++
       |                    |             |
       |<---wlan0---wiphy_register()      |
       |----start wlan0---->|             |
       |                    |<---IRQ---(RX packet)
       |              Kernel crash        |
       |              due to unallocated  |
       |              workqueue.          |
       |                    |             |
       |       alloc_ordered_workqueue()  |
       |                    |             |
       |              Misc wiphy init.    |
       |                    |             |
       |            ieee80211_if_add()    |
       |                    |             |

As evident from above sequence diagram, this race condition isn't specific
to a particular wifi driver but rather the initialization sequence in
ieee80211_register_hw() needs to be fixed. So re-order the initialization
sequence and the updated sequence diagram would look like:

    user-space  ieee80211_register_hw()  RX IRQ
    +++++++++++++++++++++++++++++++++++++++++++++
       |                    |             |
       |       alloc_ordered_workqueue()  |
       |                    |             |
       |              Misc wiphy init.    |
       |                    |             |
       |<---wlan0---wiphy_register()      |
       |----start wlan0---->|             |
       |                    |<---IRQ---(RX packet)
       |                    |             |
       |            ieee80211_if_add()    |
       |                    |             |

Cc: <stable@vger.kernel.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 net/mac80211/main.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 4c2b5ba..4ca62fc 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1051,7 +1051,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 		local->hw.wiphy->signal_type = CFG80211_SIGNAL_TYPE_UNSPEC;
 		if (hw->max_signal <= 0) {
 			result = -EINVAL;
-			goto fail_wiphy_register;
+			goto fail_workqueue;
 		}
 	}
 
@@ -1113,7 +1113,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 
 	result = ieee80211_init_cipher_suites(local);
 	if (result < 0)
-		goto fail_wiphy_register;
+		goto fail_workqueue;
 
 	if (!local->ops->remain_on_channel)
 		local->hw.wiphy->max_remain_on_channel_duration = 5000;
@@ -1139,10 +1139,6 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 
 	local->hw.wiphy->max_num_csa_counters = IEEE80211_MAX_CSA_COUNTERS_NUM;
 
-	result = wiphy_register(local->hw.wiphy);
-	if (result < 0)
-		goto fail_wiphy_register;
-
 	/*
 	 * We use the number of queues for feature tests (QoS, HT) internally
 	 * so restrict them appropriately.
@@ -1254,6 +1250,14 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 		local->sband_allocated |= BIT(band);
 	}
 
+	rtnl_unlock();
+
+	result = wiphy_register(local->hw.wiphy);
+	if (result < 0)
+		goto fail_wiphy_register;
+
+	rtnl_lock();
+
 	/* add one default STA interface if supported */
 	if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_STATION) &&
 	    !ieee80211_hw_check(hw, NO_AUTO_VIF)) {
@@ -1293,6 +1297,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 #if defined(CONFIG_INET) || defined(CONFIG_IPV6)
  fail_ifa:
 #endif
+	wiphy_unregister(local->hw.wiphy);
+ fail_wiphy_register:
 	rtnl_lock();
 	rate_control_deinitialize(local);
 	ieee80211_remove_interfaces(local);
@@ -1302,8 +1308,6 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 	ieee80211_led_exit(local);
 	destroy_workqueue(local->workqueue);
  fail_workqueue:
-	wiphy_unregister(local->hw.wiphy);
- fail_wiphy_register:
 	if (local->wiphy_ciphers_allocated)
 		kfree(local->hw.wiphy->cipher_suites);
 	kfree(local->int_scan_req);
@@ -1353,8 +1357,8 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 	skb_queue_purge(&local->skb_queue_unreliable);
 	skb_queue_purge(&local->skb_queue_tdls_chsw);
 
-	destroy_workqueue(local->workqueue);
 	wiphy_unregister(local->hw.wiphy);
+	destroy_workqueue(local->workqueue);
 	ieee80211_led_exit(local);
 	kfree(local->int_scan_req);
 }
-- 
2.7.4


             reply	other threads:[~2020-04-06 12:22 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-06 12:21 Sumit Garg [this message]
2020-04-06 12:44 ` [PATCH] mac80211: fix race in ieee80211_register_hw() Johannes Berg
2020-04-06 13:00   ` Sumit Garg
2020-04-06 12:44 ` Kalle Valo
2020-04-06 12:47   ` Johannes Berg
2020-04-06 12:52     ` Kalle Valo
2020-04-06 12:53       ` Johannes Berg
2020-04-06 13:04         ` Kalle Valo
2020-04-06 13:07           ` Johannes Berg
2020-04-06 13:21             ` Sumit Garg
2020-04-06 13:27               ` Kalle Valo
2020-04-06 13:55                 ` Krishna Chaitanya
2020-04-06 14:01                   ` Johannes Berg
2020-04-06 14:25                     ` Krishna Chaitanya
2020-04-06 15:06                       ` Johannes Berg
2020-04-06 18:08                         ` Krishna Chaitanya
2020-04-07  6:42                           ` Sumit Garg
2020-04-06 13:06     ` Sumit Garg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1586175677-3061-1-git-send-email-sumit.garg@linaro.org \
    --to=sumit.garg@linaro.org \
    --cc=Michael.Weitner@liebherr.com \
    --cc=Philipp.Berg@liebherr.com \
    --cc=daniel.thompson@linaro.org \
    --cc=davem@davemloft.net \
    --cc=johannes@sipsolutions.net \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=loic.poulain@linaro.org \
    --cc=matthias.schoepfer@ithinx.io \
    --cc=netdev@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.