Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Denis Kenzior <denkenz@gmail.com>
To:     linux-wireless@vger.kernel.org, johannes@sipsolutions.net
Cc:     Denis Kenzior <denkenz@gmail.com>
Subject: [RFCv3 2/3] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
Date:   Fri,  6 Sep 2019 10:43:02 -0500
Message-Id: <20190906154303.9303-2-denkenz@gmail.com>
In-Reply-To: <20190906154303.9303-1-denkenz@gmail.com>
References: <20190906154303.9303-1-denkenz@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

For historical reasons, NEW_WIPHY messages generated by dumps or
GET_WIPHY commands were limited to 4096 bytes.  This was due to the
fact that the kernel only allocated 4k buffers prior to commit
9063e21fb026 ("netlink: autosize skb lengthes").  Once the sizes of
NEW_WIPHY messages exceeded these sizes, split dumps were introduced.

Any new, non-legacy data was added only to messages using split-dumps
(including filtered dumps).

However, split-dumping has quite a significant overhead.  On cards
tested, split dumps generated message sizes 1.7-1.8x compared to
non-split dumps, while still comfortably fitting into an 8k buffer.  The
kernel now expects userspace to provide 16k buffers by default, and 32k
buffers are possible.

The kernel netlink layer is now much smarter and utilizes certain
heuristics for figuring out what buffer sizes userspace provides, so it
can allocate optimally sized buffers for netlink messages accordingly.
This commit changes the split logic so that messages are packed more
compactly into the (potentially) larger buffers provided by userspace.

If large-enough buffers are provided, then split dumps will generate a
single netlink NEW_WIPHY message for each wiphy being dumped, removing
any overhead.

Signed-off-by: Denis Kenzior <denkenz@gmail.com>
---
 net/wireless/nl80211.c | 222 +++++++++++++++++++++--------------------
 1 file changed, 112 insertions(+), 110 deletions(-)

Changes since last version:

- Patch completely rewritten based on Johannes' feedback.  We now try to
  pack as many attributes as can fit into the current message.  If the
  application uses large enough buffers (typically 8k is sufficient as
  of the time of this writing), then no splitting is even required.
- Rewrote the commit description based on Johannes' git history
  findings.  E.g. the kernel was at fault for the 4096 byte buffer size
  limits and not userspace.
- Patch 3 was dropped as it was no longer required

Other thoughts:

- I tested the split dump with 3k, 4k and 8k userspace buffers and
  things seem to work as expected.
- The code in case '3' is quite complex, but it does try to support a
  message running out of room in the middle of a channel dump and
  restarting from where it left off in the next split message.  Perhaps
  this can be simplified, but it seems this capability is useful.
  Please take extra care when reviewing this.
- I dropped the split and restart logic in case 13 as no current driver
  besides iwlwifi seems to support the attributes here, and the
  attributes appear to be quite small anyway.

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ff6200fcd492..03421f66eea3 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1854,8 +1854,8 @@ static int nl80211_send_pmsr_capa(struct cfg80211_registered_device *rdev,
 struct nl80211_dump_wiphy_state {
 	s64 filter_wiphy;
 	long start;
-	long split_start, band_start, chan_start, capa_start;
-	bool split;
+	long split_start, band_start, chan_start;
+	bool legacy;
 };
 
 static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
@@ -1867,8 +1867,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 	struct nlattr *nl_bands, *nl_band;
 	struct nlattr *nl_freqs, *nl_freq;
 	struct nlattr *nl_cmds;
-	enum nl80211_band band;
 	struct ieee80211_channel *chan;
+	void *last_good_pos = 0;
+	void *last_channel_pos;
 	int i;
 	const struct ieee80211_txrx_stypes *mgmt_stypes =
 				rdev->wiphy.mgmt_stypes;
@@ -1939,9 +1940,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		if ((rdev->wiphy.flags & WIPHY_FLAG_TDLS_EXTERNAL_SETUP) &&
 		    nla_put_flag(msg, NL80211_ATTR_TDLS_EXTERNAL_SETUP))
 			goto nla_put_failure;
+
+		last_good_pos = nlmsg_get_pos(msg);
 		state->split_start++;
-		if (state->split)
-			break;
 		/* fall through */
 	case 1:
 		if (nla_put(msg, NL80211_ATTR_CIPHER_SUITES,
@@ -1986,17 +1987,16 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 			}
 		}
 
+		last_good_pos = nlmsg_get_pos(msg);
 		state->split_start++;
-		if (state->split)
-			break;
 		/* fall through */
 	case 2:
 		if (nl80211_put_iftypes(msg, NL80211_ATTR_SUPPORTED_IFTYPES,
 					rdev->wiphy.interface_modes))
 				goto nla_put_failure;
+
+		last_good_pos = nlmsg_get_pos(msg);
 		state->split_start++;
-		if (state->split)
-			break;
 		/* fall through */
 	case 3:
 		nl_bands = nla_nest_start_noflag(msg,
@@ -2004,81 +2004,78 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		if (!nl_bands)
 			goto nla_put_failure;
 
-		for (band = state->band_start;
-		     band < NUM_NL80211_BANDS; band++) {
+		/* Position in the buffer if we added a set of channel info */
+		last_channel_pos = 0;
+
+		while (state->band_start < NUM_NL80211_BANDS) {
 			struct ieee80211_supported_band *sband;
 
-			sband = rdev->wiphy.bands[band];
+			sband = rdev->wiphy.bands[state->band_start];
 
-			if (!sband)
+			if (!sband) {
+				state->band_start++;
 				continue;
+			}
 
-			nl_band = nla_nest_start_noflag(msg, band);
+			nl_band = nla_nest_start_noflag(msg, state->band_start);
 			if (!nl_band)
-				goto nla_put_failure;
+				goto band_put_failure;
 
-			switch (state->chan_start) {
-			case 0:
-				if (nl80211_send_band_rateinfo(msg, sband))
-					goto nla_put_failure;
-				state->chan_start++;
-				if (state->split)
-					break;
-				/* fall through */
-			default:
-				/* add frequencies */
-				nl_freqs = nla_nest_start_noflag(msg,
-								 NL80211_BAND_ATTR_FREQS);
-				if (!nl_freqs)
-					goto nla_put_failure;
+			if (!state->chan_start &&
+			    nl80211_send_band_rateinfo(msg, sband))
+				goto band_put_failure;
+
+			nl_freqs = nla_nest_start_noflag(msg,
+							 NL80211_BAND_ATTR_FREQS);
+			if (!nl_freqs)
+				goto band_put_failure;
 
-				for (i = state->chan_start - 1;
-				     i < sband->n_channels;
-				     i++) {
-					nl_freq = nla_nest_start_noflag(msg,
-									i);
-					if (!nl_freq)
-						goto nla_put_failure;
+			while (state->chan_start < sband->n_channels) {
+				nl_freq = nla_nest_start_noflag(msg,
+								state->chan_start);
+				if (!nl_freq)
+					goto chan_put_failure;
 
-					chan = &sband->channels[i];
+				chan = &sband->channels[state->chan_start];
 
-					if (nl80211_msg_put_channel(
-							msg, &rdev->wiphy, chan,
-							state->split))
-						goto nla_put_failure;
+				if (nl80211_msg_put_channel(msg, &rdev->wiphy,
+							    chan,
+							    !state->legacy))
+					goto chan_put_failure;
 
-					nla_nest_end(msg, nl_freq);
-					if (state->split)
-						break;
-				}
-				if (i < sband->n_channels)
-					state->chan_start = i + 2;
-				else
-					state->chan_start = 0;
-				nla_nest_end(msg, nl_freqs);
+				nla_nest_end(msg, nl_freq);
+				state->chan_start++;
+				last_channel_pos = nlmsg_get_pos(msg);
 			}
 
+chan_put_failure:
+			if (!last_channel_pos)
+				goto nla_put_failure;
+
+			nlmsg_trim(msg, last_channel_pos);
+			nla_nest_end(msg, nl_freqs);
 			nla_nest_end(msg, nl_band);
 
-			if (state->split) {
-				/* start again here */
-				if (state->chan_start)
-					band--;
+			if (state->chan_start < sband->n_channels)
 				break;
-			}
+
+			state->chan_start = 0;
+			state->band_start++;
 		}
-		nla_nest_end(msg, nl_bands);
 
-		if (band < NUM_NL80211_BANDS)
-			state->band_start = band + 1;
-		else
-			state->band_start = 0;
+band_put_failure:
+		if (!last_channel_pos)
+			goto nla_put_failure;
+
+		nla_nest_end(msg, nl_bands);
 
-		/* if bands & channels are done, continue outside */
-		if (state->band_start == 0 && state->chan_start == 0)
-			state->split_start++;
-		if (state->split)
+		if (state->band_start < NUM_NL80211_BANDS)
 			break;
+
+		state->band_start = 0;
+
+		last_good_pos = nlmsg_get_pos(msg);
+		state->split_start++;
 		/* fall through */
 	case 4:
 		nl_cmds = nla_nest_start_noflag(msg,
@@ -2089,7 +2086,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		i = nl80211_add_commands_unsplit(rdev, msg);
 		if (i < 0)
 			goto nla_put_failure;
-		if (state->split) {
+		if (!state->legacy) {
 			CMD(crit_proto_start, CRIT_PROTOCOL_START);
 			CMD(crit_proto_stop, CRIT_PROTOCOL_STOP);
 			if (rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH)
@@ -2105,9 +2102,8 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 #undef CMD
 
 		nla_nest_end(msg, nl_cmds);
+		last_good_pos = nlmsg_get_pos(msg);
 		state->split_start++;
-		if (state->split)
-			break;
 		/* fall through */
 	case 5:
 		if (rdev->ops->remain_on_channel &&
@@ -2123,20 +2119,17 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 
 		if (nl80211_send_mgmt_stypes(msg, mgmt_stypes))
 			goto nla_put_failure;
+
+		last_good_pos = nlmsg_get_pos(msg);
 		state->split_start++;
-		if (state->split)
-			break;
 		/* fall through */
 	case 6:
 #ifdef CONFIG_PM
-		if (nl80211_send_wowlan(msg, rdev, state->split))
+		if (nl80211_send_wowlan(msg, rdev, !state->legacy))
 			goto nla_put_failure;
-		state->split_start++;
-		if (state->split)
-			break;
-#else
-		state->split_start++;
 #endif
+		last_good_pos = nlmsg_get_pos(msg);
+		state->split_start++;
 		/* fall through */
 	case 7:
 		if (nl80211_put_iftypes(msg, NL80211_ATTR_SOFTWARE_IFTYPES,
@@ -2144,12 +2137,11 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 			goto nla_put_failure;
 
 		if (nl80211_put_iface_combinations(&rdev->wiphy, msg,
-						   state->split))
+						   !state->legacy))
 			goto nla_put_failure;
 
+		last_good_pos = nlmsg_get_pos(msg);
 		state->split_start++;
-		if (state->split)
-			break;
 		/* fall through */
 	case 8:
 		if ((rdev->wiphy.flags & WIPHY_FLAG_HAVE_AP_SME) &&
@@ -2160,10 +2152,10 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		features = rdev->wiphy.features;
 		/*
 		 * We can only add the per-channel limit information if the
-		 * dump is split, otherwise it makes it too big. Therefore
-		 * only advertise it in that case.
+		 * dump is for a non-legacy message, otherwise it makes it too
+		 * big. Therefore only advertise it in that case.
 		 */
-		if (state->split)
+		if (!state->legacy)
 			features |= NL80211_FEATURE_ADVERTISE_CHAN_LIMITS;
 		if (nla_put_u32(msg, NL80211_ATTR_FEATURE_FLAGS, features))
 			goto nla_put_failure;
@@ -2182,19 +2174,19 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 
 		/*
 		 * Any information below this point is only available to
-		 * applications that can deal with it being split. This
-		 * helps ensure that newly added capabilities don't break
-		 * older tools by overrunning their buffers.
-		 *
-		 * We still increment split_start so that in the split
-		 * case we'll continue with more data in the next round,
-		 * but break unconditionally so unsplit data stops here.
+		 * applications that are not legacy, e.g. ones that requested
+		 * a split-dump or using large buffers.  This helps ensure
+		 * that newly added capabilities don't break older tools by
+		 * overrunning their buffers.
 		 */
-		state->split_start++;
-
-		if (!state->split)
+		if (state->legacy) {
 			state->split_start = 0;
-		break;
+			break;
+		}
+
+		last_good_pos = nlmsg_get_pos(msg);
+		state->split_start++;
+		/* Fall through */
 	case 9:
 		if (rdev->wiphy.extended_capabilities &&
 		    (nla_put(msg, NL80211_ATTR_EXT_CAPA,
@@ -2235,8 +2227,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 			nla_nest_end(msg, attr);
 		}
 
+		last_good_pos = nlmsg_get_pos(msg);
 		state->split_start++;
-		break;
+		/* Fall through */
 	case 10:
 		if (nl80211_send_coalesce(msg, rdev))
 			goto nla_put_failure;
@@ -2251,8 +2244,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 				rdev->wiphy.max_ap_assoc_sta))
 			goto nla_put_failure;
 
+		last_good_pos = nlmsg_get_pos(msg);
 		state->split_start++;
-		break;
+		/* Fall through */
 	case 11:
 		if (rdev->wiphy.n_vendor_commands) {
 			const struct nl80211_vendor_cmd_info *info;
@@ -2287,8 +2281,10 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 			}
 			nla_nest_end(msg, nested);
 		}
+
+		last_good_pos = nlmsg_get_pos(msg);
 		state->split_start++;
-		break;
+		/* Fall through */
 	case 12:
 		if (rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH &&
 		    nla_put_u8(msg, NL80211_ATTR_MAX_CSA_COUNTERS,
@@ -2329,8 +2325,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 			nla_nest_end(msg, nested);
 		}
 
+		last_good_pos = nlmsg_get_pos(msg);
 		state->split_start++;
-		break;
+		/* Fall through */
 	case 13:
 		if (rdev->wiphy.num_iftype_ext_capab &&
 		    rdev->wiphy.iftype_ext_capab) {
@@ -2341,8 +2338,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 			if (!nested)
 				goto nla_put_failure;
 
-			for (i = state->capa_start;
-			     i < rdev->wiphy.num_iftype_ext_capab; i++) {
+			for (i = 0; i < rdev->wiphy.num_iftype_ext_capab; i++) {
 				const struct wiphy_iftype_ext_capab *capab;
 
 				capab = &rdev->wiphy.iftype_ext_capab[i];
@@ -2361,14 +2357,8 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 					goto nla_put_failure;
 
 				nla_nest_end(msg, nested_ext_capab);
-				if (state->split)
-					break;
 			}
 			nla_nest_end(msg, nested);
-			if (i < rdev->wiphy.num_iftype_ext_capab) {
-				state->capa_start = i + 1;
-				break;
-			}
 		}
 
 		if (nla_put_u32(msg, NL80211_ATTR_BANDS,
@@ -2397,14 +2387,16 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 				goto nla_put_failure;
 		}
 
+		last_good_pos = nlmsg_get_pos(msg);
 		state->split_start++;
-		break;
+		/* Fall through */
 	case 14:
 		if (nl80211_send_pmsr_capa(rdev, msg))
 			goto nla_put_failure;
 
+		last_good_pos = nlmsg_get_pos(msg);
 		state->split_start++;
-		break;
+		/* Fall through */
 	case 15:
 		if (rdev->wiphy.akm_suites &&
 		    nla_put(msg, NL80211_ATTR_AKM_SUITES,
@@ -2421,8 +2413,14 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 	return 0;
 
  nla_put_failure:
-	genlmsg_cancel(msg, hdr);
-	return -EMSGSIZE;
+	if ((state->legacy && state->split_start < 9) ||
+	    !last_good_pos) {
+		genlmsg_cancel(msg, hdr);
+		return -EMSGSIZE;
+	}
+
+	nlmsg_trim(msg, last_good_pos);
+	goto finish;
 }
 
 static int nl80211_dump_wiphy_parse(struct sk_buff *skb,
@@ -2445,7 +2443,7 @@ static int nl80211_dump_wiphy_parse(struct sk_buff *skb,
 		goto out;
 	}
 
-	state->split = tb[NL80211_ATTR_SPLIT_WIPHY_DUMP];
+	state->legacy = !tb[NL80211_ATTR_SPLIT_WIPHY_DUMP];
 	if (tb[NL80211_ATTR_WIPHY])
 		state->filter_wiphy = nla_get_u32(tb[NL80211_ATTR_WIPHY]);
 	if (tb[NL80211_ATTR_WDEV])
@@ -2526,7 +2524,7 @@ static int nl80211_dump_wiphy(struct sk_buff *skb, struct netlink_callback *cb)
 				 * We can then retry with the larger buffer.
 				 */
 				if ((ret == -ENOBUFS || ret == -EMSGSIZE) &&
-				    !skb->len && !state->split &&
+				    !skb->len && state->legacy &&
 				    cb->min_dump_alloc < 4096) {
 					cb->min_dump_alloc = 4096;
 					state->split_start = 0;
@@ -2558,6 +2556,8 @@ static int nl80211_get_wiphy(struct sk_buff *skb, struct genl_info *info)
 	struct cfg80211_registered_device *rdev = info->user_ptr[0];
 	struct nl80211_dump_wiphy_state state = {};
 
+	state.legacy = true;
+
 	msg = nlmsg_new(4096, GFP_KERNEL);
 	if (!msg)
 		return -ENOMEM;
@@ -14737,6 +14737,8 @@ void nl80211_notify_wiphy(struct cfg80211_registered_device *rdev,
 	WARN_ON(cmd != NL80211_CMD_NEW_WIPHY &&
 		cmd != NL80211_CMD_DEL_WIPHY);
 
+	state.legacy = true;
+
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
 		return;
-- 
2.19.2