From patchwork Fri Oct 15 13:17:38 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bryan O'Donoghue X-Patchwork-Id: 515851 Delivered-To: patch@linaro.org Received: by 2002:adf:a11e:0:0:0:0:0 with SMTP id o30csp544084wro; Fri, 15 Oct 2021 06:15:54 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw3fA9gBjlGmzmVtj3+vWy96WxtZoztXjT7BUrcASENwCMDKw0L6zgAyZp3w2E1cO+IL2kQ X-Received: by 2002:a17:902:b205:b0:13d:b0a1:da90 with SMTP id t5-20020a170902b20500b0013db0a1da90mr11314846plr.26.1634303754132; Fri, 15 Oct 2021 06:15:54 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1634303754; cv=none; d=google.com; s=arc-20160816; b=svxTvDXS5VPzW9u18L42VJqC7UwmfBfp6R56yb28xI6PQeTy3HF2kMiGvhG/27FAjU 1qDsPZNlhtrQMOr7jODrH4eQ+yz5lAMDlwv/6hmINeQXw71u7s5c8j45tHud5GuxU4E8 n6vx5LVXpwnj+OlKBlUteo1KoOZp7PAC/LKfbVmFh8V9g8IMn1P5oHNeGrpuRqM3Hhl4 iZ1RBUbprdzwqTGO6EEnfu0LQZdy/oY+UtAoSKnUVNUixwN1K2SIFAX7oJxpsvCIP3QF INlwOtYoM1GXOklb+L9FjyQVbNS04m52Snua3ijyR9xsDD7pHtnZTM490zr7U5G3vKoM Hz9A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=errors-to:sender:content-transfer-encoding:list-subscribe:list-help :list-post:list-archive:list-unsubscribe:list-id:precedence :mime-version:references:in-reply-to:message-id:date:subject:cc:to :from:dkim-signature:dkim-signature; bh=mwdq2KpJwCiS0cl5rzzhTPWLiDl1dRBzXewlUosaX8k=; b=vO/vnNxJWFXELZNnRQO5rkb5ksc5ydG26O0+ouzjwh7grFWaKIRuOVbSqsNXLL+KY6 E8llFK7LELUX+xEo/l3ncPzy9bOOpzkuWLqiZWG4OqaUKpH4wm2hYXE6I4zIZ5gRm6Ht bt2EUzA+MEm/BsQsLLLbkJQ+hoFaU+auEpM3p7W7oXV7VPWYiduCGXYN2UWBlUk0/Vb5 BIyq2F1pxuYnfYDki2KqFaRjqGuu+lzLrtK/GdL8zpelBkshqqRDs8nlOxESh3zFuSFM qANgPJUJDjksMoRZFs3Ape56LbyjOu0W/j1yKpM+viIuq6JdXm/i6vjVUhM1OZrgGOgw /CgQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@lists.infradead.org header.s=bombadil.20210309 header.b=DVoO968Y; dkim=neutral (body hash did not verify) header.i=@linaro.org header.s=google header.b=laUkFjTz; spf=neutral (google.com: 2607:7c80:54:e::133 is neither permitted nor denied by best guess record for domain of wcn36xx-bounces+patch=linaro.org@lists.infradead.org) smtp.mailfrom="wcn36xx-bounces+patch=linaro.org@lists.infradead.org"; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by mx.google.com with ESMTPS id n2si6991318pjh.91.2021.10.15.06.15.53 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 15 Oct 2021 06:15:54 -0700 (PDT) Received-SPF: neutral (google.com: 2607:7c80:54:e::133 is neither permitted nor denied by best guess record for domain of wcn36xx-bounces+patch=linaro.org@lists.infradead.org) client-ip=2607:7c80:54:e::133; Authentication-Results: mx.google.com; dkim=pass header.i=@lists.infradead.org header.s=bombadil.20210309 header.b=DVoO968Y; dkim=neutral (body hash did not verify) header.i=@linaro.org header.s=google header.b=laUkFjTz; spf=neutral (google.com: 2607:7c80:54:e::133 is neither permitted nor denied by best guess record for domain of wcn36xx-bounces+patch=linaro.org@lists.infradead.org) smtp.mailfrom="wcn36xx-bounces+patch=linaro.org@lists.infradead.org"; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linaro.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-Id:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=mwdq2KpJwCiS0cl5rzzhTPWLiDl1dRBzXewlUosaX8k=; b=DVoO968YC+R4Eb xztFc57xn/5OBEoEHAPq+HOO5exm8BB7QQhJiHgxC1fjtUsqayVVGebPd7ImsEzZW5w41acZkiq8C SVS+JpDsP36fV2FyDiPB7zXuj7ChaCbs2uHDUraE5wNx9+ansMowC439PLwm9/F+bRjxHGGv2NnaA RT6l+9c5E3aiNWoFSchTysPBG03E0yVLGFyKCbwwf9BRjn6bsWj/uwgLbpHO9iJMB9hC5jE7dxfs7 7ZcFLmJhCWRvhv2uSULW41M8Js7214+ARz2PWGDQnVpv8txsexQbw4VvAs5rHGjUz8Ruw45Ae7OCE zKIIDOkZmC6KzodtTe0Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mbN3w-007ATw-Dt; Fri, 15 Oct 2021 13:15:52 +0000 Received: from mail-wr1-x433.google.com ([2a00:1450:4864:20::433]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mbN3s-007AQj-8V for wcn36xx@lists.infradead.org; Fri, 15 Oct 2021 13:15:51 +0000 Received: by mail-wr1-x433.google.com with SMTP id v17so26401138wrv.9 for ; Fri, 15 Oct 2021 06:15:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=UHuhRsyo5I9tg994O+s+A8x/tyH5ZdCl1bmMze2pAnU=; b=laUkFjTz3t6pC1ovKbp5JgO8lCUtujhpQa+xjjbbWLawmNYdE1g/9ekgqTm4W8jzOa foPSTOdVpo8tlBF7eTxEyW//MPy+f18a8ch0UBY6zJ3SXP6cLApQ00a6jjS6fv81nuj7 thG30CCCKWlj/tm2exCsxBJ2vZCJhQwPZl4yGsKVHyHuyk12WdZ0DbkgfYrYTjiuftin d5AqhE81uNWP5u+awyqQvPzvJytVD1MUN+OfQN1FV/u66PHYvBiqCUrl5lJYxhvLeais 55KaNvlvNtayZQehr9KYU/0VnKP1b8BA6gH+xpZcR7C677wslyH2gXVsTHVqyQkSDAvM d2+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=UHuhRsyo5I9tg994O+s+A8x/tyH5ZdCl1bmMze2pAnU=; b=4lNWnJ1+98Fs4wyrL4IhhM9fnz4nRD3w/LoDyFn4ZW2+x558ejdwB7ZAreSMBsj3nv VPFhtNjp2iReeay6vXZRxJA7EJV4GgPKCTInB6y8duf/ajAMQq+++Th21fgOvLoKjcnw fz3Bcqc0ceZfycogpclRGpnXzfciSazf7W5qArmqSTdl0oNuzbP5kadr2RDu7SoX8jPt yKsvD9x8fNnmlsWmb3Lek58cZ9fRrszJiE7HiIQybdJCZDhrd8YjqSdFMiWghgeA+N4p 3YJcFjnbzKqrt2M7hFWGlVHmuQAcE/nJTeOSa4EKfxzOeo49GJrOUx6kH2yOnVsm+stp Zy/A== X-Gm-Message-State: AOAM531A1lj3+Y55h9GJ1Pqj848mnTR7+wLpKOyN/YUUYaB6+Qu3r369 zzVFxNV4bMU8CdzmmmPstY6vVA== X-Received: by 2002:a5d:6245:: with SMTP id m5mr14385783wrv.148.1634303744292; Fri, 15 Oct 2021 06:15:44 -0700 (PDT) Received: from sagittarius-a.chello.ie (188-141-3-169.dynamic.upc.ie. [188.141.3.169]) by smtp.gmail.com with ESMTPSA id e8sm7091716wrg.48.2021.10.15.06.15.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 15 Oct 2021 06:15:43 -0700 (PDT) From: Bryan O'Donoghue To: kvalo@codeaurora.org, linux-wireless@vger.kernel.org, wcn36xx@lists.infradead.org Cc: loic.poulain@linaro.org, benl@squareup.com, daniel.thompson@linaro.org, bryan.odonoghue@linaro.org Subject: [PATCH 1/4] wcn36xx: Fix DXE lock layering violation Date: Fri, 15 Oct 2021 14:17:38 +0100 Message-Id: <20211015131741.2455824-2-bryan.odonoghue@linaro.org> X-Mailer: git-send-email 2.33.0 In-Reply-To: <20211015131741.2455824-1-bryan.odonoghue@linaro.org> References: <20211015131741.2455824-1-bryan.odonoghue@linaro.org> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211015_061548_352275_EC9AC857 X-CRM114-Status: GOOD ( 15.32 ) X-Spam-Score: -0.2 (/) X-Spam-Report: Spam detection software, running on the system "bombadil.infradead.org", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: Looking at the code here we see that txrx.c is taking the dxe.c lock to set and unset the current TX skbuff pointer. There is no obvious logical bug however, it is a layering violation to share locks like this. Content analysis details: (-0.2 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at https://www.dnswl.org/, no trust [2a00:1450:4864:20:0:0:0:433 listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID_EF Message has a valid DKIM or DK signature from envelope-from domain X-BeenThere: wcn36xx@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "wcn36xx" Errors-To: wcn36xx-bounces+patch=linaro.org@lists.infradead.org Looking at the code here we see that txrx.c is taking the dxe.c lock to set and unset the current TX skbuff pointer. There is no obvious logical bug however, it is a layering violation to share locks like this. Lets tidy up the code a bit by making access functions to set and unset the TX sbuff. This makes it easier to read and reason about this code without having to switch between multiple files. Fixes: 8e84c2582169 ("wcn36xx: mac80211 driver for Qualcomm WCN3660/WCN3680 hardware") Signed-off-by: Bryan O'Donoghue --- drivers/net/wireless/ath/wcn36xx/dxe.c | 26 +++++++++++++++++++++++++ drivers/net/wireless/ath/wcn36xx/dxe.h | 2 ++ drivers/net/wireless/ath/wcn36xx/txrx.c | 15 ++------------ 3 files changed, 30 insertions(+), 13 deletions(-) -- 2.33.0 _______________________________________________ wcn36xx mailing list wcn36xx@lists.infradead.org http://lists.infradead.org/mailman/listinfo/wcn36xx diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c index 8e1dbfda65386..4e898bde1bb8c 100644 --- a/drivers/net/wireless/ath/wcn36xx/dxe.c +++ b/drivers/net/wireless/ath/wcn36xx/dxe.c @@ -831,6 +831,32 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn, return ret; } +int wcn36xx_dxe_set_tx_ack_skb(struct wcn36xx *wcn, struct sk_buff *skb) +{ + unsigned long flags; + + spin_lock_irqsave(&wcn->dxe_lock, flags); + if (wcn->tx_ack_skb) { + spin_unlock_irqrestore(&wcn->dxe_lock, flags); + wcn36xx_warn("tx_ack_skb already set\n"); + return -EINVAL; + } + + wcn->tx_ack_skb = skb; + spin_unlock_irqrestore(&wcn->dxe_lock, flags); + + return 0; +} + +void wcn36xx_dxe_unset_tx_ack_skb(struct wcn36xx *wcn) +{ + unsigned long flags; + + spin_lock_irqsave(&wcn->dxe_lock, flags); + wcn->tx_ack_skb = NULL; + spin_unlock_irqrestore(&wcn->dxe_lock, flags); +} + int wcn36xx_dxe_init(struct wcn36xx *wcn) { int reg_data = 0, ret; diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.h b/drivers/net/wireless/ath/wcn36xx/dxe.h index 31b81b7547a32..083a95e7de576 100644 --- a/drivers/net/wireless/ath/wcn36xx/dxe.h +++ b/drivers/net/wireless/ath/wcn36xx/dxe.h @@ -467,4 +467,6 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn, struct sk_buff *skb, bool is_low); void wcn36xx_dxe_tx_ack_ind(struct wcn36xx *wcn, u32 status); +int wcn36xx_dxe_set_tx_ack_skb(struct wcn36xx *wcn, struct sk_buff *skb); +void wcn36xx_dxe_unset_tx_ack_skb(struct wcn36xx *wcn); #endif /* _DXE_H_ */ diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c index cab196bb38cd4..969210812cfbb 100644 --- a/drivers/net/wireless/ath/wcn36xx/txrx.c +++ b/drivers/net/wireless/ath/wcn36xx/txrx.c @@ -502,7 +502,6 @@ int wcn36xx_start_tx(struct wcn36xx *wcn, struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data; struct wcn36xx_vif *vif_priv = NULL; struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb); - unsigned long flags; bool is_low = ieee80211_is_data(hdr->frame_control); bool bcast = is_broadcast_ether_addr(hdr->addr1) || is_multicast_ether_addr(hdr->addr1); @@ -524,15 +523,8 @@ int wcn36xx_start_tx(struct wcn36xx *wcn, if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) { wcn36xx_dbg(WCN36XX_DBG_DXE, "TX_ACK status requested\n"); - spin_lock_irqsave(&wcn->dxe_lock, flags); - if (wcn->tx_ack_skb) { - spin_unlock_irqrestore(&wcn->dxe_lock, flags); - wcn36xx_warn("tx_ack_skb already set\n"); + if (wcn36xx_dxe_set_tx_ack_skb(wcn, skb)) return -EINVAL; - } - - wcn->tx_ack_skb = skb; - spin_unlock_irqrestore(&wcn->dxe_lock, flags); /* Only one at a time is supported by fw. Stop the TX queues * until the ack status gets back. @@ -562,10 +554,7 @@ int wcn36xx_start_tx(struct wcn36xx *wcn, /* If the skb has not been transmitted, * don't keep a reference to it. */ - spin_lock_irqsave(&wcn->dxe_lock, flags); - wcn->tx_ack_skb = NULL; - spin_unlock_irqrestore(&wcn->dxe_lock, flags); - + wcn36xx_dxe_unset_tx_ack_skb(wcn); ieee80211_wake_queues(wcn->hw); }