Message-ID: <20231201114329.c43ed5db7146.Idd29862133993877b9fdff962dca3649e842249a@changeid>
Date: Fri,  1 Dec 2023 11:41:17 +0100
From: Johannes Berg <johannes@...solutions.net>
To: linux-wireless@...r.kernel.org
Cc: netdev@...r.kernel.org,
	Johannes Berg <johannes.berg@...el.com>
Subject: [PATCH wireless-next 3/3] wifi: nl80211: report carrier up count to userspace

From: Johannes Berg <johannes.berg@...el.com>

There's a race in the carrier change that happens if userspace
sees the RX association event via nl80211, but the driver (or
mac80211) just prior to that set the carrier on. The carrier
on event is actually only processed by the link watch work, so
userspace can (and we've seen this at least in simulation with
ARCH=um and time-travel) attempt to send a frame before that
has run, if it was just waiting for the association to finish
(only on open connections, of course, for encryption it has to
go through the 4-way-handshake first before sending data frames.)

There's really no completely good way to address this, I've
previously analyzed this here:
https://lore.kernel.org/netdev/346b21d87c69f817ea3c37caceb34f1f56255884.camel@sipsolutions.net/

This new solution requires both kernel and userspace work, it
basically builds on #3 outlined in the email linked above, but
with the addition of letting userspace _know_ that it may need
to wait for the rtnetlink event.

So to solve it, with this change userspace can see the value of
the carrier_up_count at the association event, and if it hasn't
yet seen the same value via an rtnetlink event (it is imperative
that it doesn't query, it must wait for events) then it can wait
for that event before trying to send data frames.

For now, implement this for association and IBSS joined events.

Signed-off-by: Johannes Berg <johannes.berg@...el.com>
---
 include/uapi/linux/nl80211.h | 16 ++++++++++++
 net/wireless/nl80211.c       | 47 ++++++++++++++++--------------------
 2 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 0cd1da2c2902..120936f81a28 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2830,6 +2830,20 @@ enum nl80211_commands {
  * @NL80211_ATTR_MLO_LINK_DISABLED: Flag attribute indicating that the link is
  *	disabled.
  *
+ * @NL80211_ATTR_CARRIER_UP_COUNT: This u32 attribute is included in some
+ *	events that indicate a successful connection (notably association),
+ *	indicating the value of the netdev's carrier_up_count at the time
+ *	of sending this event. Userspace can use this to fix a race: when
+ *	the carrier is turned on, the actual handling thereof is done in
+ *	an asynchronous manner in the kernel. Thus, if userspace attempts
+ *	to send a frame immediately after receiving the event indicating
+ *	successful connection over nl80211, that may not go through if the
+ *	asynchronous processing in the kernel hasn't yet happened. To fix
+ *	it then userspace should be listening to rtnetlink events, and if
+ *	it didn't see the value of the carrier_up_count yet, it can wait
+ *	for a further rtnetlink event with a value equal to or bigger than
+ *	the value reported here, and only then transmit data.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -3368,6 +3382,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_MLO_LINK_DISABLED,
 
+	NL80211_ATTR_CARRIER_UP_COUNT,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 403a4a38966a..d91a99f90aaa 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -818,6 +818,7 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[NL80211_ATTR_HW_TIMESTAMP_ENABLED] = { .type = NLA_FLAG },
 	[NL80211_ATTR_EMA_RNR_ELEMS] = { .type = NLA_NESTED },
 	[NL80211_ATTR_MLO_LINK_DISABLED] = { .type = NLA_FLAG },
+	[NL80211_ATTR_CARRIER_UP_COUNT] = { .type = NLA_REJECT },
 };
 
 /* policy for the key attributes */
@@ -17738,11 +17739,13 @@ void nl80211_common_reg_change_event(enum nl80211_commands cmd_id,
 
 struct nl80211_mlme_event {
 	enum nl80211_commands cmd;
+	const u8 *mac_addr;
 	const u8 *buf;
 	size_t buf_len;
 	int uapsd_queues;
 	const u8 *req_ies;
 	size_t req_ies_len;
+	u32 carrier_up_count;
 	bool reconnect;
 };
 
@@ -17766,12 +17769,17 @@ static void nl80211_send_mlme_event(struct cfg80211_registered_device *rdev,
 
 	if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) ||
 	    nla_put_u32(msg, NL80211_ATTR_IFINDEX, netdev->ifindex) ||
-	    nla_put(msg, NL80211_ATTR_FRAME, event->buf_len, event->buf) ||
+	    (event->buf &&
+	     nla_put(msg, NL80211_ATTR_FRAME, event->buf_len, event->buf)) ||
 	    (event->req_ies &&
 	     nla_put(msg, NL80211_ATTR_REQ_IE, event->req_ies_len,
 		     event->req_ies)))
 		goto nla_put_failure;
 
+	if (event->mac_addr &&
+	    nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, event->mac_addr))
+		goto nla_put_failure;
+
 	if (event->reconnect &&
 	    nla_put_flag(msg, NL80211_ATTR_RECONNECT_REQUESTED))
 		goto nla_put_failure;
@@ -17789,6 +17797,11 @@ static void nl80211_send_mlme_event(struct cfg80211_registered_device *rdev,
 		nla_nest_end(msg, nla_wmm);
 	}
 
+	if (event->carrier_up_count &&
+	    nla_put_u32(msg, NL80211_ATTR_CARRIER_UP_COUNT,
+			event->carrier_up_count))
+		goto nla_put_failure;
+
 	genlmsg_end(msg, hdr);
 
 	genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
@@ -17824,6 +17837,7 @@ void nl80211_send_rx_assoc(struct cfg80211_registered_device *rdev,
 		.uapsd_queues = data->uapsd_queues,
 		.req_ies = data->req_ies,
 		.req_ies_len = data->req_ies_len,
+		.carrier_up_count = atomic_read(&netdev->carrier_up_count),
 	};
 
 	nl80211_send_mlme_event(rdev, netdev, &event, GFP_KERNEL);
@@ -18307,32 +18321,13 @@ void nl80211_send_ibss_bssid(struct cfg80211_registered_device *rdev,
 			     struct net_device *netdev, const u8 *bssid,
 			     gfp_t gfp)
 {
-	struct sk_buff *msg;
-	void *hdr;
+	struct nl80211_mlme_event event = {
+		.cmd = NL80211_CMD_JOIN_IBSS,
+		.mac_addr = bssid,
+		.carrier_up_count = atomic_read(&netdev->carrier_up_count),
+	};
 
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
-	if (!msg)
-		return;
-
-	hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_JOIN_IBSS);
-	if (!hdr) {
-		nlmsg_free(msg);
-		return;
-	}
-
-	if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) ||
-	    nla_put_u32(msg, NL80211_ATTR_IFINDEX, netdev->ifindex) ||
-	    nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, bssid))
-		goto nla_put_failure;
-
-	genlmsg_end(msg, hdr);
-
-	genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
-				NL80211_MCGRP_MLME, gfp);
-	return;
-
- nla_put_failure:
-	nlmsg_free(msg);
+	nl80211_send_mlme_event(rdev, netdev, &event, gfp);
 }
 
 void cfg80211_notify_new_peer_candidate(struct net_device *dev, const u8 *addr,
-- 
2.43.0

Powered by blists - more mailing lists