Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752160AbdGPN6Q (ORCPT ); Sun, 16 Jul 2017 09:58:16 -0400 Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:43194 "EHLO shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751693AbdGPN5z (ORCPT ); Sun, 16 Jul 2017 09:57:55 -0400 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit MIME-Version: 1.0 From: Ben Hutchings To: linux-kernel@vger.kernel.org, stable@vger.kernel.org CC: akpm@linux-foundation.org, "Johannes Berg" , "Sowmini Varadhan" , "Dmitry Vyukov" Date: Sun, 16 Jul 2017 14:56:46 +0100 Message-ID: X-Mailer: LinuxStableQueue (scripts by bwh) Subject: [PATCH 3.16 053/178] nl80211: fix dumpit error path RTNL deadlocks In-Reply-To: X-SA-Exim-Connect-IP: 2a02:8011:400e:2:6f00:88c8:c921:d332 X-SA-Exim-Mail-From: ben@decadent.org.uk X-SA-Exim-Scanned: No (on shadbolt.decadent.org.uk); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4443 Lines: 180 3.16.46-rc1 review patch. If anyone has any objections, please let me know. ------------------ From: Johannes Berg commit ea90e0dc8cecba6359b481e24d9c37160f6f524f upstream. Sowmini pointed out Dmitry's RTNL deadlock report to me, and it turns out to be perfectly accurate - there are various error paths that miss unlock of the RTNL. To fix those, change the locking a bit to not be conditional in all those nl80211_prepare_*_dump() functions, but make those require the RTNL to start with, and fix the buggy error paths. This also let me use sparse (by appropriately overriding the rtnl_lock/rtnl_unlock functions) to validate the changes. Reported-by: Sowmini Varadhan Reported-by: Dmitry Vyukov Signed-off-by: Johannes Berg [bwh: Backported to 3.16: - Drop changes to nl80211_dump_interface(), nl80211_dump_mpp(), nl80211_prepare_vendor_dump(), nl80211_vendor_cmd_dump() - Adjust context] Signed-off-by: Ben Hutchings --- net/wireless/nl80211.c | 127 ++++++++++++++++++++++--------------------------- 1 file changed, 56 insertions(+), 71 deletions(-) --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -471,21 +471,17 @@ static int nl80211_prepare_wdev_dump(str { int err; - rtnl_lock(); - if (!cb->args[0]) { err = nlmsg_parse(cb->nlh, GENL_HDRLEN + nl80211_fam.hdrsize, nl80211_fam.attrbuf, nl80211_fam.maxattr, nl80211_policy); if (err) - goto out_unlock; + return err; *wdev = __cfg80211_wdev_from_attrs(sock_net(skb->sk), nl80211_fam.attrbuf); - if (IS_ERR(*wdev)) { - err = PTR_ERR(*wdev); - goto out_unlock; - } + if (IS_ERR(*wdev)) + return PTR_ERR(*wdev); *rdev = wiphy_to_rdev((*wdev)->wiphy); /* 0 is the first index - add 1 to parse only once */ cb->args[0] = (*rdev)->wiphy_idx + 1; @@ -495,10 +491,8 @@ static int nl80211_prepare_wdev_dump(str struct wiphy *wiphy = wiphy_idx_to_wiphy(cb->args[0] - 1); struct wireless_dev *tmp; - if (!wiphy) { - err = -ENODEV; - goto out_unlock; - } + if (!wiphy) + return -ENODEV; *rdev = wiphy_to_rdev(wiphy); *wdev = NULL; @@ -509,21 +503,11 @@ static int nl80211_prepare_wdev_dump(str } } - if (!*wdev) { - err = -ENODEV; - goto out_unlock; - } + if (!*wdev) + return -ENODEV; } return 0; - out_unlock: - rtnl_unlock(); - return err; -} - -static void nl80211_finish_wdev_dump(struct cfg80211_registered_device *rdev) -{ - rtnl_unlock(); } /* IE validation */ @@ -3727,9 +3711,10 @@ static int nl80211_dump_station(struct s int sta_idx = cb->args[2]; int err; + rtnl_lock(); err = nl80211_prepare_wdev_dump(skb, cb, &rdev, &wdev); if (err) - return err; + goto out_err; if (!wdev->netdev) { err = -EINVAL; @@ -3765,7 +3750,7 @@ static int nl80211_dump_station(struct s cb->args[2] = sta_idx; err = skb->len; out_err: - nl80211_finish_wdev_dump(rdev); + rtnl_unlock(); return err; } @@ -4443,9 +4428,10 @@ static int nl80211_dump_mpath(struct sk_ int path_idx = cb->args[2]; int err; + rtnl_lock(); err = nl80211_prepare_wdev_dump(skb, cb, &rdev, &wdev); if (err) - return err; + goto out_err; if (!rdev->ops->dump_mpath) { err = -EOPNOTSUPP; @@ -4479,7 +4465,7 @@ static int nl80211_dump_mpath(struct sk_ cb->args[2] = path_idx; err = skb->len; out_err: - nl80211_finish_wdev_dump(rdev); + rtnl_unlock(); return err; } @@ -6157,9 +6143,12 @@ static int nl80211_dump_scan(struct sk_b int start = cb->args[2], idx = 0; int err; + rtnl_lock(); err = nl80211_prepare_wdev_dump(skb, cb, &rdev, &wdev); - if (err) + if (err) { + rtnl_unlock(); return err; + } wdev_lock(wdev); spin_lock_bh(&rdev->bss_lock); @@ -6182,7 +6171,7 @@ static int nl80211_dump_scan(struct sk_b wdev_unlock(wdev); cb->args[2] = idx; - nl80211_finish_wdev_dump(rdev); + rtnl_unlock(); return skb->len; } @@ -6255,9 +6244,10 @@ static int nl80211_dump_survey(struct sk int survey_idx = cb->args[2]; int res; + rtnl_lock(); res = nl80211_prepare_wdev_dump(skb, cb, &rdev, &wdev); if (res) - return res; + goto out_err; if (!wdev->netdev) { res = -EINVAL; @@ -6303,7 +6293,7 @@ static int nl80211_dump_survey(struct sk cb->args[2] = survey_idx; res = skb->len; out_err: - nl80211_finish_wdev_dump(rdev); + rtnl_unlock(); return res; }