Message ID | 20230425192607.18015-1-pchelkin@ispras.ru |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp3623915vqo; Tue, 25 Apr 2023 12:27:57 -0700 (PDT) X-Google-Smtp-Source: AKy350bQzRp10zJpkODpY72TjPxgspIN2eriQvkCUh9ZLR5ZqmR+8p0Jfpw/udEBId3DBwEAcBiD X-Received: by 2002:a05:6a21:6daa:b0:f3:81c1:feba with SMTP id wl42-20020a056a216daa00b000f381c1febamr12569995pzb.4.1682450877605; Tue, 25 Apr 2023 12:27:57 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1682450877; cv=none; d=google.com; s=arc-20160816; b=anWbOru2K19rQQcoW8J6Cy32Q0XkWt9vdsv/OoRBrB++T+FDLc4Vn4fcLT06rmNnSH iQdmuzL5BQh/IDa/BE3Isrb9UJaqfBiFdTDn0fhr2kh/qgXbCjXq/PrnMZK/xI+cTlVT rM73d7bNyAgada1NzwYwh8+7u3PgglLXoFd3utuXJlVSSDs5UpktkAb4EnLtokAM6dL6 PTzwbRalWH8H4DsjjNwlVkZe5G2qNI7hgGkphTV+xC9kf4kaOQiHHVR3VN+YqqPNmCGb tLUKKzXL6hIFdIXKITF/j04gNtxFeKXiAREo874vE49qW+OmHgcVANa5vmdsT+JtVfOR smLA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature:dkim-filter; bh=vtxoNftItfp9sdIuiR6TC+lugMM08soTNMG6xlHTUHY=; b=JM/PsevmVG/0+YcbeaGS1iPk1zJOhMmlw7GFIUJmQpMqUcZHBRW1dGht7LZ3vG7/nq aSAf4hC9qo1xpdJum500svhbaz/QruVnIinIDWnM/pIsz7XtOZrqREejnnsVedbCcodG r1ofD+xaepGTGOpRZD3kralndIUrQ+HIGCVfEP5yUqdhLZNi0XWnpsCxgYTyUq9mEsye 1lj+GFrVHOWIevBb4eegL4h4N1O39WtyVzSjTZpXmlR7SVpjI02+MPy2QDRwk5sah1Pk 5LnQwCRgCE5W1dA4e2vCNXVNWlpz9VqJPUO1ZsYQ6tHPVgCRZO82cQz3gaB7glQnB34H 8XEQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ispras.ru header.s=default header.b="hHs4+//l"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=ispras.ru Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 10-20020a63124a000000b0051f0d0d6359si11034483pgs.118.2023.04.25.12.27.42; Tue, 25 Apr 2023 12:27:57 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@ispras.ru header.s=default header.b="hHs4+//l"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=ispras.ru Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236040AbjDYT0h (ORCPT <rfc822;zxc52fgh@gmail.com> + 99 others); Tue, 25 Apr 2023 15:26:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47340 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236006AbjDYT0b (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 25 Apr 2023 15:26:31 -0400 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0B60516181; Tue, 25 Apr 2023 12:26:21 -0700 (PDT) Received: from fpc.intra.ispras.ru (unknown [10.10.165.4]) by mail.ispras.ru (Postfix) with ESMTPSA id AA7D440737A1; Tue, 25 Apr 2023 19:26:16 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 mail.ispras.ru AA7D440737A1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ispras.ru; s=default; t=1682450776; bh=vtxoNftItfp9sdIuiR6TC+lugMM08soTNMG6xlHTUHY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=hHs4+//l7vV8UdP9DwaVLuG8XDv+uZPdvuzCG+8MFti52uZty2gE84aRweqUZIFAQ b9RaIWs3ZkeIvOxVPB2gQ0Jjg2bCUCR5H5xy/LSRYOjFs1v4DeYERpO7JyRMgwnn9D 6nKCZLV/BySqjQd6RgrrvgrIklMSytHd5zxKyGLc= From: Fedor Pchelkin <pchelkin@ispras.ru> To: =?utf-8?q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>, Kalle Vallo <kvalo@kernel.org> Cc: Fedor Pchelkin <pchelkin@ispras.ru>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Senthil Balasubramanian <senthilkumar@atheros.com>, "John W. Linville" <linville@tuxdriver.com>, Vasanthakumar Thiagarajan <vasanth@atheros.com>, Sujith <Sujith.Manoharan@atheros.com>, linux-wireless@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Alexey Khoroshilov <khoroshilov@ispras.ru>, lvc-project@linuxtesting.org, syzbot+f2cb6e0ffdb961921e4d@syzkaller.appspotmail.com, Hillf Danton <hdanton@sina.com> Subject: [PATCH v3 1/2] wifi: ath9k: fix races between ath9k_wmi_cmd and ath9k_wmi_ctrl_rx Date: Tue, 25 Apr 2023 22:26:06 +0300 Message-Id: <20230425192607.18015-1-pchelkin@ispras.ru> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20230425033832.2041-1-hdanton@sina.com> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS, T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1764126371800470408?= X-GMAIL-MSGID: =?utf-8?q?1764177611441804311?= |
Series |
[v3,1/2] wifi: ath9k: fix races between ath9k_wmi_cmd and ath9k_wmi_ctrl_rx
|
|
Commit Message
Fedor Pchelkin
April 25, 2023, 7:26 p.m. UTC
Currently, the synchronization between ath9k_wmi_cmd() and
ath9k_wmi_ctrl_rx() is exposed to a race condition which, although being
rather unlikely, can lead to invalid behaviour of ath9k_wmi_cmd().
Consider the following scenario:
CPU0 CPU1
ath9k_wmi_cmd(...)
mutex_lock(&wmi->op_mutex)
ath9k_wmi_cmd_issue(...)
wait_for_completion_timeout(...)
---
timeout
---
/* the callback is being processed
* before last_seq_id became zero
*/
ath9k_wmi_ctrl_rx(...)
spin_lock_irqsave(...)
/* wmi->last_seq_id check here
* doesn't detect timeout yet
*/
spin_unlock_irqrestore(...)
/* last_seq_id is zeroed to
* indicate there was a timeout
*/
wmi->last_seq_id = 0
mutex_unlock(&wmi->op_mutex)
return -ETIMEDOUT
ath9k_wmi_cmd(...)
mutex_lock(&wmi->op_mutex)
/* the buffer is replaced with
* another one
*/
wmi->cmd_rsp_buf = rsp_buf
wmi->cmd_rsp_len = rsp_len
ath9k_wmi_cmd_issue(...)
spin_lock_irqsave(...)
spin_unlock_irqrestore(...)
wait_for_completion_timeout(...)
/* the continuation of the
* callback left after the first
* ath9k_wmi_cmd call
*/
ath9k_wmi_rsp_callback(...)
/* copying data designated
* to already timeouted
* WMI command into an
* inappropriate wmi_cmd_buf
*/
memcpy(...)
complete(&wmi->cmd_wait)
/* awakened by the bogus callback
* => invalid return result
*/
mutex_unlock(&wmi->op_mutex)
return 0
To fix this, update last_seq_id on timeout path inside ath9k_wmi_cmd()
under the wmi_lock. Move ath9k_wmi_rsp_callback() under wmi_lock inside
ath9k_wmi_ctrl_rx() so that the wmi->cmd_wait can be completed only for
initially designated wmi_cmd call, otherwise the path would be rejected
with last_seq_id check.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
v2: do not extract ath9k_wmi_rsp_callback() internals, rephrase
description
v3: per Hillf Danton's comment, protect last_seq_id with wmi_lock on
timeout path, divide the v2 version into two separate patches; the first
one is concerned with last_seq_id and completion under wmi_lock, the
second one is for moving rsp buffer and length recording under wmi lock.
Note that it's been only tested with Syzkaller and on basic driver
scenarios with real hardware.
drivers/net/wireless/ath/ath9k/wmi.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Comments
On Wed, Apr 26, 2023 at 07:07:08AM +0800, Hillf Danton wrote: > Given similar wait timeout[1], just taking lock on the waiter side is not > enough wrt fixing the race, because in case job done on the waker side, > waiter needs to wait again after timeout. > As I understand you correctly, you mean the case when a timeout occurs during ath9k_wmi_ctrl_rx() callback execution. I suppose if a timeout has occurred on a waiter's side, it should return immediately and doesn't have to care in which state the callback has been at that moment. AFAICS, this is controlled properly with taking a wmi_lock on waiter and waker sides, and there is no data corruption. If a callback has not managed to do its work entirely (performing a completion and subsequently waking waiting thread is included here), then, well, it is considered a timeout, in my opinion. Your suggestion makes a wmi_cmd call to give a little more chance for the belated callback to complete (although timeout has actually expired). That is probably good, but increasing a timeout value makes that job, too. I don't think it makes any sense on real hardware. Or do you mean there is data corruption that is properly fixed with your patch? That is, I agree there can be a situation when a callback makes all the logical work it should and it just hasn't got enough time to perform a completion before a timeout on waiter's side occurs. And this behaviour can be named "racy". But, technically, this seems to be a rather valid timeout. > [1] https://lore.kernel.org/lkml/9d9b9652-c1ac-58e9-2eab-9256c17b1da2@I-love.SAKURA.ne.jp/ > I don't think it's a similar case because wait_for_completion_state() is interruptible while wait_for_completion_timeout() is not. > A correct fix looks like after putting pieces together > > +++ b/drivers/net/wireless/ath/ath9k/wmi.c > @@ -238,6 +238,7 @@ static void ath9k_wmi_ctrl_rx(void *priv > spin_unlock_irqrestore(&wmi->wmi_lock, flags); > goto free_skb; > } > + wmi->last_seq_id = 0; > spin_unlock_irqrestore(&wmi->wmi_lock, flags); > > /* WMI command response */ > @@ -339,9 +340,20 @@ int ath9k_wmi_cmd(struct wmi *wmi, enum > > time_left = wait_for_completion_timeout(&wmi->cmd_wait, timeout); > if (!time_left) { > + unsigned long flags; > + int wait = 0; > + > ath_dbg(common, WMI, "Timeout waiting for WMI command: %s\n", > wmi_cmd_to_name(cmd_id)); > - wmi->last_seq_id = 0; > + > + spin_lock_irqsave(&wmi->wmi_lock, flags); > + if (wmi->last_seq_id == 0) /* job done on the waker side? */ > + wait = 1; > + else > + wmi->last_seq_id = 0; > + spin_unlock_irqrestore(&wmi->wmi_lock, flags); > + if (wait) > + wait_for_completion(&wmi->cmd_wait); > mutex_unlock(&wmi->op_mutex); > return -ETIMEDOUT; > }
Fedor Pchelkin <pchelkin@ispras.ru> writes: > On Wed, Apr 26, 2023 at 07:07:08AM +0800, Hillf Danton wrote: >> Given similar wait timeout[1], just taking lock on the waiter side is not >> enough wrt fixing the race, because in case job done on the waker side, >> waiter needs to wait again after timeout. >> > > As I understand you correctly, you mean the case when a timeout occurs > during ath9k_wmi_ctrl_rx() callback execution. I suppose if a timeout has > occurred on a waiter's side, it should return immediately and doesn't have > to care in which state the callback has been at that moment. > > AFAICS, this is controlled properly with taking a wmi_lock on waiter and > waker sides, and there is no data corruption. > > If a callback has not managed to do its work entirely (performing a > completion and subsequently waking waiting thread is included here), then, > well, it is considered a timeout, in my opinion. > > Your suggestion makes a wmi_cmd call to give a little more chance for the > belated callback to complete (although timeout has actually expired). That > is probably good, but increasing a timeout value makes that job, too. I > don't think it makes any sense on real hardware. > > Or do you mean there is data corruption that is properly fixed with your > patch? > > That is, I agree there can be a situation when a callback makes all the > logical work it should and it just hasn't got enough time to perform a > completion before a timeout on waiter's side occurs. And this behaviour > can be named "racy". But, technically, this seems to be a rather valid > timeout. > >> [1] https://lore.kernel.org/lkml/9d9b9652-c1ac-58e9-2eab-9256c17b1da2@I-love.SAKURA.ne.jp/ >> > > I don't think it's a similar case because wait_for_completion_state() is > interruptible while wait_for_completion_timeout() is not. Ping, Hillf? -Toke
On Thu, May 18, 2023 at 06:24:37PM +0800, Hillf Danton wrote: > Fedor Pchelkin <pchelkin@ispras.ru> writes: > > > On Wed, Apr 26, 2023 at 07:07:08AM +0800, Hillf Danton wrote: > >> Given similar wait timeout[1], just taking lock on the waiter side is not > >> enough wrt fixing the race, because in case job done on the waker side, > >> waiter needs to wait again after timeout. > >> > > > > As I understand you correctly, you mean the case when a timeout occurs > > during ath9k_wmi_ctrl_rx() callback execution. I suppose if a timeout has > > occurred on a waiter's side, it should return immediately and doesn't have > > to care in which state the callback has been at that moment. > > > > AFAICS, this is controlled properly with taking a wmi_lock on waiter and > > waker sides, and there is no data corruption. > > > > If a callback has not managed to do its work entirely (performing a > > completion and subsequently waking waiting thread is included here), then, > > well, it is considered a timeout, in my opinion. > > > > Your suggestion makes a wmi_cmd call to give a little more chance for the > > belated callback to complete (although timeout has actually expired). That > > is probably good, but increasing a timeout value makes that job, too. I > > don't think it makes any sense on real hardware. > > > > Or do you mean there is data corruption that is properly fixed with your patch? > > Given complete() not paired with wait_for_completion(), what is the > difference after this patch? The main thing in the patch is making ath9k_wmi_ctrl_rx() release wmi_lock after calling ath9k_wmi_rsp_callback() which does copying data into the shared wmi->cmd_rsp_buf buffer. Otherwise there can occur a data corrupting scenario outlined in the patch description (added it here, too). On Tue, 25 Apr 2023 22:26:06 +0300, Fedor Pchelkin wrote: > CPU0 CPU1 > > ath9k_wmi_cmd(...) > mutex_lock(&wmi->op_mutex) > ath9k_wmi_cmd_issue(...) > wait_for_completion_timeout(...) > --- > timeout > --- > /* the callback is being processed > * before last_seq_id became zero > */ > ath9k_wmi_ctrl_rx(...) > spin_lock_irqsave(...) > /* wmi->last_seq_id check here > * doesn't detect timeout yet > */ > spin_unlock_irqrestore(...) > /* last_seq_id is zeroed to > * indicate there was a timeout > */ > wmi->last_seq_id = 0 > mutex_unlock(&wmi->op_mutex) > return -ETIMEDOUT > > ath9k_wmi_cmd(...) > mutex_lock(&wmi->op_mutex) > /* the buffer is replaced with > * another one > */ > wmi->cmd_rsp_buf = rsp_buf > wmi->cmd_rsp_len = rsp_len > ath9k_wmi_cmd_issue(...) > spin_lock_irqsave(...) > spin_unlock_irqrestore(...) > wait_for_completion_timeout(...) > /* the continuation of the > * callback left after the first > * ath9k_wmi_cmd call > */ > ath9k_wmi_rsp_callback(...) > /* copying data designated > * to already timeouted > * WMI command into an > * inappropriate wmi_cmd_buf > */ > memcpy(...) > complete(&wmi->cmd_wait) > /* awakened by the bogus callback > * => invalid return result > */ > mutex_unlock(&wmi->op_mutex) > return 0 So before the patch the wmi->last_seq_id check in ath9k_wmi_ctrl_rx() wasn't helpful in case wmi->last_seq_id value was changed during ath9k_wmi_rsp_callback() execution because of the next ath9k_wmi_cmd() call. With the proposed patch the wmi->last_seq_id check in ath9k_wmi_ctrl_rx() accomplishes its job as: - the next ath9k_wmi_cmd call changes last_seq_id value under lock so it either waits for a belated ath9k_wmi_ctrl_rx() to finish or updates last_seq_id value so that the timeout check in ath9k_wmi_ctrl_rx() indicates that the waiter side has timeouted and we shouldn't further process the callback. - memcopying in ath9k_wmi_rsp_callback() is made to a valid place if the last_seq_id check was successful under the lock.
Fedor Pchelkin <pchelkin@ispras.ru> writes: > Currently, the synchronization between ath9k_wmi_cmd() and > ath9k_wmi_ctrl_rx() is exposed to a race condition which, although being > rather unlikely, can lead to invalid behaviour of ath9k_wmi_cmd(). > > Consider the following scenario: > > CPU0 CPU1 > > ath9k_wmi_cmd(...) > mutex_lock(&wmi->op_mutex) > ath9k_wmi_cmd_issue(...) > wait_for_completion_timeout(...) > --- > timeout > --- > /* the callback is being processed > * before last_seq_id became zero > */ > ath9k_wmi_ctrl_rx(...) > spin_lock_irqsave(...) > /* wmi->last_seq_id check here > * doesn't detect timeout yet > */ > spin_unlock_irqrestore(...) > /* last_seq_id is zeroed to > * indicate there was a timeout > */ > wmi->last_seq_id = 0 > mutex_unlock(&wmi->op_mutex) > return -ETIMEDOUT > > ath9k_wmi_cmd(...) > mutex_lock(&wmi->op_mutex) > /* the buffer is replaced with > * another one > */ > wmi->cmd_rsp_buf = rsp_buf > wmi->cmd_rsp_len = rsp_len > ath9k_wmi_cmd_issue(...) > spin_lock_irqsave(...) > spin_unlock_irqrestore(...) > wait_for_completion_timeout(...) > /* the continuation of the > * callback left after the first > * ath9k_wmi_cmd call > */ > ath9k_wmi_rsp_callback(...) > /* copying data designated > * to already timeouted > * WMI command into an > * inappropriate wmi_cmd_buf > */ > memcpy(...) > complete(&wmi->cmd_wait) > /* awakened by the bogus callback > * => invalid return result > */ > mutex_unlock(&wmi->op_mutex) > return 0 > > To fix this, update last_seq_id on timeout path inside ath9k_wmi_cmd() > under the wmi_lock. Move ath9k_wmi_rsp_callback() under wmi_lock inside > ath9k_wmi_ctrl_rx() so that the wmi->cmd_wait can be completed only for > initially designated wmi_cmd call, otherwise the path would be rejected > with last_seq_id check. > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller. > > Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> Alright, finally took the time to dig into this and convince myself that the fix if correct. Sorry for taking so long! Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
Fedor Pchelkin <pchelkin@ispras.ru> wrote: > Currently, the synchronization between ath9k_wmi_cmd() and > ath9k_wmi_ctrl_rx() is exposed to a race condition which, although being > rather unlikely, can lead to invalid behaviour of ath9k_wmi_cmd(). > > Consider the following scenario: > > CPU0 CPU1 > > ath9k_wmi_cmd(...) > mutex_lock(&wmi->op_mutex) > ath9k_wmi_cmd_issue(...) > wait_for_completion_timeout(...) > --- > timeout > --- > /* the callback is being processed > * before last_seq_id became zero > */ > ath9k_wmi_ctrl_rx(...) > spin_lock_irqsave(...) > /* wmi->last_seq_id check here > * doesn't detect timeout yet > */ > spin_unlock_irqrestore(...) > /* last_seq_id is zeroed to > * indicate there was a timeout > */ > wmi->last_seq_id = 0 > mutex_unlock(&wmi->op_mutex) > return -ETIMEDOUT > > ath9k_wmi_cmd(...) > mutex_lock(&wmi->op_mutex) > /* the buffer is replaced with > * another one > */ > wmi->cmd_rsp_buf = rsp_buf > wmi->cmd_rsp_len = rsp_len > ath9k_wmi_cmd_issue(...) > spin_lock_irqsave(...) > spin_unlock_irqrestore(...) > wait_for_completion_timeout(...) > /* the continuation of the > * callback left after the first > * ath9k_wmi_cmd call > */ > ath9k_wmi_rsp_callback(...) > /* copying data designated > * to already timeouted > * WMI command into an > * inappropriate wmi_cmd_buf > */ > memcpy(...) > complete(&wmi->cmd_wait) > /* awakened by the bogus callback > * => invalid return result > */ > mutex_unlock(&wmi->op_mutex) > return 0 > > To fix this, update last_seq_id on timeout path inside ath9k_wmi_cmd() > under the wmi_lock. Move ath9k_wmi_rsp_callback() under wmi_lock inside > ath9k_wmi_ctrl_rx() so that the wmi->cmd_wait can be completed only for > initially designated wmi_cmd call, otherwise the path would be rejected > with last_seq_id check. > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller. > > Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> 2 patches applied to ath-next branch of ath.git, thanks. b674fb513e2e wifi: ath9k: fix races between ath9k_wmi_cmd and ath9k_wmi_ctrl_rx 454994cfa9e4 wifi: ath9k: protect WMI command response buffer replacement with a lock
diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c index d652c647d56b..04f363cb90fe 100644 --- a/drivers/net/wireless/ath/ath9k/wmi.c +++ b/drivers/net/wireless/ath/ath9k/wmi.c @@ -242,10 +242,10 @@ static void ath9k_wmi_ctrl_rx(void *priv, struct sk_buff *skb, spin_unlock_irqrestore(&wmi->wmi_lock, flags); goto free_skb; } - spin_unlock_irqrestore(&wmi->wmi_lock, flags); /* WMI command response */ ath9k_wmi_rsp_callback(wmi, skb); + spin_unlock_irqrestore(&wmi->wmi_lock, flags); free_skb: kfree_skb(skb); @@ -308,8 +308,8 @@ int ath9k_wmi_cmd(struct wmi *wmi, enum wmi_cmd_id cmd_id, struct ath_common *common = ath9k_hw_common(ah); u16 headroom = sizeof(struct htc_frame_hdr) + sizeof(struct wmi_cmd_hdr); + unsigned long time_left, flags; struct sk_buff *skb; - unsigned long time_left; int ret = 0; if (ah->ah_flags & AH_UNPLUGGED) @@ -345,7 +345,9 @@ int ath9k_wmi_cmd(struct wmi *wmi, enum wmi_cmd_id cmd_id, if (!time_left) { ath_dbg(common, WMI, "Timeout waiting for WMI command: %s\n", wmi_cmd_to_name(cmd_id)); + spin_lock_irqsave(&wmi->wmi_lock, flags); wmi->last_seq_id = 0; + spin_unlock_irqrestore(&wmi->wmi_lock, flags); mutex_unlock(&wmi->op_mutex); return -ETIMEDOUT; }