If a (legacy) client requested a wiphy dump but did not provide the
NL80211_ATTR_SPLIT_WIPHY_DUMP attribute, the dump was supposed to be
composed of purely non-split NEW_WIPHY messages, with 1 wiphy per
message. At least this was the intent after commit:
3713b4e364ef ("nl80211: allow splitting wiphy information in dumps")
However, in reality the non-split dumps were broken very shortly after.
Perhaps around commit:
fe1abafd942f ("nl80211: re-add channel width and extended capa advertising")
The reason for the bug is a missing setting of split_start to 0 in the
case of a non-split dump.
Here is a sample non-split dump performed on kernel 4.19, some parts
were cut for brevity:
< Request: Get Wiphy (0x01) len 0 [ack,0x300]
> Result: New Wiphy (0x03) len 3496 [multi]
Wiphy: 0 (0x00000000)
Wiphy Name: phy0
Generation: 1 (0x00000001)
<snip>
> Result: New Wiphy (0x03) len 68 [multi]
Wiphy: 0 (0x00000000)
Wiphy Name: phy0
Generation: 1 (0x00000001)
Extended Capabilities: len 8
Capability: bit 2: Extended channel switching
Capability: bit 62: Opmode Notification
Extended Capabilities Mask: len 8
04 00 00 00 00 00 00 40 .......@
VHT Capability Mask: len 12
f0 1f 80 33 ff ff 00 00 ff ff 00 00 ...3........
> Result: New Wiphy (0x03) len 28 [multi]
Wiphy: 0 (0x00000000)
Wiphy Name: phy0
Generation: 1 (0x00000001)
> Result: New Wiphy (0x03) len 28 [multi]
Wiphy: 0 (0x00000000)
Wiphy Name: phy0
Generation: 1 (0x00000001)
> Result: New Wiphy (0x03) len 52 [multi]
Wiphy: 0 (0x00000000)
Wiphy Name: phy0
Generation: 1 (0x00000001)
Max CSA Counters: len 1
02 .
Scheduled Scan Maximum Requests: len 4
01 00 00 00 ....
Extended Features: len 4
02 02 00 04 ....
> Result: New Wiphy (0x03) len 36 [multi]
Wiphy: 0 (0x00000000)
Wiphy Name: phy0
Generation: 1 (0x00000001)
Reserved: len 4
00 00 00 00 ....
> Complete: Get Wiphy (0x01) len 4 [multi]
Status: 0
Signed-off-by: Denis Kenzior <[email protected]>
---
net/wireless/nl80211.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 3e30e18d1d89..ff6200fcd492 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2191,6 +2191,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
* but break unconditionally so unsplit data stops here.
*/
state->split_start++;
+
+ if (!state->split)
+ state->split_start = 0;
break;
case 9:
if (rdev->wiphy.extended_capabilities &&
--
2.19.2
Send large NEW_WIPHY events on a new multicast group so that clients
that can accept larger messages do not need to round-trip to the kernel
and perform extra filtered wiphy dumps.
A new multicast group is introduced and the large message is sent before
the legacy message. This way clients that listen on both multicast
groups can ignore duplicate legacy messages if needed.
Signed-off-by: Denis Kenzior <[email protected]>
---
include/uapi/linux/nl80211.h | 31 +++++++++++++++++++++++++++++++
net/wireless/nl80211.c | 34 ++++++++++++++++++++++++++++++++--
2 files changed, 63 insertions(+), 2 deletions(-)
Changes in this version:
- Updated the docs based on Johannes' feedback
- Added WARN_ON in case the large message building fails (e.g. our
buffer size needs to be increased)
- Minor style fixes based on Johannes' feedback
- Added a missing skb_get to take an extra reference when sending
NEW/DEL INTERFACE events.
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index beee59c831a7..7a125cb4d9d9 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -50,6 +50,7 @@
#define NL80211_MULTICAST_GROUP_MLME "mlme"
#define NL80211_MULTICAST_GROUP_VENDOR "vendor"
#define NL80211_MULTICAST_GROUP_NAN "nan"
+#define NL80211_MULTICAST_GROUP_CONFIG2 "config2"
#define NL80211_MULTICAST_GROUP_TESTMODE "testmode"
#define NL80211_EDMG_BW_CONFIG_MIN 4
@@ -267,8 +268,30 @@
* @NL80211_CMD_NEW_WIPHY: Newly created wiphy, response to get request
* or rename notification. Has attributes %NL80211_ATTR_WIPHY and
* %NL80211_ATTR_WIPHY_NAME.
+ *
+ * Note that when %NL80211_CMD_NEW_WIPHY is being sent as an event, it
+ * will be multicast on two groups: "config" and "config2". The messages
+ * on the two multicast groups will be different. On "config" multicast
+ * group, %NL80211_CMD_NEW_WIPHY messages will be 'reduced' size and will
+ * only contain legacy information. This is due to historical kernel
+ * behavior that limited such messages to 4096 bytes. The "config2"
+ * multicast group was introduced to support applications that can
+ * allocate larger buffers and can thus accept new wiphy events with
+ * the full set of information included. Messages on the "config2"
+ * multicast group are sent before the "config" multicast group.
+ *
+ * There are no limits (outside of netlink protocol limits) on
+ * message sizes that can be sent over the "config2" multicast group. It
+ * is assumed that applications utilizing "config2" multicast group
+ * utilize buffers that are inherently large enough or can utilize
+ * MSG_PEEK/MSG_TRUNC in the netlink transport in order to allocate big
+ * enough buffers.
* @NL80211_CMD_DEL_WIPHY: Wiphy deleted. Has attributes
* %NL80211_ATTR_WIPHY and %NL80211_ATTR_WIPHY_NAME.
+ * Note that when %NL80211_CMD_DEL_WIPHY is being sent as an event, it
+ * will be multicast on two groups: "config" and "config2". Messages on
+ * the "config2" multicast group are sent before the "config" multicast
+ * group.
*
* @NL80211_CMD_GET_INTERFACE: Request an interface's configuration;
* either a dump request for all interfaces or a specific get with a
@@ -281,10 +304,18 @@
* be sent from userspace to request creation of a new virtual interface,
* then requires attributes %NL80211_ATTR_WIPHY, %NL80211_ATTR_IFTYPE and
* %NL80211_ATTR_IFNAME.
+ * Note that when %NL80211_CMD_NEW_INTERFACE is being sent as an event, it
+ * will be multicast on two groups: "config" and "config2". Messages on
+ * the "config2" multicast group are sent before the "config" multicast
+ * group.
* @NL80211_CMD_DEL_INTERFACE: Virtual interface was deleted, has attributes
* %NL80211_ATTR_IFINDEX and %NL80211_ATTR_WIPHY. Can also be sent from
* userspace to request deletion of a virtual interface, then requires
* attribute %NL80211_ATTR_IFINDEX.
+ * Note that when %NL80211_CMD_DEL_INTERFACE is being sent as an event, it
+ * will be multicast on two groups: "config" and "config2". Messages on
+ * the "config2" multicast group are sent before the "config" multicast
+ * group.
*
* @NL80211_CMD_GET_KEY: Get sequence counter information for a key specified
* by %NL80211_ATTR_KEY_IDX and/or %NL80211_ATTR_MAC.
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 03421f66eea3..68f496c0c0a4 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -46,6 +46,7 @@ enum nl80211_multicast_groups {
NL80211_MCGRP_MLME,
NL80211_MCGRP_VENDOR,
NL80211_MCGRP_NAN,
+ NL80211_MCGRP_CONFIG2,
NL80211_MCGRP_TESTMODE /* keep last - ifdef! */
};
@@ -56,6 +57,7 @@ static const struct genl_multicast_group nl80211_mcgrps[] = {
[NL80211_MCGRP_MLME] = { .name = NL80211_MULTICAST_GROUP_MLME },
[NL80211_MCGRP_VENDOR] = { .name = NL80211_MULTICAST_GROUP_VENDOR },
[NL80211_MCGRP_NAN] = { .name = NL80211_MULTICAST_GROUP_NAN },
+ [NL80211_MCGRP_CONFIG2] = { .name = NL80211_MULTICAST_GROUP_CONFIG2 },
#ifdef CONFIG_NL80211_TESTMODE
[NL80211_MCGRP_TESTMODE] = { .name = NL80211_MULTICAST_GROUP_TESTMODE }
#endif
@@ -1856,6 +1858,7 @@ struct nl80211_dump_wiphy_state {
long start;
long split_start, band_start, chan_start;
bool legacy;
+ bool large_message;
};
static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
@@ -2414,7 +2417,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
nla_put_failure:
if ((state->legacy && state->split_start < 9) ||
- !last_good_pos) {
+ !last_good_pos || state->large_message) {
genlmsg_cancel(msg, hdr);
return -EMSGSIZE;
}
@@ -14732,14 +14735,37 @@ void nl80211_notify_wiphy(struct cfg80211_registered_device *rdev,
enum nl80211_commands cmd)
{
struct sk_buff *msg;
+ size_t alloc_size;
struct nl80211_dump_wiphy_state state = {};
WARN_ON(cmd != NL80211_CMD_NEW_WIPHY &&
cmd != NL80211_CMD_DEL_WIPHY);
+ if (cmd == NL80211_CMD_NEW_WIPHY) {
+ state.large_message = true;
+ alloc_size = 8192UL;
+ } else {
+ alloc_size = NLMSG_DEFAULT_SIZE;
+ }
+
+ msg = nlmsg_new(alloc_size, GFP_KERNEL);
+ if (!msg)
+ goto legacy;
+
+ if (WARN_ON(nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0)) {
+ nlmsg_free(msg);
+ goto legacy;
+ }
+
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_CONFIG2, GFP_KERNEL);
+
+legacy:
+ state.large_message = false;
state.legacy = true;
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ alloc_size = NLMSG_DEFAULT_SIZE;
+ msg = nlmsg_new(alloc_size, GFP_KERNEL);
if (!msg)
return;
@@ -14767,6 +14793,10 @@ void nl80211_notify_iface(struct cfg80211_registered_device *rdev,
return;
}
+ /* We will be sending the same message twice, grab an extra ref */
+ msg = skb_get(msg);
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_CONFIG2, GFP_KERNEL);
genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
NL80211_MCGRP_CONFIG, GFP_KERNEL);
}
--
2.19.2
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 <[email protected]>
---
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
Hi,
The first patch looks good, couple of nits/comments on this one below.
On Fri, 2019-09-06 at 10:43 -0500, Denis Kenzior wrote:
> 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.
Actually, userspace prior to around the same time *also* only used 4k
buffers (old libnl), and so even with that kernel we still could
possibly have to deal with userspace that had 4k messages only ... but
we could have solved that part trivially instead of adding code to split
it, just the kernel part was still in the way then.
Anyway, I can reword this per my understanding (but will have to reread
all my messages I guess).
> findings. E.g. the kernel was at fault for the 4096 byte buffer size
> limits and not userspace.
Nit: I think most of the time when you write "e.g." ("exempli gratia",
"for example") you really mean "i.e." ("id est", "which is").
> - 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.
Is it useful? You say it basically all fits today, and that means the
channels will either fit into a single message or not ... Then again, if
we add a lot of channels or a lot more data to each channel. Hmm. OK, I
guess better if we do have it.
> + void *last_good_pos = 0;
Use NULL.
> + last_good_pos = nlmsg_get_pos(msg);
> state->split_start++;
Maybe we're better off having a local macro for these two lines? That
way, we don't risk updating one without the other, which would be
confusing.
> @@ -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;
NULL
> [snip]
> +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;
Thinking out loud, maybe we could simplify this by just having a small
"stack" of nested attributes to end?
I mean, essentially, you have here similar code to the nla_put_failure
label, in that it finishes and sends out the message, except here you
have to end a bunch of nested attributes.
What if we did something like
#define dump_nest_start(msg, attr) ({ \
struct nlattr r = nla_nest_start_noflag(msg, attr); \
BUG_ON(nest_stack_depth >= ARRAY_SIZE(nest_stack); \
nest_stack[nest_stack_depth++] = r; \
r; \
})
#define dump_nest_end(msg, r) do { \
BUG_ON(nest_stack_depth > 0); \
nest_stack_depth--; \
BUG_ON(nest_stack[nest_stack_depth] == r); \
nla_nest_end(msg, r); \
} while (0)
or something like that (we probably don't want to use
nla_nest_start_noflag() for future attributes, etc. but anyway).
Then we could unwind any nesting at the end in the common code at the
nla_put_failure label very easily, I think?
> + * applications that are not legacy, e.g. ones that requested
i.e. :)
johannes
On Fri, 2019-09-06 at 10:43 -0500, Denis Kenzior wrote:
>
> + * There are no limits (outside of netlink protocol limits) on
> + * message sizes that can be sent over the "config2" multicast group. It
> + * is assumed that applications utilizing "config2" multicast group
> + * utilize buffers that are inherently large enough or can utilize
> + * MSG_PEEK/MSG_TRUNC in the netlink transport in order to allocate big
> + * enough buffers.
I'm not sure I see how the applications could do buffers that are
"inherently" large enough, there's no practical message size limit, is
there (32-bits for the size).
I'd argue this should just say that applications should use large
buffers and still use MSG_PEEK/handle MSG_TRUNC, but I can also edit it
later.
> + msg = nlmsg_new(alloc_size, GFP_KERNEL);
> + if (!msg)
> + goto legacy;
> +
> + if (WARN_ON(nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0)) {
> + nlmsg_free(msg);
> + goto legacy;
> + }
> +
> + genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
> + NL80211_MCGRP_CONFIG2, GFP_KERNEL);
> +
> +legacy:
nit: just use "else" instead of the goto?
johannes
hi Johannes,
On 9/11/19 4:47 AM, Johannes Berg wrote:
> On Fri, 2019-09-06 at 10:43 -0500, Denis Kenzior wrote:
>>
>> + * There are no limits (outside of netlink protocol limits) on
>> + * message sizes that can be sent over the "config2" multicast group. It
>> + * is assumed that applications utilizing "config2" multicast group
>> + * utilize buffers that are inherently large enough or can utilize
>> + * MSG_PEEK/MSG_TRUNC in the netlink transport in order to allocate big
>> + * enough buffers.
>
> I'm not sure I see how the applications could do buffers that are
> "inherently" large enough, there's no practical message size limit, is
> there (32-bits for the size).
The kernel caps this to 32k right now if I read the code correctly. But
fair point.
>
> I'd argue this should just say that applications should use large
> buffers and still use MSG_PEEK/handle MSG_TRUNC, but I can also edit it
> later.
>
>> + msg = nlmsg_new(alloc_size, GFP_KERNEL);
>> + if (!msg)
>> + goto legacy;
>> +
>> + if (WARN_ON(nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0)) {
>> + nlmsg_free(msg);
>> + goto legacy;
>> + }
>> +
>> + genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
>> + NL80211_MCGRP_CONFIG2, GFP_KERNEL);
>> +
>> +legacy:
>
> nit: just use "else" instead of the goto?
I'm not sure I understand? We want to send both messages here...
Regards,
-Denis
Hi Johannes,
On 9/11/19 4:44 AM, Johannes Berg wrote:
> Hi,
>
> The first patch looks good, couple of nits/comments on this one below.
>
> On Fri, 2019-09-06 at 10:43 -0500, Denis Kenzior wrote:
>> 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.
>
> Actually, userspace prior to around the same time *also* only used 4k
> buffers (old libnl), and so even with that kernel we still could
> possibly have to deal with userspace that had 4k messages only ... but
> we could have solved that part trivially instead of adding code to split
> it, just the kernel part was still in the way then.
>
> Anyway, I can reword this per my understanding (but will have to reread
> all my messages I guess).
>
Sure
>> - 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.
>
> Is it useful? You say it basically all fits today, and that means the
> channels will either fit into a single message or not ... Then again, if
> we add a lot of channels or a lot more data to each channel. Hmm. OK, I
> guess better if we do have it.
>
For my dual band cards the channel dump all fits into a ~1k attribute if
I recall correctly. So there may not really be a need for this. Or at
the very least we could keep things simple(r) and only split at the band
level, not at the individual channel level.
All the cards I tried would split well after case 9 with 4096 byte
buffers anyway. The channel dump is quite early in the message and it
would really need to become bloated for this code path to be triggered...
>
>> + void *last_good_pos = 0;
>
> Use NULL.
Will fix
>
>> + last_good_pos = nlmsg_get_pos(msg);
>> state->split_start++;
>
> Maybe we're better off having a local macro for these two lines? That
> way, we don't risk updating one without the other, which would be
> confusing.
Yep, will do that.
>
>> @@ -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;
>
> NULL
Will fix
>
>> [snip]
>
>> +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;
>
> Thinking out loud, maybe we could simplify this by just having a small
> "stack" of nested attributes to end?
>
> I mean, essentially, you have here similar code to the nla_put_failure
> label, in that it finishes and sends out the message, except here you
> have to end a bunch of nested attributes.
>
> What if we did something like
>
> #define dump_nest_start(msg, attr) ({ \
> struct nlattr r = nla_nest_start_noflag(msg, attr); \
> BUG_ON(nest_stack_depth >= ARRAY_SIZE(nest_stack); \
> nest_stack[nest_stack_depth++] = r; \
> r; \
> })
>
> #define dump_nest_end(msg, r) do { \
> BUG_ON(nest_stack_depth > 0); \
> nest_stack_depth--; \
> BUG_ON(nest_stack[nest_stack_depth] == r); \
> nla_nest_end(msg, r); \
> } while (0)
>
>
> or something like that (we probably don't want to use
> nla_nest_start_noflag() for future attributes, etc. but anyway).
>
> Then we could unwind any nesting at the end in the common code at the
> nla_put_failure label very easily, I think?
I see where you're going with this, I think I do anyway...
The current logic uses last_channel_pos for some of the trickery in
addition to last_good_pos. So nla_put_failure would have to be made
aware of that. Perhaps we can store last_good_pos in the stack, but the
split mechanism only allows the splits to be done at certain points...
I think the above could be doable, but the code would be even more complex.
Right now only the channel dump uses this (and I'm still not fully
convinced we should go to all the trouble), so one argument would be not
to introduce something this generic until another user of it manifests
itself?
Regards,
-Denis
On Wed, 2019-09-11 at 07:20 -0500, Denis Kenzior wrote:
> > I'm not sure I see how the applications could do buffers that are
> > "inherently" large enough, there's no practical message size limit, is
> > there (32-bits for the size).
>
> The kernel caps this to 32k right now if I read the code correctly. But
> fair point.
The kernel caps this for dumps only, no? We can allocate here ourselves
for multicasting a message as large as we like I think.
> > > + if (WARN_ON(nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0)) {
> > > + nlmsg_free(msg);
> > > + goto legacy;
> > > + }
> > > +
> > > + genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
> > > + NL80211_MCGRP_CONFIG2, GFP_KERNEL);
> > > +
> > > +legacy:
> >
> > nit: just use "else" instead of the goto?
>
> I'm not sure I understand? We want to send both messages here...
It's equivalent to:
-----
if (WARN_ON(nl80211_send_wiphy(...) < 0)
nlmsg_free(msg);
else
genlmsg_multicast_netns(...);
... code for legacy ...
-----
no?
johannes
Hi Johannes,
On 9/11/19 10:12 AM, Johannes Berg wrote:
> On Wed, 2019-09-11 at 07:20 -0500, Denis Kenzior wrote:
>
>>> I'm not sure I see how the applications could do buffers that are
>>> "inherently" large enough, there's no practical message size limit, is
>>> there (32-bits for the size).
>>
>> The kernel caps this to 32k right now if I read the code correctly. But
>> fair point.
>
> The kernel caps this for dumps only, no? We can allocate here ourselves
> for multicasting a message as large as we like I think.
>
Right, but it is set for only 8k at the moment. Anyway, I will take
care of this.
>>>> + if (WARN_ON(nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0)) {
>>>> + nlmsg_free(msg);
>>>> + goto legacy;
>>>> + }
>>>> +
>>>> + genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
>>>> + NL80211_MCGRP_CONFIG2, GFP_KERNEL);
>>>> +
>>>> +legacy:
>>>
>>> nit: just use "else" instead of the goto?
>>
>> I'm not sure I understand? We want to send both messages here...
>
> It's equivalent to:
>
> -----
> if (WARN_ON(nl80211_send_wiphy(...) < 0)
> nlmsg_free(msg);
> else
> genlmsg_multicast_netns(...);
>
> ... code for legacy ...
> -----
>
> no?
Ah, now I see what you want. Sure I will take care of this in v4.
Regards,
-Denis
On Wed, 2019-09-11 at 07:41 -0500, Denis Kenzior wrote:
>
> For my dual band cards the channel dump all fits into a ~1k attribute if
> I recall correctly. So there may not really be a need for this. Or at
> the very least we could keep things simple(r) and only split at the band
> level, not at the individual channel level.
Yeah, that does seem reasonable, especially if we're moving to bigger
messages anyway. If we do add something huge to each channel, we can
recover that code I suppose.
> > > [snip]
> > > +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;
> >
> > Thinking out loud, maybe we could simplify this by just having a small
> > "stack" of nested attributes to end?
> >
> > I mean, essentially, you have here similar code to the nla_put_failure
> > label, in that it finishes and sends out the message, except here you
> > have to end a bunch of nested attributes.
> >
> > What if we did something like
> >
> > #define dump_nest_start(msg, attr) ({ \
> > struct nlattr r = nla_nest_start_noflag(msg, attr); \
> > BUG_ON(nest_stack_depth >= ARRAY_SIZE(nest_stack); \
> > nest_stack[nest_stack_depth++] = r; \
> > r; \
> > })
> >
> > #define dump_nest_end(msg, r) do { \
> > BUG_ON(nest_stack_depth > 0); \
> > nest_stack_depth--; \
> > BUG_ON(nest_stack[nest_stack_depth] == r); \
> > nla_nest_end(msg, r); \
> > } while (0)
> >
> >
> > or something like that (we probably don't want to use
> > nla_nest_start_noflag() for future attributes, etc. but anyway).
> >
> > Then we could unwind any nesting at the end in the common code at the
> > nla_put_failure label very easily, I think?
>
> I see where you're going with this, I think I do anyway...
I'm not sure I am going anywhere with this ;-)
> The current logic uses last_channel_pos for some of the trickery in
> addition to last_good_pos. So nla_put_failure would have to be made
> aware of that. Perhaps we can store last_good_pos in the stack, but the
> split mechanism only allows the splits to be done at certain points...
Hmm, not sure I understand. last_channel_pos and last_good_pos are
basically equivalent, no? In fact, I'm not sure why you need
last_channel_pos vs. just using last_good_pos instead? Maybe I'm missing
something.
To me, conceptually, the "state->band_start" and "state->chan_start" is
basically a sub-state of "state->split", so this is underneath state-
>split == 3 (I think?), you basically get 3.0.0, 3.0.1, 3.0.2, ...,
3.1.0, 3.1.1 ... for the state? Which you have to unwind in terms of
message formatting, but the last_good_pos should be sufficient?
IOW, the only difference I see between the normal split states 1, 2, ...
and the band/channel split states 3.0.0, 3.0.1, ... is the fact that we
have to also fix up the nested attributes after we trim to last_good_pos
on failures. Where am I wrong?
> Right now only the channel dump uses this (and I'm still not fully
> convinced we should go to all the trouble), so one argument would be not
> to introduce something this generic until another user of it manifests
> itself?
I was thinking it'd actually be less complex, but I guess I have to try
to write it to see for myself.
johannes
Hi Johannes,
On 9/11/19 10:28 AM, Johannes Berg wrote:
> On Wed, 2019-09-11 at 07:41 -0500, Denis Kenzior wrote:
>>
>> For my dual band cards the channel dump all fits into a ~1k attribute if
>> I recall correctly. So there may not really be a need for this. Or at
>> the very least we could keep things simple(r) and only split at the band
>> level, not at the individual channel level.
>
> Yeah, that does seem reasonable, especially if we're moving to bigger
> messages anyway. If we do add something huge to each channel, we can
> recover that code I suppose.
So do you want me to drop the channel splitting logic and only allow
this for bands? Or just keep this since it is already done?
>> The current logic uses last_channel_pos for some of the trickery in
>> addition to last_good_pos. So nla_put_failure would have to be made
>> aware of that. Perhaps we can store last_good_pos in the stack, but the
>> split mechanism only allows the splits to be done at certain points...
>
> Hmm, not sure I understand. last_channel_pos and last_good_pos are
> basically equivalent, no? In fact, I'm not sure why you need
> last_channel_pos vs. just using last_good_pos instead? Maybe I'm missing
> something.
Sort of. The way I did it, last_channel_pos keeps track of whether any
channel info was added or not. So if NULL, we simply backtrack to
last_good_pos in nla_put_failure. You can probably use last_good_pos for
everything and an extra variable for the channel info tracking.
>
> To me, conceptually, the "state->band_start" and "state->chan_start" is
> basically a sub-state of "state->split", so this is underneath state-
>> split == 3 (I think?), you basically get 3.0.0, 3.0.1, 3.0.2, ...,
> 3.1.0, 3.1.1 ... for the state? Which you have to unwind in terms of
> message formatting, but the last_good_pos should be sufficient?
Right. And as I mentioned above, this could be done, but you probably
need another state variable..
>
> IOW, the only difference I see between the normal split states 1, 2, ...
> and the band/channel split states 3.0.0, 3.0.1, ... is the fact that we
> have to also fix up the nested attributes after we trim to last_good_pos
> on failures. Where am I wrong?
>
Didn't say that you were ;)
>> Right now only the channel dump uses this (and I'm still not fully
>> convinced we should go to all the trouble), so one argument would be not
>> to introduce something this generic until another user of it manifests
>> itself?
>
> I was thinking it'd actually be less complex, but I guess I have to try
> to write it to see for myself.
To be clear, I think it is a good approach and can be made to work. My
main hesitation is whether doing it now is worth it given the discussion
at the very top. But I can see what I can come up with if you want.
Regards,
-Denis
Hi,
> > Yeah, that does seem reasonable, especially if we're moving to bigger
> > messages anyway. If we do add something huge to each channel, we can
> > recover that code I suppose.
>
> So do you want me to drop the channel splitting logic and only allow
> this for bands? Or just keep this since it is already done?
I guess I'm saying I don't really care. If we have it now might as well
keep it?
Actually, don't we already have up to 240 bytes per channel, counting
the channel nesting itself and all the flags and WMM data that can be
inside?
Am I counting this wrong?
If this is right, we really do need to be able to split this, because we
have ~60 channels in the 6/7 GHz band ...
> > > The current logic uses last_channel_pos for some of the trickery in
> > > addition to last_good_pos. So nla_put_failure would have to be made
> > > aware of that. Perhaps we can store last_good_pos in the stack, but the
> > > split mechanism only allows the splits to be done at certain points...
> >
> > Hmm, not sure I understand. last_channel_pos and last_good_pos are
> > basically equivalent, no? In fact, I'm not sure why you need
> > last_channel_pos vs. just using last_good_pos instead? Maybe I'm missing
> > something.
>
> Sort of. The way I did it, last_channel_pos keeps track of whether any
> channel info was added or not. So if NULL, we simply backtrack to
> last_good_pos in nla_put_failure. You can probably use last_good_pos for
> everything and an extra variable for the channel info tracking.
Right.
> > To me, conceptually, the "state->band_start" and "state->chan_start" is
> > basically a sub-state of "state->split", so this is underneath state-
> > > split == 3 (I think?), you basically get 3.0.0, 3.0.1, 3.0.2, ...,
> > 3.1.0, 3.1.1 ... for the state? Which you have to unwind in terms of
> > message formatting, but the last_good_pos should be sufficient?
>
> Right. And as I mentioned above, this could be done, but you probably
> need another state variable..
>
> > IOW, the only difference I see between the normal split states 1, 2, ...
> > and the band/channel split states 3.0.0, 3.0.1, ... is the fact that we
> > have to also fix up the nested attributes after we trim to last_good_pos
> > on failures. Where am I wrong?
> >
>
> Didn't say that you were ;)
No, you didn't, but I thought I probably was missing something :)
> To be clear, I think it is a good approach and can be made to work. My
> main hesitation is whether doing it now is worth it given the discussion
> at the very top. But I can see what I can come up with if you want.
See above - unless I'm doing something completely wrong, each channel
with all the WMM data and stuff can really be big, no?
(Now, why did we put the WMM data into each channel? Maybe we could've
done per band? I think it can differ on the UNII subbands though)
johannes
> Ah, now I see what you want. Sure I will take care of this in v4.
I just remembered this because of Martin Willi's patch, and see now that
I actually just did the first patch of this series again while looking
into it ...
Did you ever make a v4 of this?
johannes
Hi Johannes,
On 9/28/20 6:14 AM, Johannes Berg wrote:
>
>> Ah, now I see what you want. Sure I will take care of this in v4.
>
> I just remembered this because of Martin Willi's patch, and see now that
> I actually just did the first patch of this series again while looking
> into it ...
>
> Did you ever make a v4 of this?
No. Got distracted by other work and never revisited this.
Regards,
-Denis