| Message ID | 1497010130-16186-4-git-send-email-arend.vanspriel@broadcom.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Johannes Berg |
| Headers | show |
On Fri, 2017-06-09 at 13:08 +0100, Arend van Spriel wrote: > 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. For 802.1X the driver is expected to issue the > NL80211_CMD_CONNECT notification twice. First without the new > attribute to inform user-space that 802.1X authentication can > start and a second one with the new attribute to indicate the > completion of the handshakes. So I was going to apply this, but now I'm no longer sure. First of all, I'm concerned it could confuse older tools that don't expect to see the notification twice. OTOH, they'd get the connected notification now and not really know whether or not data can be passed, so it seems unlikely they'd rely on it - but are we really sure about that? Additionally, I just discussed this with Avi, and it's not clear to us what the second notification is actually useful for? If you were going to say maybe it makes sense for when you can't send ROAMED, but still connected again to the same network so you've already done the 4-way-HS at the time of the first notification - that I think makes sense. But it seems that wpa_s moves all state machines to COMPLETE when it sends the PMK to the device for offload, so it won't really care about when that finished? However, I think this might actually be a BAD thing, because it would leave higher layers trying to do DHCP when the link isn't actually ready yet. (This, btw, is another problem we can solve with the EAPOL-over-nl80211 since then we can set the carrier later when the port is authorized.) So - I have no objections to this patch, but I think the double notification needs to be revised, and if needed a separate notification for "4-way-HS is done now" can be added. johannes
On 6/13/2017 11:02 AM, Johannes Berg wrote: > On Fri, 2017-06-09 at 13:08 +0100, Arend van Spriel wrote: >> 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. For 802.1X the driver is expected to issue the >> NL80211_CMD_CONNECT notification twice. First without the new >> attribute to inform user-space that 802.1X authentication can >> start and a second one with the new attribute to indicate the >> completion of the handshakes. > > So I was going to apply this, but now I'm no longer sure. Not sure if this means you can be convinced, but let me try anyway ;-) > First of all, I'm concerned it could confuse older tools that don't > expect to see the notification twice. OTOH, they'd get the connected > notification now and not really know whether or not data can be passed, > so it seems unlikely they'd rely on it - but are we really sure about > that? Not sure what should be considered under the term "older tools". For what it is worth, in wpa_supplicant both nl80211 CONNECT and ROAMED event result in wpa_supplicant ASSOC event handled in wpa_supplicant/events.c. This event already deals with authorized flag to support QCA vendor stuff which typed it as u8 attribute iso flag. > Additionally, I just discussed this with Avi, and it's not clear to us > what the second notification is actually useful for? If you were going > to say maybe it makes sense for when you can't send ROAMED, but still > connected again to the same network so you've already done the 4-way-HS > at the time of the first notification - that I think makes sense. > > But it seems that wpa_s moves all state machines to COMPLETE when it > sends the PMK to the device for offload, so it won't really care about > when that finished? > > However, I think this might actually be a BAD thing, because it would > leave higher layers trying to do DHCP when the link isn't actually > ready yet. Indeed. It introduces a window where the link is not ready. When I stumbled on that in wpa_s it made sense to me to have the second notification. > (This, btw, is another problem we can solve with the EAPOL-over-nl80211 > since then we can set the carrier later when the port is authorized.) Agree. I did see that thread. However I kinda missed the whole EAPOL-over-nl80211 idea. Was that result of summit discussion? > So - I have no objections to this patch, but I think the double > notification needs to be revised, and if needed a separate notification > for "4-way-HS is done now" can be added. The separate notification was suggested, but as you guys already added a flag in the ROAMED event we decided to do the same for CONNECT event. If you still have concerns about the double CONNECT we can revisit that or leave it for now and deal with it through carrier state with EAPOL-over-nl80211. Regards, Arend
> > So I was going to apply this, but now I'm no longer sure. > > Not sure if this means you can be convinced, but let me try anyway ;-) :-) > > First of all, I'm concerned it could confuse older tools that don't > > expect to see the notification twice. OTOH, they'd get the > > connected > > notification now and not really know whether or not data can be > > passed, > > so it seems unlikely they'd rely on it - but are we really sure > > about > > that? > > Not sure what should be considered under the term "older tools". For > what it is worth, in wpa_supplicant both nl80211 CONNECT and ROAMED > event result in wpa_supplicant ASSOC event handled in > wpa_supplicant/events.c. This event already deals with authorized > flag to support QCA vendor stuff which typed it as u8 attribute iso > flag. Yeah, I guess wpa_s would deal with it just fine. I'm a little concerned there might be other tools, and we'd only find out in a long time when all of this comes together for enough users. OTOH, most tools that aren't already quite broken will hopefully use wpa_supplicant events. > > But it seems that wpa_s moves all state machines to COMPLETE when > > it sends the PMK to the device for offload, so it won't really care > > about when that finished? > > > > However, I think this might actually be a BAD thing, because it > > would leave higher layers trying to do DHCP when the link isn't > > actually ready yet. > > Indeed. It introduces a window where the link is not ready. When I > stumbled on that in wpa_s it made sense to me to have the second > notification. Right. > > (This, btw, is another problem we can solve with the EAPOL-over- > > nl80211 since then we can set the carrier later when the port is > > authorized.) > > Agree. I did see that thread. However I kinda missed the whole > EAPOL-over-nl80211 idea. Was that result of summit discussion? Yeah it came up a few times and we finally decided it's worth doing, but so far nobody had stepped up yet. Looks like Denis decided it might be worth doing. > > So - I have no objections to this patch, but I think the double > > notification needs to be revised, and if needed a separate > > notification for "4-way-HS is done now" can be added. > > The separate notification was suggested, but as you guys already > added a flag in the ROAMED event we decided to do the same for > CONNECT event. If you still have concerns about the double CONNECT we > can revisit that or leave it for now and deal with it through carrier > state with EAPOL-over-nl80211. For ROAMED the case is clearer: it can either happen with or without the attribute, but it'll never happen twice because of the attribute. Also, if the attribute is ignored by some tool, it's pretty harmless since it'll still know that it was roaming, it just won't know that no EAP authentication is needed. I think I'd be OK with adding a new notification, but EAPOL-over- nl80211 seems like it'd solve this, and then we don't need the extra notification since we can just set the carrier after it's all complete... I don't really know. If we can wait a bit for this, I think it'd be the nicer thing, and it looks like the currently existing code has the race and is still working OK for people. johannes
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 2174e51..7097310 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -5280,6 +5280,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). + * @authorized: Indicates whether the connection is ready to transport + * data packets. */ struct cfg80211_connect_resp_params { int status; @@ -5297,6 +5299,7 @@ struct cfg80211_connect_resp_params { size_t pmk_len; const u8 *pmkid; enum nl80211_timeout_reason timeout_reason; + bool authorized; }; /** diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index 2f49275..6f139a3 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -2130,6 +2130,9 @@ enum nl80211_commands { * in %NL80211_CMD_CONNECT to indicate that for 802.1X authentication it * wants to use the supported offload of the 4-way handshake. * @NL80211_ATTR_PMKR0_NAME: PMK-R0 Name for offloaded FT. + * @NL80211_ATTR_PORT_AUTHORIZED: flag attribute used in %NL80211_CMD_CONNECT + * notification indicating that PTK/GTK handshakes for 802.1X have been + * completed successfully. * * @NUM_NL80211_ATTR: total number of nl80211_attrs available * @NL80211_ATTR_MAX: highest attribute number currently defined @@ -2555,6 +2558,7 @@ enum nl80211_attrs { NL80211_ATTR_WANT_1X_4WAY_HS, 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 8148b01..d0bf6bd 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->authorized && + 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 532a000..a46eda8 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c @@ -861,6 +861,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.authorized = params->authorized; spin_lock_irqsave(&wdev->event_lock, flags); list_add_tail(&ev->list, &wdev->event_list);
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. For 802.1X the driver is expected to issue the NL80211_CMD_CONNECT notification twice. First without the new attribute to inform user-space that 802.1X authentication can start and a second one with the new attribute to indicate the completion of the handshakes. Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> --- include/net/cfg80211.h | 3 +++ include/uapi/linux/nl80211.h | 4 ++++ net/wireless/nl80211.c | 2 ++ net/wireless/sme.c | 1 + 4 files changed, 10 insertions(+)