[RFC] cfg80211: Add feature flag for 4-way handshake offload

[RFC] cfg80211: Add feature flag for 4-way handshake offload

[Arend van Spriel]

From: Gautam Kumar Shukla <gautams@broadcom.com>

The new feature flag allows the driver to indicate that it can
offload the 4-way handshake for WPA/RSN-PSK. With the
wiphy::features flag being used up this patch adds a new
field wiphy::ext_features. Considering extensibility this
new field is declared as a byte array.

Signed-off-by: Gautam (Gautam Kumar) Shukla <gautams@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
Hi Johannes,

Here the proposed way to deal with new feature flags. Let
me know if this is suitable.

Regards,
Arend
---
 include/net/cfg80211.h       | 25 +++++++++++++++++++++++++
 include/uapi/linux/nl80211.h | 13 +++++++++++++
 net/wireless/nl80211.c       |  5 +++++
 net/wireless/util.c          | 20 ++++++++++++++++++++
 4 files changed, 63 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 670b9ed..8cb04da 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3013,6 +3013,8 @@ struct wiphy_vendor_command {
  * @regulatory_flags: wiphy regulatory flags, see
  *	&enum ieee80211_regulatory_flags
  * @features: features advertised to nl80211, see &enum nl80211_feature_flags.
+ * @ext_features: extended features advertised to nl80211, see
+ *	&enum nl80211_ext_feature_index.
  * @bss_priv_size: each BSS struct has private data allocated with it,
  *	this variable determines its size
  * @max_scan_ssids: maximum number of SSIDs the device can scan for in
@@ -3122,6 +3124,7 @@ struct wiphy {
 	u16 max_acl_mac_addrs;
 
 	u32 flags, regulatory_flags, features;
+	u8 ext_features[1];
 
 	u32 ap_sme_capa;
 
@@ -5049,6 +5052,28 @@ void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev,
  */
 void cfg80211_shutdown_all_interfaces(struct wiphy *wiphy);
 
+/**
+ * cfg80211_ext_feature_set - set the extended feature flag
+ *
+ * @wiphy: the wiphy to modify.
+ * @ftidx: extended feature bit index.
+ *
+ * The extended features are flagged in multiple bytes (see
+ * &struct wiphy.@ext_features)
+ */
+void cfg80211_ext_feature_set(struct wiphy *wiphy,
+			      enum nl80211_ext_feature_index ftidx);
+/**
+ * cfg80211_ext_feature_isset - check the extended feature flag
+ *
+ * @wiphy: the wiphy to modify.
+ * @ftidx: extended feature bit index.
+ *
+ * The extended features are flagged in multiple bytes (see
+ * &struct wiphy.@ext_features)
+ */
+bool cfg80211_ext_feature_isset(struct wiphy *wiphy,
+				enum nl80211_ext_feature_index ftidx);
 
 /* ethtool helper */
 void cfg80211_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info);
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index c0383e9..b4c4120 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1713,6 +1713,8 @@ enum nl80211_commands {
  *	obtained from it is coming from the device's wiphy and not the global
  *	cfg80211 regdomain.
  *
+ * @NL80211_ATTR_EXT_FEATURES: extended feature flags (u8 array)
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2072,6 +2074,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_WIPHY_SELF_MANAGED_REG,
 
+	NL80211_ATTR_EXT_FEATURES,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -4221,6 +4225,15 @@ enum nl80211_feature_flags {
 };
 
 /**
+ * enum nl80211_ext_feature_index - bit index of extended features.
+ *
+ * @NL80211_EXT_FEATURE_4WAY_HANDSHAKE: the device supports 4way handshake
+ */
+enum nl80211_ext_feature_index {
+	NL80211_EXT_FEATURE_4WAY_HANDSHAKE,
+};
+
+/**
  * enum nl80211_probe_resp_offload_support_attr - optional supported
  *	protocols for probe-response offloading by the driver/FW.
  *	To be used with the %NL80211_ATTR_PROBE_RESP_OFFLOAD attribute.
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 7029201..7c00577 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1603,6 +1603,11 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		if (nla_put_u32(msg, NL80211_ATTR_FEATURE_FLAGS, features))
 			goto nla_put_failure;
 
+		if (nla_put(msg, NL80211_ATTR_EXT_FEATURES,
+			    sizeof(rdev->wiphy.ext_features),
+			    rdev->wiphy.ext_features))
+			goto nla_put_failure;
+
 		if (rdev->wiphy.ht_capa_mod_mask &&
 		    nla_put(msg, NL80211_ATTR_HT_CAPABILITY_MASK,
 			    sizeof(*rdev->wiphy.ht_capa_mod_mask),
diff --git a/net/wireless/util.c b/net/wireless/util.c
index d0ac795..af8d0dd 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1568,6 +1568,26 @@ int cfg80211_get_station(struct net_device *dev, const u8 *mac_addr,
 }
 EXPORT_SYMBOL(cfg80211_get_station);
 
+void cfg80211_ext_feature_set(struct wiphy *wiphy,
+			      enum nl80211_ext_feature_index ftidx)
+{
+	u8 *ft_byte;
+
+	ft_byte = &wiphy->ext_features[ftidx / 8];
+	*ft_byte |= BIT(ftidx % 8);
+}
+EXPORT_SYMBOL(cfg80211_ext_feature_set);
+
+bool cfg80211_ext_feature_isset(struct wiphy *wiphy,
+				enum nl80211_ext_feature_index ftidx)
+{
+	u8 ft_byte;
+
+	ft_byte = wiphy->ext_features[ftidx / 8];
+	return (ft_byte & BIT(ftidx % 8)) != 0;
+}
+EXPORT_SYMBOL(cfg80211_ext_feature_isset);
+
 /* See IEEE 802.1H for LLC/SNAP encapsulation/decapsulation */
 /* Ethernet-II snap header (RFC1042 for most EtherTypes) */
 const unsigned char rfc1042_header[] __aligned(2) =
-- 
1.9.1

Re: [RFC] cfg80211: Add feature flag for 4-way handshake offload

[Johannes Berg]

On Thu, 2014-12-18 at 13:51 +0100, Arend van Spriel wrote:

> Here the proposed way to deal with new feature flags. Let
> me know if this is suitable.

I guess that works. I think those functions should be inlines - no point
in having them with export symbol etc (the structures needed for that
are *far* bigger than the functions.)

johannes

Re: [RFC] cfg80211: Add feature flag for 4-way handshake offload

[Eliad Peller]

hi,

On Thu, Dec 18, 2014 at 2:51 PM, Arend van Spriel <arend@broadcom.com> wrote:
> From: Gautam Kumar Shukla <gautams@broadcom.com>
>
> The new feature flag allows the driver to indicate that it can
> offload the 4-way handshake for WPA/RSN-PSK. With the
> wiphy::features flag being used up this patch adds a new
> field wiphy::ext_features. Considering extensibility this
> new field is declared as a byte array.
>
> Signed-off-by: Gautam (Gautam Kumar) Shukla <gautams@broadcom.com>
> Signed-off-by: Arend van Spriel <arend@broadcom.com>
> ---
> Hi Johannes,
>
> Here the proposed way to deal with new feature flags. Let
> me know if this is suitable.
>
> Regards,
> Arend
> ---

> @@ -3122,6 +3124,7 @@ struct wiphy {
>         u16 max_acl_mac_addrs;
>
>         u32 flags, regulatory_flags, features;
> +       u8 ext_features[1];
>
i think it would be nicer to use unsigned long (instead of u8) along
with BITS_TO_LONGS

>
>  /**
> + * enum nl80211_ext_feature_index - bit index of extended features.
> + *
> + * @NL80211_EXT_FEATURE_4WAY_HANDSHAKE: the device supports 4way handshake
> + */
> +enum nl80211_ext_feature_index {
> +       NL80211_EXT_FEATURE_4WAY_HANDSHAKE,
> +};

just add the standard LAST/MAX defines here, and the bitmap size will
be allocated automatically.

>
> +void cfg80211_ext_feature_set(struct wiphy *wiphy,
> +                             enum nl80211_ext_feature_index ftidx)
> +{
> +       u8 *ft_byte;
> +
> +       ft_byte = &wiphy->ext_features[ftidx / 8];
> +       *ft_byte |= BIT(ftidx % 8);
> +}
> +EXPORT_SYMBOL(cfg80211_ext_feature_set);
> +
> +bool cfg80211_ext_feature_isset(struct wiphy *wiphy,
> +                               enum nl80211_ext_feature_index ftidx)
> +{
> +       u8 ft_byte;
> +
> +       ft_byte = wiphy->ext_features[ftidx / 8];
> +       return (ft_byte & BIT(ftidx % 8)) != 0;
> +}
> +EXPORT_SYMBOL(cfg80211_ext_feature_isset);

and these (or only the implementations) could be replaced by set_bit/test_bit.

Eliad.

Re: [RFC] cfg80211: Add feature flag for 4-way handshake offload

[Johannes Berg]

On Thu, 2014-12-18 at 15:44 +0200, Eliad Peller wrote:
> >         u32 flags, regulatory_flags, features;
> > +       u8 ext_features[1];
> >
> i think it would be nicer to use unsigned long (instead of u8) along
> with BITS_TO_LONGS

That works in the kernel, but not in the userspace API. We can do it on
the kernel side, but then we have to write a translation loop in
nl80211. I'm not sure that's worth it.

> >  /**
> > + * enum nl80211_ext_feature_index - bit index of extended features.
> > + *
> > + * @NL80211_EXT_FEATURE_4WAY_HANDSHAKE: the device supports 4way handshake
> > + */
> > +enum nl80211_ext_feature_index {
> > +       NL80211_EXT_FEATURE_4WAY_HANDSHAKE,
> > +};
> 
> just add the standard LAST/MAX defines here, and the bitmap size will
> be allocated automatically.

If you add a max here then we just have to hope that in userspace nobody
abuses it ... but I guess we can and should do that to size the flags
array properly automatically. The problem is that if somebody tries to
use it in userspace for parsing, like
  u8 eflags[DIV_ROUND_UP(FEATURE_MAX, 8)];
  memcpy(eflags, nla_data(attr), nla_len(attr));

then surely that will cause memory corruption... so we should probably
add a note or something there.

> and these (or only the implementations) could be replaced by set_bit/test_bit.

They could - but as I said above then we have to translate this to
userspace. The problem with the unsigned long array thing is that the
size of unsigned long varies on different platforms (32 vs 64 bit) and
not all platforms are little endian where that wouldn't matter ...

Maybe the better choice would be to create u8 bitmap helper functions -
after all we already have them in at least 2 other places in wifi:
 * TIM IE
 * extended capabilities IE

Probably there are others elsewhere?

johannes

Re: [RFC] cfg80211: Add feature flag for 4-way handshake offload

[Arend van Spriel]

On 12/18/14 14:44, Eliad Peller wrote:
> hi,
>
> On Thu, Dec 18, 2014 at 2:51 PM, Arend van Spriel<arend@broadcom.com>  wrote:
>> From: Gautam Kumar Shukla<gautams@broadcom.com>
>>
>> The new feature flag allows the driver to indicate that it can
>> offload the 4-way handshake for WPA/RSN-PSK. With the
>> wiphy::features flag being used up this patch adds a new
>> field wiphy::ext_features. Considering extensibility this
>> new field is declared as a byte array.
>>
>> Signed-off-by: Gautam (Gautam Kumar) Shukla<gautams@broadcom.com>
>> Signed-off-by: Arend van Spriel<arend@broadcom.com>
>> ---
>> Hi Johannes,
>>
>> Here the proposed way to deal with new feature flags. Let
>> me know if this is suitable.
>>
>> Regards,
>> Arend
>> ---
>
>> @@ -3122,6 +3124,7 @@ struct wiphy {
>>          u16 max_acl_mac_addrs;
>>
>>          u32 flags, regulatory_flags, features;
>> +       u8 ext_features[1];
>>
> i think it would be nicer to use unsigned long (instead of u8) along
> with BITS_TO_LONGS

Thanks, Eliad

I considered that, but decided against it. The main reason for me was 
the fact that this is passed in nl80211 attribute and using u8 seemed 
more straightforward to me. That said I think I will need to describe in 
nl80211 how the bits are ordered in the byte-array.

>>
>>   /**
>> + * enum nl80211_ext_feature_index - bit index of extended features.
>> + *
>> + * @NL80211_EXT_FEATURE_4WAY_HANDSHAKE: the device supports 4way handshake
>> + */
>> +enum nl80211_ext_feature_index {
>> +       NL80211_EXT_FEATURE_4WAY_HANDSHAKE,
>> +};
>
> just add the standard LAST/MAX defines here, and the bitmap size will
> be allocated automatically.

I thought that was only for attribute enumerations. This is a bit index 
within a attribute.

>>
>> +void cfg80211_ext_feature_set(struct wiphy *wiphy,
>> +                             enum nl80211_ext_feature_index ftidx)
>> +{
>> +       u8 *ft_byte;
>> +
>> +       ft_byte =&wiphy->ext_features[ftidx / 8];
>> +       *ft_byte |= BIT(ftidx % 8);
>> +}
>> +EXPORT_SYMBOL(cfg80211_ext_feature_set);
>> +
>> +bool cfg80211_ext_feature_isset(struct wiphy *wiphy,
>> +                               enum nl80211_ext_feature_index ftidx)
>> +{
>> +       u8 ft_byte;
>> +
>> +       ft_byte = wiphy->ext_features[ftidx / 8];
>> +       return (ft_byte&  BIT(ftidx % 8)) != 0;
>> +}
>> +EXPORT_SYMBOL(cfg80211_ext_feature_isset);
>
> and these (or only the implementations) could be replaced by set_bit/test_bit.

See above.

Regards,
Arend

> Eliad.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] cfg80211: Add feature flag for 4-way handshake offload

[Eliad Peller]

hi Arend,

On Thu, Dec 18, 2014 at 5:21 PM, Arend van Spriel <arend@broadcom.com> wrote:
> On 12/18/14 14:44, Eliad Peller wrote:
>> On Thu, Dec 18, 2014 at 2:51 PM, Arend van Spriel<arend@broadcom.com>
>> wrote:
>>>
>>> From: Gautam Kumar Shukla<gautams@broadcom.com>
>>>
>>> The new feature flag allows the driver to indicate that it can
>>> offload the 4-way handshake for WPA/RSN-PSK. With the
>>> wiphy::features flag being used up this patch adds a new
>>> field wiphy::ext_features. Considering extensibility this
>>> new field is declared as a byte array.
>>>
>>> Signed-off-by: Gautam (Gautam Kumar) Shukla<gautams@broadcom.com>
>>> Signed-off-by: Arend van Spriel<arend@broadcom.com>
>>> ---
>>> Hi Johannes,
>>>
>>> Here the proposed way to deal with new feature flags. Let
>>> me know if this is suitable.
>>>
>>> Regards,
>>> Arend
>>> ---
>>
>>
>>> @@ -3122,6 +3124,7 @@ struct wiphy {
>>>          u16 max_acl_mac_addrs;
>>>
>>>          u32 flags, regulatory_flags, features;
>>> +       u8 ext_features[1];
>>>
>> i think it would be nicer to use unsigned long (instead of u8) along
>> with BITS_TO_LONGS
>
>
> Thanks, Eliad
>
> I considered that, but decided against it. The main reason for me was the
> fact that this is passed in nl80211 attribute and using u8 seemed more
> straightforward to me. That said I think I will need to describe in nl80211
> how the bits are ordered in the byte-array.
>
usually bitmaps are implemented by unsigned long array, but i didn't
consider that in this case the array is passed to userspace as well
(as you and Johannes pointed out).
adding a conversion function to nl80211 can do the trick, but i guess
simply using u8 instead in this case makes sense :)

>>>
>>>   /**
>>> + * enum nl80211_ext_feature_index - bit index of extended features.
>>> + *
>>> + * @NL80211_EXT_FEATURE_4WAY_HANDSHAKE: the device supports 4way
>>> handshake
>>> + */
>>> +enum nl80211_ext_feature_index {
>>> +       NL80211_EXT_FEATURE_4WAY_HANDSHAKE,
>>> +};
>>
>>
>> just add the standard LAST/MAX defines here, and the bitmap size will
>> be allocated automatically.
>
>
> I thought that was only for attribute enumerations. This is a bit index
> within a attribute.
>
it's usually not needed, but in this case it will allow defining
ext_features with proper length automatically.

Eliad.

Re: [RFC] cfg80211: Add feature flag for 4-way handshake offload

[Arend van Spriel]

On 12/18/14 14:31, Johannes Berg wrote:
> On Thu, 2014-12-18 at 13:51 +0100, Arend van Spriel wrote:
>
>> Here the proposed way to deal with new feature flags. Let
>> me know if this is suitable.
>
> I guess that works. I think those functions should be inlines - no point
> in having them with export symbol etc (the structures needed for that
> are *far* bigger than the functions.)

Ok. I should do some self-learning about the things going on behind the 
scene called EXPORT_SYMBOL. Thanks.

Regards,
Arend