Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
From:   gregory.greenman@intel.com
To:     johannes@sipsolutions.net
Cc:     linux-wireless@vger.kernel.org,
        Emmanuel Grumbach <emmanuel.grumbach@intel.com>,
        Gregory Greenman <gregory.greenman@intel.com>
Subject: [PATCH 15/18] wifi: mac80211: fix a expired vs. cancel race in roc
Date:   Thu, 28 Sep 2023 17:35:36 +0300
Message-Id: <20230928172905.4e4469be20ac.Iab0525f5cc4698acf23eab98b8b1eec02099cde0@changeid>
In-Reply-To: <20230928143539.1037500-1-gregory.greenman@intel.com>
References: <20230928143539.1037500-1-gregory.greenman@intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Emmanuel Grumbach <emmanuel.grumbach@intel.com>

When the remain on channel is removed at the time it should
have expired, we have a race: the driver could be handling
the flow of the expiration while mac80211 is cancelling
that very same remain on channel request.

This wouldn't be problem in itself, but since mac80211
can send the next request to the driver in the cancellation
flow, we can get to the following situation:

           CPU0                             CPU1
expiration of roc in driver
ieee80211_remain_on_channel_expired()
                                         Cancellation of the roc
schedules a worker (hw_roc_done)
                                         Add next roc
hw_roc_done_wk runs and ends
the second roc prematurely.

Since, by design, there is only one single request sent to the
driver at a time, we can safely assume that after the cancel()
request returns from the driver, we should not handle any worker
that handles the expiration of the request.

Cancel the hw_roc_done worker after the cancellation to make
sure we start the next one with a clean slate.

Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Signed-off-by: Gregory Greenman <gregory.greenman@intel.com>
---
 net/mac80211/offchannel.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index 071582dbe6a5..6c4080202573 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -717,6 +717,23 @@ static int ieee80211_cancel_roc(struct ieee80211_local *local,
 			return ret;
 		}
 
+		/*
+		 * We could be racing against the notification from the driver:
+		 *  + driver is handling the notification on CPU0
+		 *  + user space is cancelling the remain on channel and
+		 *    schedules the hw_roc_done worker.
+		 *
+		 *  Now hw_roc_done might start to run after the next roc will
+		 *  start and mac80211 will think that this second roc has
+		 *  ended prematurely.
+		 *  Cancel the work to make sure that all the pending workers
+		 *  have completed execution.
+		 *  Note that this assumes that by the time the driver returns
+		 *  from drv_cancel_remain_on_channel, it has completed all
+		 *  the processing of related notifications.
+		 */
+		wiphy_work_cancel(local->hw.wiphy, &local->hw_roc_done);
+
 		/* TODO:
 		 * if multiple items were combined here then we really shouldn't
 		 * cancel them all - we should wait for as much time as needed
-- 
2.38.1