Add a new NL80211_CMD_SET_TX_BITRATE_MASK command and related
attributes to provide support for setting TX rate mask for rate
control. This uses the existing cfg80211 set_bitrate_mask operation
that was previously used only with WEXT compat code (SIOCSIWRATE). The
nl80211 command allows more generic configuration of allowed rates as
a mask instead of fixed/max rate.
Signed-off-by: Jouni Malinen <[email protected]>
---
include/linux/nl80211.h | 44 +++++++++++++++++++
include/net/cfg80211.h | 4 -
net/wireless/nl80211.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 157 insertions(+), 2 deletions(-)
--- wireless-testing.orig/include/linux/nl80211.h 2009-12-29 10:50:56.000000000 +0200
+++ wireless-testing/include/linux/nl80211.h 2009-12-29 10:56:42.000000000 +0200
@@ -295,6 +295,10 @@
* This command is also used as an event to notify when a requested
* remain-on-channel duration has expired.
*
+ * @NL80211_CMD_SET_TX_BITRATE_MASK: Set the mask of rates to be used in TX
+ * rate selection. %NL80211_ATTR_IFINDEX is used to specify the interface
+ * and @NL80211_ATTR_TX_RATES the set of allowed rates.
+ *
* @NL80211_CMD_MAX: highest used command number
* @__NL80211_CMD_AFTER_LAST: internal use
*/
@@ -381,6 +385,8 @@ enum nl80211_commands {
NL80211_CMD_REMAIN_ON_CHANNEL,
NL80211_CMD_CANCEL_REMAIN_ON_CHANNEL,
+ NL80211_CMD_SET_TX_BITRATE_MASK,
+
/* add new commands above here */
/* used to define NL80211_CMD_MAX below */
@@ -638,6 +644,13 @@ enum nl80211_commands {
*
* @NL80211_ATTR_COOKIE: Generic 64-bit cookie to identify objects.
*
+ * @NL80211_ATTR_TX_RATES: Nested set of attributes
+ * (enum nl80211_tx_rate_attributes) describing TX rates per band. The
+ * enum nl80211_band value is used as the index (nla_type() of the nested
+ * data. If a band is not included, it will be configured to allow all
+ * rates based on negotiated supported rates information. This attribute
+ * is used with %NL80211_CMD_SET_TX_BITRATE_MASK.
+ *
* @NL80211_ATTR_MAX: highest attribute number currently defined
* @__NL80211_ATTR_AFTER_LAST: internal use
*/
@@ -779,6 +792,8 @@ enum nl80211_attrs {
NL80211_ATTR_COOKIE,
+ NL80211_ATTR_TX_RATES,
+
/* add attributes here, update the policy in nl80211.c */
__NL80211_ATTR_AFTER_LAST,
@@ -1478,4 +1493,33 @@ enum nl80211_key_attributes {
NL80211_KEY_MAX = __NL80211_KEY_AFTER_LAST - 1
};
+/**
+ * enum nl80211_tx_rate_attributes - TX rate set attributes
+ * @__NL80211_TXRATE_INVALID: invalid
+ * @NL80211_TXRATE_LEGACY: Legacy (non-MCS) rates allowed for TX rate selection
+ * in an array of rates as defined in IEEE 802.11 7.3.2.2 (u8 values with
+ * 1 = 500 kbps) but without the IE length restriction (at most
+ * %NL80211_MAX_SUPP_RATES in a single array).
+ * @__NL80211_TXRATE_AFTER_LAST: internal
+ * @NL80211_TXRATE_MAX: highest TX rate attribute
+ */
+enum nl80211_tx_rate_attributes {
+ __NL80211_TXRATE_INVALID,
+ NL80211_TXRATE_LEGACY,
+
+ /* keep last */
+ __NL80211_TXRATE_AFTER_LAST,
+ NL80211_TXRATE_MAX = __NL80211_TXRATE_AFTER_LAST - 1
+};
+
+/**
+ * enum nl80211_band - Frequency band
+ * @NL80211_BAND_2GHZ - 2.4 GHz ISM band
+ * @NL80211_BAND_5GHZ - around 5 GHz band (4.9 - 5.7 GHz)
+ */
+enum nl80211_band {
+ NL80211_BAND_2GHZ,
+ NL80211_BAND_5GHZ,
+};
+
#endif /* __LINUX_NL80211_H */
--- wireless-testing.orig/net/wireless/nl80211.c 2009-12-29 10:50:57.000000000 +0200
+++ wireless-testing/net/wireless/nl80211.c 2009-12-29 10:57:37.000000000 +0200
@@ -143,6 +143,7 @@ static struct nla_policy nl80211_policy[
.len = WLAN_PMKID_LEN },
[NL80211_ATTR_DURATION] = { .type = NLA_U32 },
[NL80211_ATTR_COOKIE] = { .type = NLA_U64 },
+ [NL80211_ATTR_TX_RATES] = { .type = NLA_NESTED },
};
/* policy for the attributes */
@@ -572,6 +573,7 @@ static int nl80211_send_wiphy(struct sk_
CMD(del_pmksa, DEL_PMKSA);
CMD(flush_pmksa, FLUSH_PMKSA);
CMD(remain_on_channel, REMAIN_ON_CHANNEL);
+ CMD(set_bitrate_mask, SET_TX_BITRATE_MASK);
if (dev->wiphy.flags & WIPHY_FLAG_NETNS_OK) {
i++;
NLA_PUT_U32(msg, i, NL80211_CMD_SET_WIPHY_NETNS);
@@ -4423,6 +4425,109 @@ static int nl80211_cancel_remain_on_chan
return err;
}
+static u32 rateset_to_mask(struct ieee80211_supported_band *sband,
+ u8 *rates, u8 rates_len)
+{
+ u8 i;
+ u32 mask = 0;
+
+ for (i = 0; i < rates_len; i++) {
+ int rate = (rates[i] & 0x7f) * 5;
+ int ridx;
+ for (ridx = 0; ridx < sband->n_bitrates; ridx++) {
+ struct ieee80211_rate *srate =
+ &sband->bitrates[ridx];
+ if (rate == srate->bitrate) {
+ mask |= 1 << ridx;
+ break;
+ }
+ }
+ if (ridx == sband->n_bitrates)
+ return 0; /* rate not found */
+ }
+
+ return mask;
+}
+
+static struct nla_policy
+nl80211_txattr_policy[NL80211_TXRATE_MAX + 1] __read_mostly = {
+ [NL80211_TXRATE_LEGACY] = { .type = NLA_BINARY,
+ .len = NL80211_MAX_SUPP_RATES },
+};
+
+static int nl80211_set_tx_bitrate_mask(struct sk_buff *skb,
+ struct genl_info *info)
+{
+ struct nlattr *tb[NL80211_TXRATE_MAX + 1];
+ struct cfg80211_registered_device *rdev;
+ struct cfg80211_bitrate_mask mask;
+ int err, rem, i;
+ struct net_device *dev;
+ struct nlattr *tx_rates;
+ struct ieee80211_supported_band *sband;
+
+ if (info->attrs[NL80211_ATTR_TX_RATES] == NULL)
+ return -EINVAL;
+
+ rtnl_lock();
+
+ err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
+ if (err)
+ goto unlock_rtnl;
+
+ if (!rdev->ops->set_bitrate_mask) {
+ err = -EOPNOTSUPP;
+ goto unlock;
+ }
+
+ memset(&mask, 0, sizeof(mask));
+ /* Default to all rates enabled */
+ for (i = 0; i < IEEE80211_NUM_BANDS; i++) {
+ sband = rdev->wiphy.bands[i];
+ mask.control[i].legacy =
+ sband ? (1 << sband->n_bitrates) - 1 : 0;
+ }
+
+ /*
+ * The nested attribute uses enum nl80211_band as the index. This maps
+ * directly to the enum ieee80211_band values used in cfg80211.
+ */
+ nla_for_each_nested(tx_rates, info->attrs[NL80211_ATTR_TX_RATES], rem)
+ {
+ enum ieee80211_band band = nla_type(tx_rates);
+ if (band < 0 || band >= IEEE80211_NUM_BANDS) {
+ err = -EINVAL;
+ goto unlock;
+ }
+ sband = rdev->wiphy.bands[band];
+ if (sband == NULL) {
+ err = -EINVAL;
+ goto unlock;
+ }
+ nla_parse(tb, NL80211_TXRATE_MAX, nla_data(tx_rates),
+ nla_len(tx_rates), nl80211_txattr_policy);
+ if (tb[NL80211_TXRATE_LEGACY]) {
+ mask.control[band].legacy = rateset_to_mask(
+ sband,
+ nla_data(tb[NL80211_TXRATE_LEGACY]),
+ nla_len(tb[NL80211_TXRATE_LEGACY]));
+ if (mask.control[band].legacy == 0) {
+ err = -EINVAL;
+ goto unlock;
+ }
+ }
+ }
+
+ err = rdev->ops->set_bitrate_mask(&rdev->wiphy, dev, NULL, &mask);
+
+ unlock:
+ dev_put(dev);
+ cfg80211_unlock_rdev(rdev);
+ unlock_rtnl:
+ rtnl_unlock();
+ return err;
+}
+
static struct genl_ops nl80211_ops[] = {
{
.cmd = NL80211_CMD_GET_WIPHY,
@@ -4697,6 +4802,12 @@ static struct genl_ops nl80211_ops[] = {
.policy = nl80211_policy,
.flags = GENL_ADMIN_PERM,
},
+ {
+ .cmd = NL80211_CMD_SET_TX_BITRATE_MASK,
+ .doit = nl80211_set_tx_bitrate_mask,
+ .policy = nl80211_policy,
+ .flags = GENL_ADMIN_PERM,
+ },
};
static struct genl_multicast_group nl80211_mlme_mcgrp = {
--- wireless-testing.orig/include/net/cfg80211.h 2009-12-29 10:54:29.000000000 +0200
+++ wireless-testing/include/net/cfg80211.h 2009-12-29 10:55:52.000000000 +0200
@@ -39,8 +39,8 @@
* @IEEE80211_BAND_5GHZ: around 5GHz band (4.9-5.7)
*/
enum ieee80211_band {
- IEEE80211_BAND_2GHZ,
- IEEE80211_BAND_5GHZ,
+ IEEE80211_BAND_2GHZ = NL80211_BAND_2GHZ,
+ IEEE80211_BAND_5GHZ = NL80211_BAND_5GHZ,
/* keep last */
IEEE80211_NUM_BANDS
--
--
Jouni Malinen PGP id EFC895FA
On Tue, 2009-12-29 at 12:59 +0200, Jouni Malinen wrote:
> plain text document attachment (nl80211-rc-rate-mask.patch)
> Add a new NL80211_CMD_SET_TX_BITRATE_MASK command and related
> attributes to provide support for setting TX rate mask for rate
> control. This uses the existing cfg80211 set_bitrate_mask operation
> that was previously used only with WEXT compat code (SIOCSIWRATE). The
> nl80211 command allows more generic configuration of allowed rates as
> a mask instead of fixed/max rate.
>
> Signed-off-by: Jouni Malinen <[email protected]>
Looks fine.
Acked-by: Johannes Berg <[email protected]>
> ---
> include/linux/nl80211.h | 44 +++++++++++++++++++
> include/net/cfg80211.h | 4 -
> net/wireless/nl80211.c | 111
> ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 157 insertions(+), 2 deletions(-)
>
> --- wireless-testing.orig/include/linux/nl80211.h 2009-12-29
> 10:50:56.000000000 +0200
> +++ wireless-testing/include/linux/nl80211.h 2009-12-29
> 10:56:42.000000000 +0200
> @@ -295,6 +295,10 @@
> * This command is also used as an event to notify when a requested
> * remain-on-channel duration has expired.
> *
> + * @NL80211_CMD_SET_TX_BITRATE_MASK: Set the mask of rates to be used
> in TX
> + * rate selection. %NL80211_ATTR_IFINDEX is used to specify the
> interface
> + * and @NL80211_ATTR_TX_RATES the set of allowed rates.
> + *
> * @NL80211_CMD_MAX: highest used command number
> * @__NL80211_CMD_AFTER_LAST: internal use
> */
> @@ -381,6 +385,8 @@ enum nl80211_commands {
> NL80211_CMD_REMAIN_ON_CHANNEL,
> NL80211_CMD_CANCEL_REMAIN_ON_CHANNEL,
>
> + NL80211_CMD_SET_TX_BITRATE_MASK,
> +
> /* add new commands above here */
>
> /* used to define NL80211_CMD_MAX below */
> @@ -638,6 +644,13 @@ enum nl80211_commands {
> *
> * @NL80211_ATTR_COOKIE: Generic 64-bit cookie to identify objects.
> *
> + * @NL80211_ATTR_TX_RATES: Nested set of attributes
> + * (enum nl80211_tx_rate_attributes) describing TX rates per band.
> The
> + * enum nl80211_band value is used as the index (nla_type() of the
> nested
> + * data. If a band is not included, it will be configured to allow
> all
> + * rates based on negotiated supported rates information. This
> attribute
> + * is used with %NL80211_CMD_SET_TX_BITRATE_MASK.
> + *
> * @NL80211_ATTR_MAX: highest attribute number currently defined
> * @__NL80211_ATTR_AFTER_LAST: internal use
> */
> @@ -779,6 +792,8 @@ enum nl80211_attrs {
>
> NL80211_ATTR_COOKIE,
>
> + NL80211_ATTR_TX_RATES,
> +
> /* add attributes here, update the policy in nl80211.c */
>
> __NL80211_ATTR_AFTER_LAST,
> @@ -1478,4 +1493,33 @@ enum nl80211_key_attributes {
> NL80211_KEY_MAX = __NL80211_KEY_AFTER_LAST - 1
> };
>
> +/**
> + * enum nl80211_tx_rate_attributes - TX rate set attributes
> + * @__NL80211_TXRATE_INVALID: invalid
> + * @NL80211_TXRATE_LEGACY: Legacy (non-MCS) rates allowed for TX rate
> selection
> + * in an array of rates as defined in IEEE 802.11 7.3.2.2 (u8 values
> with
> + * 1 = 500 kbps) but without the IE length restriction (at most
> + * %NL80211_MAX_SUPP_RATES in a single array).
> + * @__NL80211_TXRATE_AFTER_LAST: internal
> + * @NL80211_TXRATE_MAX: highest TX rate attribute
> + */
> +enum nl80211_tx_rate_attributes {
> + __NL80211_TXRATE_INVALID,
> + NL80211_TXRATE_LEGACY,
> +
> + /* keep last */
> + __NL80211_TXRATE_AFTER_LAST,
> + NL80211_TXRATE_MAX = __NL80211_TXRATE_AFTER_LAST - 1
> +};
> +
> +/**
> + * enum nl80211_band - Frequency band
> + * @NL80211_BAND_2GHZ - 2.4 GHz ISM band
> + * @NL80211_BAND_5GHZ - around 5 GHz band (4.9 - 5.7 GHz)
> + */
> +enum nl80211_band {
> + NL80211_BAND_2GHZ,
> + NL80211_BAND_5GHZ,
> +};
> +
> #endif /* __LINUX_NL80211_H */
> --- wireless-testing.orig/net/wireless/nl80211.c 2009-12-29
> 10:50:57.000000000 +0200
> +++ wireless-testing/net/wireless/nl80211.c 2009-12-29
> 10:57:37.000000000 +0200
> @@ -143,6 +143,7 @@ static struct nla_policy nl80211_policy[
> .len = WLAN_PMKID_LEN },
> [NL80211_ATTR_DURATION] = { .type = NLA_U32 },
> [NL80211_ATTR_COOKIE] = { .type = NLA_U64 },
> + [NL80211_ATTR_TX_RATES] = { .type = NLA_NESTED },
> };
>
> /* policy for the attributes */
> @@ -572,6 +573,7 @@ static int nl80211_send_wiphy(struct sk_
> CMD(del_pmksa, DEL_PMKSA);
> CMD(flush_pmksa, FLUSH_PMKSA);
> CMD(remain_on_channel, REMAIN_ON_CHANNEL);
> + CMD(set_bitrate_mask, SET_TX_BITRATE_MASK);
> if (dev->wiphy.flags & WIPHY_FLAG_NETNS_OK) {
> i++;
> NLA_PUT_U32(msg, i, NL80211_CMD_SET_WIPHY_NETNS);
> @@ -4423,6 +4425,109 @@ static int nl80211_cancel_remain_on_chan
> return err;
> }
>
> +static u32 rateset_to_mask(struct ieee80211_supported_band *sband,
> + u8 *rates, u8 rates_len)
> +{
> + u8 i;
> + u32 mask = 0;
> +
> + for (i = 0; i < rates_len; i++) {
> + int rate = (rates[i] & 0x7f) * 5;
> + int ridx;
> + for (ridx = 0; ridx < sband->n_bitrates; ridx++) {
> + struct ieee80211_rate *srate =
> + &sband->bitrates[ridx];
> + if (rate == srate->bitrate) {
> + mask |= 1 << ridx;
> + break;
> + }
> + }
> + if (ridx == sband->n_bitrates)
> + return 0; /* rate not found */
> + }
> +
> + return mask;
> +}
> +
> +static struct nla_policy
> +nl80211_txattr_policy[NL80211_TXRATE_MAX + 1] __read_mostly = {
> + [NL80211_TXRATE_LEGACY] = { .type = NLA_BINARY,
> + .len = NL80211_MAX_SUPP_RATES },
> +};
> +
> +static int nl80211_set_tx_bitrate_mask(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct nlattr *tb[NL80211_TXRATE_MAX + 1];
> + struct cfg80211_registered_device *rdev;
> + struct cfg80211_bitrate_mask mask;
> + int err, rem, i;
> + struct net_device *dev;
> + struct nlattr *tx_rates;
> + struct ieee80211_supported_band *sband;
> +
> + if (info->attrs[NL80211_ATTR_TX_RATES] == NULL)
> + return -EINVAL;
> +
> + rtnl_lock();
> +
> + err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
> + if (err)
> + goto unlock_rtnl;
> +
> + if (!rdev->ops->set_bitrate_mask) {
> + err = -EOPNOTSUPP;
> + goto unlock;
> + }
> +
> + memset(&mask, 0, sizeof(mask));
> + /* Default to all rates enabled */
> + for (i = 0; i < IEEE80211_NUM_BANDS; i++) {
> + sband = rdev->wiphy.bands[i];
> + mask.control[i].legacy =
> + sband ? (1 << sband->n_bitrates) - 1 : 0;
> + }
> +
> + /*
> + * The nested attribute uses enum nl80211_band as the index. This
> maps
> + * directly to the enum ieee80211_band values used in cfg80211.
> + */
> + nla_for_each_nested(tx_rates, info->attrs[NL80211_ATTR_TX_RATES],
> rem)
> + {
> + enum ieee80211_band band = nla_type(tx_rates);
> + if (band < 0 || band >= IEEE80211_NUM_BANDS) {
> + err = -EINVAL;
> + goto unlock;
> + }
> + sband = rdev->wiphy.bands[band];
> + if (sband == NULL) {
> + err = -EINVAL;
> + goto unlock;
> + }
> + nla_parse(tb, NL80211_TXRATE_MAX, nla_data(tx_rates),
> + nla_len(tx_rates), nl80211_txattr_policy);
> + if (tb[NL80211_TXRATE_LEGACY]) {
> + mask.control[band].legacy = rateset_to_mask(
> + sband,
> + nla_data(tb[NL80211_TXRATE_LEGACY]),
> + nla_len(tb[NL80211_TXRATE_LEGACY]));
> + if (mask.control[band].legacy == 0) {
> + err = -EINVAL;
> + goto unlock;
> + }
> + }
> + }
> +
> + err = rdev->ops->set_bitrate_mask(&rdev->wiphy, dev, NULL, &mask);
> +
> + unlock:
> + dev_put(dev);
> + cfg80211_unlock_rdev(rdev);
> + unlock_rtnl:
> + rtnl_unlock();
> + return err;
> +}
> +
> static struct genl_ops nl80211_ops[] = {
> {
> .cmd = NL80211_CMD_GET_WIPHY,
> @@ -4697,6 +4802,12 @@ static struct genl_ops nl80211_ops[] = {
> .policy = nl80211_policy,
> .flags = GENL_ADMIN_PERM,
> },
> + {
> + .cmd = NL80211_CMD_SET_TX_BITRATE_MASK,
> + .doit = nl80211_set_tx_bitrate_mask,
> + .policy = nl80211_policy,
> + .flags = GENL_ADMIN_PERM,
> + },
> };
>
> static struct genl_multicast_group nl80211_mlme_mcgrp = {
> --- wireless-testing.orig/include/net/cfg80211.h 2009-12-29
> 10:54:29.000000000 +0200
> +++ wireless-testing/include/net/cfg80211.h 2009-12-29
> 10:55:52.000000000 +0200
> @@ -39,8 +39,8 @@
> * @IEEE80211_BAND_5GHZ: around 5GHz band (4.9-5.7)
> */
> enum ieee80211_band {
> - IEEE80211_BAND_2GHZ,
> - IEEE80211_BAND_5GHZ,
> + IEEE80211_BAND_2GHZ = NL80211_BAND_2GHZ,
> + IEEE80211_BAND_5GHZ = NL80211_BAND_5GHZ,
>
> /* keep last */
> IEEE80211_NUM_BANDS
>
> --
>
On 8/24/10 3:16 AM, Johannes Berg wrote:
> On Fri, 2010-08-20 at 15:54 -0700, Philip Prindeville wrote:
>
>> What I'm trying to say is that even though the driver handles the
>> condition of nla_type == 0 when it generates the message, the
>> condition isn't handled correct when the message finally gets passed
>> up into user-space, because the above sequence in the netlink library
>> gets hit both for "iw" and "hostapd".
>>
>> So it's not enough to handle it correctly in the driver: it needs to
>> be handled in libnl as well.
>>
>> There are 3 potential solutions:
>>
>> (1) go with a 1-based enum for the band, instead of 0-based;
>> (2) remove the test for nla_type == 0 in nla_parse() in libnl;
>> (3) use a way to convey the band that doesn't involve overloading
>> nla_type;
>>
>> What's the correct fix?
> You managed to totally throw me off course ... The problem is completely
> unrelated to this, see the patch I just sent :-)
>
> johannes
>
Confirmed your fix. http://pastebin.ca/1924624
Thanks!
On 8/20/10 9:33 AM, Philip Prindeville wrote:
> On 8/20/10 2:20 AM, Johannes Berg wrote:
>> On Thu, 2010-08-19 at 14:26 -0700, Philip Prindeville wrote:
>>
>>>> + /*
>>>> + * The nested attribute uses enum nl80211_band as the index. This maps
>>>> + * directly to the enum ieee80211_band values used in cfg80211.
>>>> + */
>>>> + nla_for_each_nested(tx_rates, info->attrs[NL80211_ATTR_TX_RATES], rem)
>>>> + {
>>>> + enum ieee80211_band band = nla_type(tx_rates);
>>> Can this even work? The first entry in nl80211_band is NL80211_BAND_2GHZ, i.e. zero.
>>>
>>> Yet looking at libnl-1.1/lib/attr.c there's:
>>>
>>> int nla_parse(struct nlattr *tb[], int maxtype, struct nlattr *head, int len,
>>> struct nla_policy *policy)
>>> {
>>> ...
>>> nla_for_each_attr(nla, head, len, rem) {
>>> int type = nla_type(nla);
>>>
>>> if (type == 0) {
>> Well, notice how you're quoting code that does the same thing
>> (for_each_attr), but the latter code does the extra non-zero check,
>> which we don't.
>>
>> We don't use nla_parse for this, we have to use
>> nla_parse_nested/for_each_attr. Yeah, it's a little quirky, but still
>> works fine.
>>
>> johannes
>>
>
> Well, I grepped through all the relevant source (i.e. libnl and iw) and the only place that the string "Illegal nla->nla_type == 0" gets printed was there, and I'm definitely seeing it.
>
> So while the condition might be set in the driver, it's detected in "iw" here.
>
> That was my point.
What I'm trying to say is that even though the driver handles the condition of nla_type == 0 when it generates the message, the condition isn't handled correct when the message finally gets passed up into user-space, because the above sequence in the netlink library gets hit both for "iw" and "hostapd".
So it's not enough to handle it correctly in the driver: it needs to be handled in libnl as well.
There are 3 potential solutions:
(1) go with a 1-based enum for the band, instead of 0-based;
(2) remove the test for nla_type == 0 in nla_parse() in libnl;
(3) use a way to convey the band that doesn't involve overloading nla_type;
What's the correct fix?
Thanks,
-Philip
On Fri, 2010-08-20 at 15:54 -0700, Philip Prindeville wrote:
> What I'm trying to say is that even though the driver handles the
> condition of nla_type == 0 when it generates the message, the
> condition isn't handled correct when the message finally gets passed
> up into user-space, because the above sequence in the netlink library
> gets hit both for "iw" and "hostapd".
>
> So it's not enough to handle it correctly in the driver: it needs to
> be handled in libnl as well.
>
> There are 3 potential solutions:
>
> (1) go with a 1-based enum for the band, instead of 0-based;
> (2) remove the test for nla_type == 0 in nla_parse() in libnl;
> (3) use a way to convey the band that doesn't involve overloading
> nla_type;
>
> What's the correct fix?
You managed to totally throw me off course ... The problem is completely
unrelated to this, see the patch I just sent :-)
johannes
On 12/29/09 2:59 AM, Jouni Malinen wrote:
> Add a new NL80211_CMD_SET_TX_BITRATE_MASK command and related
> attributes to provide support for setting TX rate mask for rate
> control. This uses the existing cfg80211 set_bitrate_mask operation
> that was previously used only with WEXT compat code (SIOCSIWRATE). The
> nl80211 command allows more generic configuration of allowed rates as
> a mask instead of fixed/max rate.
>
> Signed-off-by: Jouni Malinen<[email protected]>
>
> ---
> include/linux/nl80211.h | 44 +++++++++++++++++++
> include/net/cfg80211.h | 4 -
> net/wireless/nl80211.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 157 insertions(+), 2 deletions(-)
>
> --- wireless-testing.orig/include/linux/nl80211.h 2009-12-29 10:50:56.000000000 +0200
> +++ wireless-testing/include/linux/nl80211.h 2009-12-29 10:56:42.000000000 +0200
> @@ -295,6 +295,10 @@
> * This command is also used as an event to notify when a requested
> * remain-on-channel duration has expired.
> *
> + * @NL80211_CMD_SET_TX_BITRATE_MASK: Set the mask of rates to be used in TX
> + * rate selection. %NL80211_ATTR_IFINDEX is used to specify the interface
> + * and @NL80211_ATTR_TX_RATES the set of allowed rates.
> + *
> * @NL80211_CMD_MAX: highest used command number
> * @__NL80211_CMD_AFTER_LAST: internal use
> */
> @@ -381,6 +385,8 @@ enum nl80211_commands {
> NL80211_CMD_REMAIN_ON_CHANNEL,
> NL80211_CMD_CANCEL_REMAIN_ON_CHANNEL,
>
> + NL80211_CMD_SET_TX_BITRATE_MASK,
> +
> /* add new commands above here */
>
> /* used to define NL80211_CMD_MAX below */
> @@ -638,6 +644,13 @@ enum nl80211_commands {
> *
> * @NL80211_ATTR_COOKIE: Generic 64-bit cookie to identify objects.
> *
> + * @NL80211_ATTR_TX_RATES: Nested set of attributes
> + * (enum nl80211_tx_rate_attributes) describing TX rates per band. The
> + * enum nl80211_band value is used as the index (nla_type() of the nested
> + * data. If a band is not included, it will be configured to allow all
> + * rates based on negotiated supported rates information. This attribute
> + * is used with %NL80211_CMD_SET_TX_BITRATE_MASK.
> + *
> * @NL80211_ATTR_MAX: highest attribute number currently defined
> * @__NL80211_ATTR_AFTER_LAST: internal use
> */
> @@ -779,6 +792,8 @@ enum nl80211_attrs {
>
> NL80211_ATTR_COOKIE,
>
> + NL80211_ATTR_TX_RATES,
> +
> /* add attributes here, update the policy in nl80211.c */
>
> __NL80211_ATTR_AFTER_LAST,
> @@ -1478,4 +1493,33 @@ enum nl80211_key_attributes {
> NL80211_KEY_MAX = __NL80211_KEY_AFTER_LAST - 1
> };
>
> +/**
> + * enum nl80211_tx_rate_attributes - TX rate set attributes
> + * @__NL80211_TXRATE_INVALID: invalid
> + * @NL80211_TXRATE_LEGACY: Legacy (non-MCS) rates allowed for TX rate selection
> + * in an array of rates as defined in IEEE 802.11 7.3.2.2 (u8 values with
> + * 1 = 500 kbps) but without the IE length restriction (at most
> + * %NL80211_MAX_SUPP_RATES in a single array).
> + * @__NL80211_TXRATE_AFTER_LAST: internal
> + * @NL80211_TXRATE_MAX: highest TX rate attribute
> + */
> +enum nl80211_tx_rate_attributes {
> + __NL80211_TXRATE_INVALID,
> + NL80211_TXRATE_LEGACY,
> +
> + /* keep last */
> + __NL80211_TXRATE_AFTER_LAST,
> + NL80211_TXRATE_MAX = __NL80211_TXRATE_AFTER_LAST - 1
> +};
> +
> +/**
> + * enum nl80211_band - Frequency band
> + * @NL80211_BAND_2GHZ - 2.4 GHz ISM band
> + * @NL80211_BAND_5GHZ - around 5 GHz band (4.9 - 5.7 GHz)
> + */
> +enum nl80211_band {
> + NL80211_BAND_2GHZ,
> + NL80211_BAND_5GHZ,
> +};
> +
> #endif /* __LINUX_NL80211_H */
> --- wireless-testing.orig/net/wireless/nl80211.c 2009-12-29 10:50:57.000000000 +0200
> +++ wireless-testing/net/wireless/nl80211.c 2009-12-29 10:57:37.000000000 +0200
> @@ -143,6 +143,7 @@ static struct nla_policy nl80211_policy[
> .len = WLAN_PMKID_LEN },
> [NL80211_ATTR_DURATION] = { .type = NLA_U32 },
> [NL80211_ATTR_COOKIE] = { .type = NLA_U64 },
> + [NL80211_ATTR_TX_RATES] = { .type = NLA_NESTED },
> };
>
> /* policy for the attributes */
> @@ -572,6 +573,7 @@ static int nl80211_send_wiphy(struct sk_
> CMD(del_pmksa, DEL_PMKSA);
> CMD(flush_pmksa, FLUSH_PMKSA);
> CMD(remain_on_channel, REMAIN_ON_CHANNEL);
> + CMD(set_bitrate_mask, SET_TX_BITRATE_MASK);
> if (dev->wiphy.flags& WIPHY_FLAG_NETNS_OK) {
> i++;
> NLA_PUT_U32(msg, i, NL80211_CMD_SET_WIPHY_NETNS);
> @@ -4423,6 +4425,109 @@ static int nl80211_cancel_remain_on_chan
> return err;
> }
>
> +static u32 rateset_to_mask(struct ieee80211_supported_band *sband,
> + u8 *rates, u8 rates_len)
> +{
> + u8 i;
> + u32 mask = 0;
> +
> + for (i = 0; i< rates_len; i++) {
> + int rate = (rates[i]& 0x7f) * 5;
> + int ridx;
> + for (ridx = 0; ridx< sband->n_bitrates; ridx++) {
> + struct ieee80211_rate *srate =
> + &sband->bitrates[ridx];
> + if (rate == srate->bitrate) {
> + mask |= 1<< ridx;
> + break;
> + }
> + }
> + if (ridx == sband->n_bitrates)
> + return 0; /* rate not found */
> + }
> +
> + return mask;
> +}
> +
> +static struct nla_policy
> +nl80211_txattr_policy[NL80211_TXRATE_MAX + 1] __read_mostly = {
> + [NL80211_TXRATE_LEGACY] = { .type = NLA_BINARY,
> + .len = NL80211_MAX_SUPP_RATES },
> +};
> +
> +static int nl80211_set_tx_bitrate_mask(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct nlattr *tb[NL80211_TXRATE_MAX + 1];
> + struct cfg80211_registered_device *rdev;
> + struct cfg80211_bitrate_mask mask;
> + int err, rem, i;
> + struct net_device *dev;
> + struct nlattr *tx_rates;
> + struct ieee80211_supported_band *sband;
> +
> + if (info->attrs[NL80211_ATTR_TX_RATES] == NULL)
> + return -EINVAL;
> +
> + rtnl_lock();
> +
> + err = get_rdev_dev_by_info_ifindex(info,&rdev,&dev);
> + if (err)
> + goto unlock_rtnl;
> +
> + if (!rdev->ops->set_bitrate_mask) {
> + err = -EOPNOTSUPP;
> + goto unlock;
> + }
> +
> + memset(&mask, 0, sizeof(mask));
> + /* Default to all rates enabled */
> + for (i = 0; i< IEEE80211_NUM_BANDS; i++) {
> + sband = rdev->wiphy.bands[i];
> + mask.control[i].legacy =
> + sband ? (1<< sband->n_bitrates) - 1 : 0;
> + }
> +
> + /*
> + * The nested attribute uses enum nl80211_band as the index. This maps
> + * directly to the enum ieee80211_band values used in cfg80211.
> + */
> + nla_for_each_nested(tx_rates, info->attrs[NL80211_ATTR_TX_RATES], rem)
> + {
> + enum ieee80211_band band = nla_type(tx_rates);
Can this even work? The first entry in nl80211_band is NL80211_BAND_2GHZ, i.e. zero.
Yet looking at libnl-1.1/lib/attr.c there's:
int nla_parse(struct nlattr *tb[], int maxtype, struct nlattr *head, int len,
struct nla_policy *policy)
{
...
nla_for_each_attr(nla, head, len, rem) {
int type = nla_type(nla);
if (type == 0) {
fprintf(stderr, "Illegal nla->nla_type == 0\n");
continue;
}
so any time nla->nla_type is set to NL80211_BAND_2GHZ, isn't this going to be problematic?
Or am I misreading this code?
-Philip
> + if (band< 0 || band>= IEEE80211_NUM_BANDS) {
> + err = -EINVAL;
> + goto unlock;
> + }
> + sband = rdev->wiphy.bands[band];
> + if (sband == NULL) {
> + err = -EINVAL;
> + goto unlock;
> + }
> + nla_parse(tb, NL80211_TXRATE_MAX, nla_data(tx_rates),
> + nla_len(tx_rates), nl80211_txattr_policy);
> + if (tb[NL80211_TXRATE_LEGACY]) {
> + mask.control[band].legacy = rateset_to_mask(
> + sband,
> + nla_data(tb[NL80211_TXRATE_LEGACY]),
> + nla_len(tb[NL80211_TXRATE_LEGACY]));
> + if (mask.control[band].legacy == 0) {
> + err = -EINVAL;
> + goto unlock;
> + }
> + }
> + }
> +
> + err = rdev->ops->set_bitrate_mask(&rdev->wiphy, dev, NULL,&mask);
> +
> + unlock:
> + dev_put(dev);
> + cfg80211_unlock_rdev(rdev);
> + unlock_rtnl:
> + rtnl_unlock();
> + return err;
> +}
> +
> static struct genl_ops nl80211_ops[] = {
> {
> .cmd = NL80211_CMD_GET_WIPHY,
> @@ -4697,6 +4802,12 @@ static struct genl_ops nl80211_ops[] = {
> .policy = nl80211_policy,
> .flags = GENL_ADMIN_PERM,
> },
> + {
> + .cmd = NL80211_CMD_SET_TX_BITRATE_MASK,
> + .doit = nl80211_set_tx_bitrate_mask,
> + .policy = nl80211_policy,
> + .flags = GENL_ADMIN_PERM,
> + },
> };
>
> static struct genl_multicast_group nl80211_mlme_mcgrp = {
> --- wireless-testing.orig/include/net/cfg80211.h 2009-12-29 10:54:29.000000000 +0200
> +++ wireless-testing/include/net/cfg80211.h 2009-12-29 10:55:52.000000000 +0200
> @@ -39,8 +39,8 @@
> * @IEEE80211_BAND_5GHZ: around 5GHz band (4.9-5.7)
> */
> enum ieee80211_band {
> - IEEE80211_BAND_2GHZ,
> - IEEE80211_BAND_5GHZ,
> + IEEE80211_BAND_2GHZ = NL80211_BAND_2GHZ,
> + IEEE80211_BAND_5GHZ = NL80211_BAND_5GHZ,
>
> /* keep last */
> IEEE80211_NUM_BANDS
>
On 8/20/10 2:20 AM, Johannes Berg wrote:
> On Thu, 2010-08-19 at 14:26 -0700, Philip Prindeville wrote:
>
>>> + /*
>>> + * The nested attribute uses enum nl80211_band as the index. This maps
>>> + * directly to the enum ieee80211_band values used in cfg80211.
>>> + */
>>> + nla_for_each_nested(tx_rates, info->attrs[NL80211_ATTR_TX_RATES], rem)
>>> + {
>>> + enum ieee80211_band band = nla_type(tx_rates);
>> Can this even work? The first entry in nl80211_band is NL80211_BAND_2GHZ, i.e. zero.
>>
>> Yet looking at libnl-1.1/lib/attr.c there's:
>>
>> int nla_parse(struct nlattr *tb[], int maxtype, struct nlattr *head, int len,
>> struct nla_policy *policy)
>> {
>> ...
>> nla_for_each_attr(nla, head, len, rem) {
>> int type = nla_type(nla);
>>
>> if (type == 0) {
> Well, notice how you're quoting code that does the same thing
> (for_each_attr), but the latter code does the extra non-zero check,
> which we don't.
>
> We don't use nla_parse for this, we have to use
> nla_parse_nested/for_each_attr. Yeah, it's a little quirky, but still
> works fine.
>
> johannes
>
Well, I grepped through all the relevant source (i.e. libnl and iw) and the only place that the string "Illegal nla->nla_type == 0" gets printed was there, and I'm definitely seeing it.
So while the condition might be set in the driver, it's detected in "iw" here.
That was my point.
On Thu, 2010-08-19 at 14:26 -0700, Philip Prindeville wrote:
> > + /*
> > + * The nested attribute uses enum nl80211_band as the index. This maps
> > + * directly to the enum ieee80211_band values used in cfg80211.
> > + */
> > + nla_for_each_nested(tx_rates, info->attrs[NL80211_ATTR_TX_RATES], rem)
> > + {
> > + enum ieee80211_band band = nla_type(tx_rates);
>
> Can this even work? The first entry in nl80211_band is NL80211_BAND_2GHZ, i.e. zero.
>
> Yet looking at libnl-1.1/lib/attr.c there's:
>
> int nla_parse(struct nlattr *tb[], int maxtype, struct nlattr *head, int len,
> struct nla_policy *policy)
> {
> ...
> nla_for_each_attr(nla, head, len, rem) {
> int type = nla_type(nla);
>
> if (type == 0) {
Well, notice how you're quoting code that does the same thing
(for_each_attr), but the latter code does the extra non-zero check,
which we don't.
We don't use nla_parse for this, we have to use
nla_parse_nested/for_each_attr. Yeah, it's a little quirky, but still
works fine.
johannes