From patchwork Fri May 12 16:41:58 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Brian Norris X-Patchwork-Id: 9724599 X-Patchwork-Delegate: kvalo@adurom.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 57F2A601E7 for ; Fri, 12 May 2017 16:42:58 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 424E028854 for ; Fri, 12 May 2017 16:42:58 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 35A0528863; Fri, 12 May 2017 16:42:58 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.5 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9368928854 for ; Fri, 12 May 2017 16:42:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933324AbdELQmn (ORCPT ); Fri, 12 May 2017 12:42:43 -0400 Received: from mail-pf0-f178.google.com ([209.85.192.178]:34605 "EHLO mail-pf0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933277AbdELQml (ORCPT ); Fri, 12 May 2017 12:42:41 -0400 Received: by mail-pf0-f178.google.com with SMTP id e64so32714191pfd.1 for ; Fri, 12 May 2017 09:42:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id; bh=D7dbfTq+PM9+4rfUHwnAB0fZwjDDBiAVGJZO30hQcpM=; b=My+KxiQfYoCuMnfw3yRfzKMWa0TED+FXWNDxxbkdRCYaQa1Fgu+YAjdDYvD0P3VLbu diNmjH2YMdiloa6YtuwQJW08T09g6VUOnMsov1oTVZ/faz49rJoDQ5pvcpY4A5kNh5nZ 96N65KBuDYhMoET7k6kr8DFbeUUb/gw8ym2zE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=D7dbfTq+PM9+4rfUHwnAB0fZwjDDBiAVGJZO30hQcpM=; b=N3iANB75onlVP1vz4fenvlrv8Rcnaa7ihtmUgZBMaZrgHnqmp3rxiiEmwWFXI2ZNkg g3EzBjAWpTsAxfopfI0eypIKahkGu9WQ7LnkDsiUBKpOR28OLWbpWTaypwJ05sPjV6Go ATvJKjEdXWZY11FlrPO8n+q9L0sJiYxOMZAXaGE1z0p5Lzh0W8/Q/WeQOWsSUajA48uG dO3diDIqd5lhFMr+YnUYpqgasnqjK9ojOaY4j4OAh5ublZgVqf1vJay+yJpgCGM6A4MH qVGOMXWrv0uJ3AgtA0svnVgSvLSfxmdf0SNspgoXJsJXc8T3pSgVbJye2t2XxzTQN/Z9 lZsw== X-Gm-Message-State: AODbwcDo7oiCn2okkV/6zlVYnwfS+0E2PHx7d2/lcHSqQ3uYULDNlV+z JkZFWVpxBKJn9kmk X-Received: by 10.99.125.12 with SMTP id y12mr5371447pgc.155.1494607345703; Fri, 12 May 2017 09:42:25 -0700 (PDT) Received: from ban.mtv.corp.google.com ([172.22.64.120]) by smtp.gmail.com with ESMTPSA id u74sm7304499pfk.58.2017.05.12.09.42.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 12 May 2017 09:42:24 -0700 (PDT) From: Brian Norris To: Ganapathi Bhat , Nishant Sarmukadam Cc: , Dmitry Torokhov , Amitkumar Karwar , Kalle Valo , linux-wireless@vger.kernel.org, Doug Anderson , Brian Norris Subject: [PATCH 01/11] mwifiex: fixup error cases in mwifiex_add_virtual_intf() Date: Fri, 12 May 2017 09:41:58 -0700 Message-Id: <20170512164208.38725-1-briannorris@chromium.org> X-Mailer: git-send-email 2.13.0.rc2.291.g57267f2277-goog Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP If we fail to add an interface in mwifiex_add_virtual_intf(), we might hit a BUG_ON() in the networking code, because we didn't tear things down properly. Among the problems: (a) when failing to allocate workqueues, we fail to unregister the netdev before calling free_netdev() (b) even if we do try to unregister the netdev, we're still holding the rtnl lock, so the device never properly unregistered; we'll be at state NETREG_UNREGISTERING, and then hit free_netdev()'s: BUG_ON(dev->reg_state != NETREG_UNREGISTERED); (c) we're allocating some dependent resources (e.g., DFS workqueues) after we've registered the interface; this may or may not cause problems, but it's good practice to allocate these before registering (d) we're not even trying to unwind anything when mwifiex_send_cmd() or mwifiex_sta_init_cmd() fail To fix these issues, let's: * add a stacked set of error handling labels, to keep error handling consistent and properly ordered (resolving (a) and (d)) * move the workqueue allocations before the registration (to resolve (c); also resolves (b) by avoiding error cases where we have to unregister) [Incidentally, it's pretty easy to interrupt the alloc_workqueue() in, e.g., the following: iw phy phy0 interface add mlan0 type station by sending it SIGTERM.] This bugfix covers commits like commit 7d652034d1a0 ("mwifiex: channel switch support for mwifiex"), but parts of this bug exist all the way back to the introduction of dynamic interface handling in commit 93a1df48d224 ("mwifiex: add cfg80211 handlers add/del_virtual_intf"). Cc: Signed-off-by: Brian Norris --- drivers/net/wireless/marvell/mwifiex/cfg80211.c | 71 ++++++++++++------------- 1 file changed, 35 insertions(+), 36 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c index 7ec06bf13413..025bc06a19d6 100644 --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c @@ -2964,10 +2964,8 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, if (!dev) { mwifiex_dbg(adapter, ERROR, "no memory available for netdevice\n"); - memset(&priv->wdev, 0, sizeof(priv->wdev)); - priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED; - priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED; - return ERR_PTR(-ENOMEM); + ret = -ENOMEM; + goto err_alloc_netdev; } mwifiex_init_priv_params(priv, dev); @@ -2976,11 +2974,11 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, ret = mwifiex_send_cmd(priv, HostCmd_CMD_SET_BSS_MODE, HostCmd_ACT_GEN_SET, 0, NULL, true); if (ret) - return ERR_PTR(ret); + goto err_set_bss_mode; ret = mwifiex_sta_init_cmd(priv, false, false); if (ret) - return ERR_PTR(ret); + goto err_sta_init; mwifiex_setup_ht_caps(&wiphy->bands[NL80211_BAND_2GHZ]->ht_cap, priv); if (adapter->is_hw_11ac_capable) @@ -3011,31 +3009,14 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, SET_NETDEV_DEV(dev, adapter->dev); - /* Register network device */ - if (register_netdevice(dev)) { - mwifiex_dbg(adapter, ERROR, - "cannot register virtual network device\n"); - free_netdev(dev); - priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED; - priv->netdev = NULL; - memset(&priv->wdev, 0, sizeof(priv->wdev)); - priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED; - return ERR_PTR(-EFAULT); - } - priv->dfs_cac_workqueue = alloc_workqueue("MWIFIEX_DFS_CAC%s", WQ_HIGHPRI | WQ_MEM_RECLAIM | WQ_UNBOUND, 1, name); if (!priv->dfs_cac_workqueue) { - mwifiex_dbg(adapter, ERROR, - "cannot register virtual network device\n"); - free_netdev(dev); - priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED; - priv->netdev = NULL; - memset(&priv->wdev, 0, sizeof(priv->wdev)); - priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED; - return ERR_PTR(-ENOMEM); + mwifiex_dbg(adapter, ERROR, "cannot alloc DFS CAC queue\n"); + ret = -ENOMEM; + goto err_alloc_cac; } INIT_DELAYED_WORK(&priv->dfs_cac_work, mwifiex_dfs_cac_work_queue); @@ -3044,16 +3025,9 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, WQ_HIGHPRI | WQ_UNBOUND | WQ_MEM_RECLAIM, 1, name); if (!priv->dfs_chan_sw_workqueue) { - mwifiex_dbg(adapter, ERROR, - "cannot register virtual network device\n"); - free_netdev(dev); - priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED; - priv->netdev = NULL; - memset(&priv->wdev, 0, sizeof(priv->wdev)); - priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED; - destroy_workqueue(priv->dfs_cac_workqueue); - priv->dfs_cac_workqueue = NULL; - return ERR_PTR(-ENOMEM); + mwifiex_dbg(adapter, ERROR, "cannot alloc DFS channel sw queue\n"); + ret = -ENOMEM; + goto err_alloc_chsw; } INIT_DELAYED_WORK(&priv->dfs_chan_sw_work, @@ -3061,6 +3035,13 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, sema_init(&priv->async_sem, 1); + /* Register network device */ + if (register_netdevice(dev)) { + mwifiex_dbg(adapter, ERROR, "cannot register network device\n"); + ret = -EFAULT; + goto err_reg_netdev; + } + mwifiex_dbg(adapter, INFO, "info: %s: Marvell 802.11 Adapter\n", dev->name); @@ -3081,11 +3062,29 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, adapter->curr_iface_comb.p2p_intf++; break; default: + /* This should be dead code; checked above */ mwifiex_dbg(adapter, ERROR, "type not supported\n"); return ERR_PTR(-EINVAL); } return &priv->wdev; + +err_reg_netdev: + destroy_workqueue(priv->dfs_chan_sw_workqueue); + priv->dfs_chan_sw_workqueue = NULL; +err_alloc_chsw: + destroy_workqueue(priv->dfs_cac_workqueue); + priv->dfs_cac_workqueue = NULL; +err_alloc_cac: + free_netdev(dev); + priv->netdev = NULL; +err_sta_init: +err_set_bss_mode: +err_alloc_netdev: + memset(&priv->wdev, 0, sizeof(priv->wdev)); + priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED; + priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED; + return ERR_PTR(ret); } EXPORT_SYMBOL_GPL(mwifiex_add_virtual_intf);