diff mbox

[RFC] cfg80211: add control port state to struct cfg80211_connect_resp_params

Message ID 1492808507-31224-1-git-send-email-arend.vanspriel@broadcom.com (mailing list archive)
State RFC
Delegated to: Johannes Berg
Headers show

Commit Message

Arend van Spriel April 21, 2017, 9:01 p.m. UTC
When the driver supports offloading of the PTK/GTK handshakes
completion of that during connect changes the layer 2 control
port state to authorized. This patch allows the driver to pass
that state in cfg80211_connect_done() resulting in adding the
new flag NL80211_ATTR_PORT_AUTHORIZED in the NL80211_CMD_CONNECT
notification.

Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
Hi Johannes, Jouni, et al

I have been working on 4-way handshake offloading and one of the
things discussed was the addition of PORT_AUTHORIZED flag. So
this is what I came up with, but I suppose wpa_supplicant wants
to know whether it can expect this attribute or not. One option
is to have PORT_UNAUTHORIZED flag instead. Another option would
be introducing it as nl80211 protocol feature although not sure
if it could be considered as such. What do you guys think?

Regards,
Arend
---
 include/net/cfg80211.h       | 9 +++++++++
 include/uapi/linux/nl80211.h | 3 +++
 net/wireless/nl80211.c       | 2 ++
 net/wireless/sme.c           | 1 +
 4 files changed, 15 insertions(+)

Comments

Johannes Berg April 25, 2017, 2:40 p.m. UTC | #1
On Fri, 2017-04-21 at 22:01 +0100, Arend van Spriel wrote:

> I have been working on 4-way handshake offloading and one of the
> things discussed was the addition of PORT_AUTHORIZED flag. So
> this is what I came up with, but I suppose wpa_supplicant wants
> to know whether it can expect this attribute or not. One option
> is to have PORT_UNAUTHORIZED flag instead. Another option would
> be introducing it as nl80211 protocol feature although not sure
> if it could be considered as such. What do you guys think?

I think it could be, but I'm not really sure it matters?

> +	    (cr->port_state != CONTROL_PORT_STATE_UNAUTHORIZED &&
> +	     nla_put_flag(msg, NL80211_ATTR_PORT_AUTHORIZED)) ||
>  	    (cr->req_ie &&
> 
This doesn't really make sense - why does unspecified equal authorized?

johannes
Arend van Spriel April 25, 2017, 6:34 p.m. UTC | #2
On 25-4-2017 16:40, Johannes Berg wrote:
> On Fri, 2017-04-21 at 22:01 +0100, Arend van Spriel wrote:
> 
>> I have been working on 4-way handshake offloading and one of the
>> things discussed was the addition of PORT_AUTHORIZED flag. So
>> this is what I came up with, but I suppose wpa_supplicant wants
>> to know whether it can expect this attribute or not. One option
>> is to have PORT_UNAUTHORIZED flag instead. Another option would
>> be introducing it as nl80211 protocol feature although not sure
>> if it could be considered as such. What do you guys think?
> 
> I think it could be, but I'm not really sure it matters?
> 
>> +	    (cr->port_state != CONTROL_PORT_STATE_UNAUTHORIZED &&
>> +	     nla_put_flag(msg, NL80211_ATTR_PORT_AUTHORIZED)) ||
>>  	    (cr->req_ie &&
>>
> This doesn't really make sense - why does unspecified equal authorized?

I was considering default behavior here for drivers that do not provide
this information, ie. drivers not supporting 4-way handshake offload. So
wpa_supplicant just looks for the PORT_AUTHORIZED attribute and deals
with it without need for checking 4-way handshake offload is supported.

Regards,
Arend
Johannes Berg April 25, 2017, 6:36 p.m. UTC | #3
On Tue, 2017-04-25 at 20:34 +0200, Arend Van Spriel wrote:

> > > +	    (cr->port_state != CONTROL_PORT_STATE_UNAUTHORIZED
> > > &&
> > > +	     nla_put_flag(msg, NL80211_ATTR_PORT_AUTHORIZED)) ||
> > >  	    (cr->req_ie &&
> > > 
> > 
> > This doesn't really make sense - why does unspecified equal
> > authorized?
> 
> I was considering default behavior here for drivers that do not
> provide this information, ie. drivers not supporting 4-way handshake
> offload. So wpa_supplicant just looks for the PORT_AUTHORIZED
> attribute and deals with it without need for checking 4-way handshake
> offload is supported.

There are two such cases - the driver is old and doesn't provide it,
but of course once you see the attribute you know that's not the case.
And the case that the driver doesn't support 4-way-HS.

Can you really distinguish these though if you *don't* see the
attribute?

But anyway, if it's like mac80211 not providing the data at all, then
it would say authorized, which seems wrong since that's clearly not the
case for mac80211?

Or maybe I'm just confused.

johannes
Arend van Spriel April 25, 2017, 6:56 p.m. UTC | #4
On 25-4-2017 20:36, Johannes Berg wrote:
> On Tue, 2017-04-25 at 20:34 +0200, Arend Van Spriel wrote:
> 
>>>> +	    (cr->port_state != CONTROL_PORT_STATE_UNAUTHORIZED
>>>> &&
>>>> +	     nla_put_flag(msg, NL80211_ATTR_PORT_AUTHORIZED)) ||
>>>>  	    (cr->req_ie &&
>>>>
>>>
>>> This doesn't really make sense - why does unspecified equal
>>> authorized?
>>
>> I was considering default behavior here for drivers that do not
>> provide this information, ie. drivers not supporting 4-way handshake
>> offload. So wpa_supplicant just looks for the PORT_AUTHORIZED
>> attribute and deals with it without need for checking 4-way handshake
>> offload is supported.
> 
> There are two such cases - the driver is old and doesn't provide it,
> but of course once you see the attribute you know that's not the case.
> And the case that the driver doesn't support 4-way-HS.
> 
> Can you really distinguish these though if you *don't* see the
> attribute?
> 
> But anyway, if it's like mac80211 not providing the data at all, then
> it would say authorized, which seems wrong since that's clearly not the
> case for mac80211?
> 
> Or maybe I'm just confused.

You might, but not about this ;-) The other approach I had in mind is to
only pass the flag for drivers with 4-way-hs support. In that case
wpa_supp also has to check that to determine whether the flag should be
taken into account. Assuming the driver supporting 4-way-hs can provide
the port state info. Otherwise, a new ext_feature flag would be needed.

Regards,
Arend
Johannes Berg April 26, 2017, 7:20 a.m. UTC | #5
On Tue, 2017-04-25 at 20:56 +0200, Arend Van Spriel wrote:
> 
> You might, but not about this ;-) The other approach I had in mind is
> to only pass the flag for drivers with 4-way-hs support. In that case
> wpa_supp also has to check that to determine whether the flag should
> be taken into account. Assuming the driver supporting 4-way-hs can
> provide the port state info. Otherwise, a new ext_feature flag would
> be needed.

I think it's reasonable to assume 4-way-HS offload drivers can support
it.

johannes
Arend van Spriel April 26, 2017, 6:46 p.m. UTC | #6
On 26-4-2017 9:20, Johannes Berg wrote:
> On Tue, 2017-04-25 at 20:56 +0200, Arend Van Spriel wrote:
>>
>> You might, but not about this ;-) The other approach I had in mind is
>> to only pass the flag for drivers with 4-way-hs support. In that case
>> wpa_supp also has to check that to determine whether the flag should
>> be taken into account. Assuming the driver supporting 4-way-hs can
>> provide the port state info. Otherwise, a new ext_feature flag would
>> be needed.
> 
> I think it's reasonable to assume 4-way-HS offload drivers can support
> it.

I tested the 4-way-hs (both Personal and 802.1X) with boolean parameter
similar to what is proposed in the patch for roaming "cfg80211/nl80211:
add authorized flag to roaming event" and it works fine. So I can extend
it for use in connect result. Just had one issue regarding the type, ie.
flag vs. u8 because of how things are done in wpa_supplicant supporting
QCA roam+auth vendor-specific event.

Regards,
Arend
Johannes Berg April 28, 2017, 12:02 p.m. UTC | #7
> I tested the 4-way-hs (both Personal and 802.1X) with boolean
> parameter similar to what is proposed in the patch for roaming
> "cfg80211/nl80211: add authorized flag to roaming event" and it works
> fine. So I can extend it for use in connect result. Just had one
> issue regarding the type, ie. flag vs. u8 because of how things are
> done in wpa_supplicant supporting QCA roam+auth vendor-specific
> event.

Ok.

We have lots of related but somewhat conflicting things in flight right
now, and we need somebody to go collect them all into a consistent
patchset, preferably even with the 4-way-HS offload [1] thrown in. That
needs rebasing anyway because it introduces the _PMK attribute that
Jouni already added for FILS now.

Do you want to pick that up? I probably can't do it soon - I'm likely
busy with other things the next week or two, then travel again, ...

johannes

[1] https://patchwork.kernel.org/patch/9584201/
Arend van Spriel April 28, 2017, 12:46 p.m. UTC | #8
On 28-4-2017 14:02, Johannes Berg wrote:
> 
>> I tested the 4-way-hs (both Personal and 802.1X) with boolean
>> parameter similar to what is proposed in the patch for roaming
>> "cfg80211/nl80211: add authorized flag to roaming event" and it works
>> fine. So I can extend it for use in connect result. Just had one
>> issue regarding the type, ie. flag vs. u8 because of how things are
>> done in wpa_supplicant supporting QCA roam+auth vendor-specific
>> event.
> 
> Ok.
> 
> We have lots of related but somewhat conflicting things in flight right
> now, and we need somebody to go collect them all into a consistent
> patchset, preferably even with the 4-way-HS offload [1] thrown in. That
> needs rebasing anyway because it introduces the _PMK attribute that
> Jouni already added for FILS now.

I already have the 4-way-hs stuff rebased with modified ATTR_PMK
description and additional length check in nl80211 for it. Jouni
commented on that already in 4-way-hs patch.

> Do you want to pick that up? I probably can't do it soon - I'm likely
> busy with other things the next week or two, then travel again, ...

That is my plan.

Regards,
Arend
Johannes Berg April 28, 2017, 12:53 p.m. UTC | #9
On Fri, 2017-04-28 at 14:46 +0200, Arend Van Spriel wrote:

> I already have the 4-way-hs stuff rebased with modified ATTR_PMK
> description and additional length check in nl80211 for it. Jouni
> commented on that already in 4-way-hs patch.

Cool.

> > Do you want to pick that up? I probably can't do it soon - I'm
> > likely busy with other things the next week or two, then travel
> > again, ...
> 
> That is my plan.

Awesome, thanks!

johannes
diff mbox

Patch

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index bdc4424..f416d55 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5236,6 +5236,12 @@  static inline void cfg80211_testmode_event(struct sk_buff *skb, gfp_t gfp)
 #define CFG80211_TESTMODE_DUMP(cmd)
 #endif
 
+enum cfg80211_control_port_state {
+	CONTROL_PORT_STATE_UNSPECIFIED,
+	CONTROL_PORT_STATE_UNAUTHORIZED,
+	CONTROL_PORT_STATE_AUTHORIZED
+};
+
 /**
  * struct cfg80211_connect_resp_params - Connection response params
  * @status: Status code, %WLAN_STATUS_SUCCESS for successful connection, use
@@ -5271,6 +5277,8 @@  static inline void cfg80211_testmode_event(struct sk_buff *skb, gfp_t gfp)
  *	not known. This value is used only if @status < 0 to indicate that the
  *	failure is due to a timeout and not due to explicit rejection by the AP.
  *	This value is ignored in other cases (@status >= 0).
+ * @port_state: Indicates whether the connection is ready to transport
+ *	data packets (see &enum cfg80211_control_port_state).
  */
 struct cfg80211_connect_resp_params {
 	int status;
@@ -5288,6 +5296,7 @@  struct cfg80211_connect_resp_params {
 	size_t pmk_len;
 	const u8 *pmkid;
 	enum nl80211_timeout_reason timeout_reason;
+	enum cfg80211_control_port_state port_state;
 };
 
 /**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 087493d..34738df 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2106,6 +2106,8 @@  enum nl80211_commands {
  *	in %NL80211_CMD_CONNECT to indicate that for 802.1X authentication it
  *	wants to use the supported offload.
  * @NL80211_ATTR_PMKR0_NAME: PMK-R0 Name for offloaded FT.
+ * @NL80211_ATTR_PORT_AUTHORIZED: flag attribute used in %NL80211_CMD_CONNECT
+ *	notification indicating 4-way handshake offload finished successfully.
  *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
@@ -2531,6 +2533,7 @@  enum nl80211_attrs {
 
 	NL80211_ATTR_WANT_1X_OFFLOAD,
 	NL80211_ATTR_PMKR0_NAME,
+	NL80211_ATTR_PORT_AUTHORIZED,
 
 	/* add attributes here, update the policy in nl80211.c */
 
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index e08c0d3..7fff668 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -13745,6 +13745,8 @@  void nl80211_send_connect_result(struct cfg80211_registered_device *rdev,
 	     (nla_put_flag(msg, NL80211_ATTR_TIMED_OUT) ||
 	      nla_put_u32(msg, NL80211_ATTR_TIMEOUT_REASON,
 			  cr->timeout_reason))) ||
+	    (cr->port_state != CONTROL_PORT_STATE_UNAUTHORIZED &&
+	     nla_put_flag(msg, NL80211_ATTR_PORT_AUTHORIZED)) ||
 	    (cr->req_ie &&
 	     nla_put(msg, NL80211_ATTR_REQ_IE, cr->req_ie_len, cr->req_ie)) ||
 	    (cr->resp_ie &&
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 6459bb7..a0d4010 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -860,6 +860,7 @@  void cfg80211_connect_done(struct net_device *dev,
 	ev->cr.bss = params->bss;
 	ev->cr.status = params->status;
 	ev->cr.timeout_reason = params->timeout_reason;
+	ev->cr.port_state = params->port_state;
 
 	spin_lock_irqsave(&wdev->event_lock, flags);
 	list_add_tail(&ev->list, &wdev->event_list);