Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <a9bc44dcb782d3fd37311db511c943f7828821b2.camel@sipsolutions.net>
Subject: Re: [PATCH 1/5] mac80211: Random MAC address for a Management frame
 exchange
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Jouni Malinen <jouni@codeaurora.org>
Cc:     linux-wireless@vger.kernel.org
Date:   Wed, 29 Apr 2020 16:14:54 +0200
In-Reply-To: <20200425155713.25687-1-jouni@codeaurora.org>
References: <20200425155713.25687-1-jouni@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 (3.34.4-1.fc31) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Sat, 2020-04-25 at 18:57 +0300, Jouni Malinen wrote:

> +++ b/net/mac80211/offchannel.c
> @@ -955,6 +955,12 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>  		IEEE80211_SKB_CB(skb)->hw_queue =
>  			local->hw.offchannel_tx_hw_queue;
>  
> +	/* remember a random MAC address for Management frame exchange */
> +	if (wiphy_ext_feature_isset(wiphy,
> +				    NL80211_EXT_FEATURE_MGMT_TX_RANDOM_TA) &&
> +	    !ether_addr_equal(mgmt->sa, wdev_address(wdev)))
> +		memcpy(local->mgmt_tx_rand_addr, mgmt->sa, ETH_ALEN);
> +
>  	/* This will handle all kinds of coalescing and immediate TX */
>  	ret = ieee80211_start_roc_work(local, sdata, params->chan,
>  				       params->wait, cookie, skb,

This feels wrong to me. It seems it should be made part of the roc work
item, and only copied over when that item actually starts, and also used
to not coalesce different items if they specify conflicting temporary
addresses.

> @@ -971,6 +977,9 @@ int ieee80211_mgmt_tx_cancel_wait(struct wiphy *wiphy,
>  {
>  	struct ieee80211_local *local = wiphy_priv(wiphy);
>  
> +	/* stop using the random MAC address for Management frame exchange */
> +	eth_zero_addr(local->mgmt_tx_rand_addr);
> +
>  	return ieee80211_cancel_roc(local, cookie, true);

Similar here, not clear that the ROC item even started yet at this
point.

It seems to me it needs to be pushed a layer deeper, say into
ieee80211_handle_roc_started() and ieee80211_offchannel_return() or so.

johannes