From patchwork Mon Oct 18 23:17:18 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: 515926 Delivered-To: patch@linaro.org Received: by 2002:ac0:cd8c:0:0:0:0:0 with SMTP id d12csp32630imp; Mon, 18 Oct 2021 16:15:30 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz7qmN0L9eyiT8hHRscOCeT1etvI6qpisY9b2vl6zryq7Pj7Gt39mtwo8WPjMmpvWhija4r X-Received: by 2002:a9d:72de:: with SMTP id d30mr2268147otk.213.1634598930310; Mon, 18 Oct 2021 16:15:30 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1634598930; cv=none; d=google.com; s=arc-20160816; b=xbiT02Wfu6HR+OpkYE7K1FSgNLWkdz2iEyOuSbkQc9NK7XrkN4r9b8PXGekj5cOXcz 9YbDDDCgZhTNvRab9XURuTCLoSuFIIJMXwR2FPZfx2v6AUsN/3HloIQqoIB1//+bauWB 3lKkhbI9UhTqRfZHVjwcAVxTv1vVIovjqoYzdwlAu9xSc+yOKLke6SAGFD5NIzn2o/9F 1+R1DlMzQp5tcKJC5AxyBBOF1GNBhmc+FUWlJsNcawNVW4Vmuq2I/9iAyzWMXHsmm4qE 5qhR3enO5HwaRCxSivU2d61SxTwxIiSPjsYzmaTldgVFrITaL8occbjzI2CfDt/+IFtS JQWA== 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=N63KxLS0hABD9B8/d2Tzjke7oHDWPpw3CXsGjevNJS8=; b=CxaDBXb08BjUntDP53hsp42TUM+RldRld8EKOxCoal84uqPxsOkgMVtRCRz7CS0w5a 3aMjfr+qd72UPG7Kry65ZkL1R/pX37Dt6gkIGzf60UYh6oA2ADOfZaMu6t4oMaO5f77U FyDCsxPQPegUYTd837a7KQaaQs4OtAUeWkdY+doR4K8p/CYPpZEvNRmL0aHjlL5vzlm3 TuwYlRYbf1V2eSo+Fi4scDgGcek2pvZOl7iCY0mKjSTvKa9erbeSz7tlH/Uqm7VcqjoU 7s+VvC0Kj+E2FVJBV5s4eSmQH4qAMsSv4Wq/Our482hqJxQiRL/M1Z7bK5dgoLDsufFS c5Rg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@lists.infradead.org header.s=bombadil.20210309 header.b=DCt89Kr0; dkim=neutral (body hash did not verify) header.i=@linaro.org header.s=google header.b=Y0ODsvaf; spf=pass (google.com: best guess record for domain of wcn36xx-bounces+patch=linaro.org@lists.infradead.org designates 2607:7c80:54:e::133 as permitted sender) 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 (bombadil.infradead.org. [2607:7c80:54:e::133]) by mx.google.com with ESMTPS id 8si14789731otw.22.2021.10.18.16.15.30 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Oct 2021 16:15:30 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of wcn36xx-bounces+patch=linaro.org@lists.infradead.org designates 2607:7c80:54:e::133 as permitted sender) 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=DCt89Kr0; dkim=neutral (body hash did not verify) header.i=@linaro.org header.s=google header.b=Y0ODsvaf; spf=pass (google.com: best guess record for domain of wcn36xx-bounces+patch=linaro.org@lists.infradead.org designates 2607:7c80:54:e::133 as permitted sender) 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=N63KxLS0hABD9B8/d2Tzjke7oHDWPpw3CXsGjevNJS8=; b=DCt89Kr0QmVBTY KhLjjYZ2QiybvzWt4FuYlM/Th/fw70PvdMFfwqy41MO2vkuKR3ZeIZIA5d8VkhXk4HQHExRUc2WoA h/mzdsta4mw94cIlM5CguO/1xJR/Dj/LjKmlPQabvllIWSrnWR8w6IK4wmHpvvaQr3FOKdiTyCC6p Ql62Zah+8LzIlm3ln/6WjHWGwxWdQsMZKAtLNUV39U7nDr7gI5ZEnKp/7NEDdUjHPiUtoEoL0ghH4 UK6oDpWCtfFm4/OXgKQd+CAHLGnPrz+tO+UdSOASuTQLGVXAI6Ohx6OHNE3CJvdKBg42Zi6Kvpv8H f5yjLrSYUBZKGvBPWnKA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mcbqq-00HR9N-QV; Mon, 18 Oct 2021 23:15:28 +0000 Received: from mail-wr1-x42b.google.com ([2a00:1450:4864:20::42b]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mcbqo-00HR8T-JM for wcn36xx@lists.infradead.org; Mon, 18 Oct 2021 23:15:28 +0000 Received: by mail-wr1-x42b.google.com with SMTP id r7so44243241wrc.10 for ; Mon, 18 Oct 2021 16:15:26 -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=Cqgq7CmylZ5uOrbViyRL+gfi0biEauoJtPG6puA4TVg=; b=Y0ODsvaf5ZQxj5xz7bmOLKad1kYhn+RH4Ua0wsqAeZod6Uq9jKGhPwNCJG4hxeEkSL FM9WV4HKZtzBGJySM7/sP6V/Vf5l/mMGXb49vJqqFj1zMJ/ujbXntX6cpUINNPI0ZDZ3 6Hcu/LgwjoWIezkxF6NcQFlfYgjUgn60z/bCsjS3k/PEHreq6fB/psqCqxXKG5CnskBp nWpwcwHY7CVNEljum3cTy+hLSZewKVzBSBhLsYiiaWJeU7w507uBc3Ju3hiBJNjVwW+y iBYTVYFgHzTCjBnxmZDOR4tK4gcwSeZNsIZb+jmhO4WyDgwPRk/30KMNJ+TOfnSOrpSJ Lqqg== 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=Cqgq7CmylZ5uOrbViyRL+gfi0biEauoJtPG6puA4TVg=; b=7yuVCGQ+P++pc9Ht7IQ8YqZwjYuOhGiNahA2A3/h+9RmCwpMXGJP5MBZPx5mDcznrU +hp2PNm5uC5VlygZLf/CQrXXxaRuHjl9nF6eVWIsfepooqwYT605nke93k29P89nz7Xp OKcpzf2eRGTqMi/4T2ZfN1yNTub3BRq0yhMRbRmrXfC/ojjP7wICpUYswwH2EMinmglB Mh3En5weuENyzKKWDj6VbhY9Y9Qw6DaSQflUbGoU5Syq3LXMve1oF52XZkkn+vcn6qer MfiyPsLTmoBvTkxr2E4GQ+8rR0dDK0bCNLZECtvQNkBz2RSycqWeh9fYc3djDEl7QKgQ BGsA== X-Gm-Message-State: AOAM530nhnXfUviOznvAZr4Y89fJNcXybg0eCpIbfw6PGKBnpSCzN5N1 dwKKYTy3ES2bYI9e9rE+STbeLQ== X-Received: by 2002:adf:d1ee:: with SMTP id g14mr39642914wrd.264.1634598925129; Mon, 18 Oct 2021 16:15:25 -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 s8sm3685379wrr.15.2021.10.18.16.15.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Oct 2021 16:15:24 -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, johannes@sipsolutions.net, bryan.odonoghue@linaro.org Subject: [PATCH v2 1/5] wcn36xx: Fix dxe lock layering violation Date: Tue, 19 Oct 2021 00:17:18 +0100 Message-Id: <20211018231722.873525-2-bryan.odonoghue@linaro.org> X-Mailer: git-send-email 2.33.0 In-Reply-To: <20211018231722.873525-1-bryan.odonoghue@linaro.org> References: <20211018231722.873525-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-20211018_161526_662882_CDCBD472 X-CRM114-Status: GOOD ( 15.95 ) 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: We are looking at some interesting crashes with wcn36xx in the wild, with some of the data appearing to indicate multiple instances of "WARNING Spurious TX complete indication". 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. 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:42b 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 We are looking at some interesting crashes with wcn36xx in the wild, with some of the data appearing to indicate multiple instances of "WARNING Spurious TX complete indication". 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 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 c4e9e939d7d6d..6c43df4bc92c3 100644 --- a/drivers/net/wireless/ath/wcn36xx/dxe.c +++ b/drivers/net/wireless/ath/wcn36xx/dxe.c @@ -878,6 +878,32 @@ int wcn36xx_dxe_tx_flush(struct wcn36xx *wcn) return -EBUSY; } +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 26a31edf52e99..9a7655d6af982 100644 --- a/drivers/net/wireless/ath/wcn36xx/dxe.h +++ b/drivers/net/wireless/ath/wcn36xx/dxe.h @@ -468,4 +468,6 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn, bool is_low); int wcn36xx_dxe_tx_flush(struct wcn36xx *wcn); 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 d727b0dd98de5..1218bd85de3ba 100644 --- a/drivers/net/wireless/ath/wcn36xx/txrx.c +++ b/drivers/net/wireless/ath/wcn36xx/txrx.c @@ -584,7 +584,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); @@ -606,15 +605,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. @@ -644,10 +636,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); }