diff mbox series

[3/8] wifi: mac80211: make associated BSS pointer visible to the driver

Message ID 20240206164849.6fe9782b87b4.Ifbffef638f07ca7f5c2b27f40d2cf2942d21de0b@changeid
State New
Headers show
Series cfg80211/mac80211 patches from our internal tree 2024-02-06 | expand

Commit Message

Miri Korenblit Feb. 6, 2024, 2:54 p.m. UTC 
Some drivers need the data in it, so move it to the link conf,
which is exposed to the driver.

Signed-off-by: Miri Korenblit <miriam.rachel.korenblit@intel.com>
Reviewed-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/mac80211.h |  3 +++
 net/mac80211/mlme.c    | 18 +++++++++---------
 2 files changed, 12 insertions(+), 9 deletions(-)

Comments

Johannes Berg Feb. 12, 2024, 1:45 p.m. UTC | #1 
On Tue, 2024-02-06 at 16:54 +0200, Miri Korenblit wrote:
> Some drivers need the data in it, so move it to the link conf,
> which is exposed to the driver.

> + * @bss: the cfg80211 bss descriptor. Valid only for a station, and only
> + *	when associated.
> 

For the record, I'm dropping this patch.

Yes, the data is there in the stack, but the cfg80211 BSS contains data
that is not authenticated (from probe responses etc.) whereas mac80211
is (hopefully always) operating on data that is authenticated with
beacon protection.

So exposing this to the driver feels fragile - if it actually gets used
for pretty much anything, it won't necessarily be authenticated data.

We were planning to use it for just the RSSI, which is probably fine,
but we can do that differently.

johannes
Johannes Berg Feb. 20, 2024, 8:29 a.m. UTC | #2 
On Mon, 2024-02-12 at 14:45 +0100, Johannes Berg wrote:
> On Tue, 2024-02-06 at 16:54 +0200, Miri Korenblit wrote:
> > Some drivers need the data in it, so move it to the link conf,
> > which is exposed to the driver.
> 
> > + * @bss: the cfg80211 bss descriptor. Valid only for a station, and only
> > + *	when associated.
> > 
> 
> For the record, I'm dropping this patch.
> 
> Yes, the data is there in the stack, but the cfg80211 BSS contains data
> that is not authenticated (from probe responses etc.) whereas mac80211
> is (hopefully always) operating on data that is authenticated with
> beacon protection.
> 
> So exposing this to the driver feels fragile - if it actually gets used
> for pretty much anything, it won't necessarily be authenticated data.

Changed my mind again ;-)

The issue ends up being as described above, but there are some other
things that the iwlwifi driver would like to access that are most easily
obtained via this information, not just RSSI. The other thing on our
list now is the BSS load, and we had a patch [1] to parse it in mac80211
and then it's authenticated, but really we don't care much for security
purposes and would like to also have it from probe responses which are
never authenticated.

[1] https://lore.kernel.org/r/20240216135047.b771830d9b12.If5885d651cb0114711ee1f6c1cb8fe31a69bf0a7@changeid


So I'll apply this patch with a documentation change to warn the data
contained is not authenticated, and drop the patch [1] linked above
instead. This gives the driver more flexibility, at the expense of
perhaps a slightly greater risk of using it for something it shouldn't
be used for, but the alternatives would put more driver-specific logic
into mac80211, which we don't really want either.

johannes
diff mbox series

Patch

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 8d6ae22c09bf..9c6ffdc248f0 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -539,6 +539,8 @@  struct ieee80211_fils_discovery {
  * to that BSS) that can change during the lifetime of the BSS.
  *
  * @vif: reference to owning VIF
+ * @bss: the cfg80211 bss descriptor. Valid only for a station, and only
+ *	when associated.
  * @addr: (link) address used locally
  * @link_id: link ID, or 0 for non-MLO
  * @htc_trig_based_pkt_ext: default PE in 4us units, if BSS supports HE
@@ -684,6 +686,7 @@  struct ieee80211_fils_discovery {
  */
 struct ieee80211_bss_conf {
 	struct ieee80211_vif *vif;
+	struct cfg80211_bss *bss;
 
 	const u8 *bssid;
 	unsigned int link_id;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index cc9a8eaffa6b..0a03b5bb2c5a 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1857,7 +1857,7 @@  ieee80211_sta_process_chanswitch(struct ieee80211_link_data *link,
 	struct ieee80211_sub_if_data *sdata = link->sdata;
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
-	struct cfg80211_bss *cbss = link->u.mgd.bss;
+	struct cfg80211_bss *cbss = link->conf->bss;
 	struct ieee80211_chanctx_conf *conf;
 	struct ieee80211_chanctx *chanctx;
 	enum nl80211_band current_band;
@@ -2769,7 +2769,7 @@  static u64 ieee80211_link_set_associated(struct ieee80211_link_data *link,
 
 	ieee80211_check_rate_mask(link);
 
-	link->u.mgd.bss = cbss;
+	link->conf->bss = cbss;
 	memcpy(link->u.mgd.bssid, cbss->bssid, ETH_ALEN);
 
 	if (sdata->vif.p2p ||
@@ -2917,7 +2917,7 @@  static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 	ifmgd->associated = false;
 
 	/* other links will be destroyed */
-	sdata->deflink.u.mgd.bss = NULL;
+	sdata->deflink.conf->bss = NULL;
 
 	netif_carrier_off(sdata->dev);
 
@@ -3245,7 +3245,7 @@  static void ieee80211_mgd_probe_ap_send(struct ieee80211_sub_if_data *sdata)
 		ieee80211_mlme_send_probe_req(sdata, sdata->vif.addr, dst,
 					      sdata->vif.cfg.ssid,
 					      sdata->vif.cfg.ssid_len,
-					      sdata->deflink.u.mgd.bss->channel);
+					      sdata->deflink.conf->bss->channel);
 	}
 
 	ifmgd->probe_timeout = jiffies + msecs_to_jiffies(probe_wait_ms);
@@ -3328,7 +3328,7 @@  struct sk_buff *ieee80211_ap_probereq_get(struct ieee80211_hw *hw,
 		return NULL;
 
 	if (ifmgd->associated)
-		cbss = sdata->deflink.u.mgd.bss;
+		cbss = sdata->deflink.conf->bss;
 	else if (ifmgd->auth_data)
 		cbss = ifmgd->auth_data->bss;
 	else if (ifmgd->assoc_data && ifmgd->assoc_data->link[0].bss)
@@ -3407,8 +3407,8 @@  static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
 			link = sdata_dereference(sdata->link[link_id], sdata);
 			if (!link)
 				continue;
-			cfg80211_unlink_bss(local->hw.wiphy, link->u.mgd.bss);
-			link->u.mgd.bss = NULL;
+			cfg80211_unlink_bss(local->hw.wiphy, link->conf->bss);
+			link->conf->bss = NULL;
 		}
 	}
 
@@ -6246,7 +6246,7 @@  static void ieee80211_rx_mgmt_beacon(struct ieee80211_link_data *link,
 	}
 
 	if (!ifmgd->associated ||
-	    !ieee80211_rx_our_beacon(bssid, link->u.mgd.bss))
+	    !ieee80211_rx_our_beacon(bssid, link->conf->bss))
 		return;
 	bssid = link->u.mgd.bssid;
 
@@ -6273,7 +6273,7 @@  static void ieee80211_rx_mgmt_beacon(struct ieee80211_link_data *link,
 	 */
 	if (!ieee80211_is_s1g_beacon(hdr->frame_control))
 		ncrc = crc32_be(0, (void *)&mgmt->u.beacon.beacon_int, 4);
-	parse_params.bss = link->u.mgd.bss;
+	parse_params.bss = link->conf->bss;
 	parse_params.filter = care_about_ies;
 	parse_params.crc = ncrc;
 	elems = ieee802_11_parse_elems_full(&parse_params);