| Message ID | 20171228175832.3253-5-denkenz@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
On Thu, 2017-12-28 at 11:58 -0600, Denis Kenzior wrote: > This commit implements the TX side of NL80211_CMD_CONTROL_PORT_FRAME. > Userspace provides the raw EAPoL frame using NL80211_ATTR_FRAME. A > skbuf is built and then injected onto the netdev of the wireless device. > The CONTROL_PORT_ETHERTYPE_NO_ENCRYPT will still in theory be honored by You mean CONTROL_PORT_NO_ENCRYPT :-) > the underlying TX path code. I think this isn't good enough. There are cases where CONTROL_PORT_ETHERTYPE_NO_ENCRYPT should be unset, but specific frames still shouldn't be encrypted. So I think for this particular path it would be better to deprecate CONTROL_PORT_ETHERTYPE_NO_ENCRYPT entirely, and have a separate per- frame flag. That also means that we can't really implement it fully in cfg80211, but have to provide some functionality for the driver to do things to be able to honour such flags. Perhaps a new operation, where we pass a pre-built SKB and control flags? johannes
Hi Johannes, On 01/02/2018 07:30 AM, Johannes Berg wrote: > On Thu, 2017-12-28 at 11:58 -0600, Denis Kenzior wrote: >> This commit implements the TX side of NL80211_CMD_CONTROL_PORT_FRAME. >> Userspace provides the raw EAPoL frame using NL80211_ATTR_FRAME. A >> skbuf is built and then injected onto the netdev of the wireless device. >> The CONTROL_PORT_ETHERTYPE_NO_ENCRYPT will still in theory be honored by > > You mean CONTROL_PORT_NO_ENCRYPT :-) Right > >> the underlying TX path code. > > I think this isn't good enough. > That was my thinking as well > There are cases where CONTROL_PORT_ETHERTYPE_NO_ENCRYPT should be > unset, but specific frames still shouldn't be encrypted. > > So I think for this particular path it would be better to deprecate > CONTROL_PORT_ETHERTYPE_NO_ENCRYPT entirely, and have a separate per- > frame flag. > > That also means that we can't really implement it fully in cfg80211, > but have to provide some functionality for the driver to do things to > be able to honour such flags. Here's another thought I had while poking around. Given the above I don't want to pursue it too seriously unless you think it might work: We already have the IEEE80211_TX_INTFL_DONT_ENCRYPT flag on the skb and some drivers seem to honor this. At least that seems to be the intent as the CONTROL_PORT_NO_ENCRYPT flag ends up being translated to this somewhere in net/mac80211/tx.c. Are the drivers supposed to honor that flag? If so, can we do something like what ieee80211_process_sa_query_req in net/mac80211/rx.c or ieee80211_tdls_prep_mgmt_packet in net/mac80211/tdls.c do? E.g. use ieee80211_tx_skb or __ieee80211_subif_start_xmit or similar to inject the skb with the DONT_ENCRYPT flag? > > Perhaps a new operation, where we pass a pre-built SKB and control > flags? > This would likely mean that legacy behavior would still have to be supported for quite some time (forever?) for drivers that don't get around to implementing this, which would be unfortunate. Regards, -Denis
On Tue, 2018-01-02 at 12:22 -0600, Denis Kenzior wrote: > > There are cases where CONTROL_PORT_ETHERTYPE_NO_ENCRYPT should be > > unset, but specific frames still shouldn't be encrypted. > > > > So I think for this particular path it would be better to deprecate > > CONTROL_PORT_ETHERTYPE_NO_ENCRYPT entirely, and have a separate per- > > frame flag. > > > > That also means that we can't really implement it fully in cfg80211, > > but have to provide some functionality for the driver to do things to > > be able to honour such flags. > > Here's another thought I had while poking around. Given the above I > don't want to pursue it too seriously unless you think it might work: > > We already have the IEEE80211_TX_INTFL_DONT_ENCRYPT flag on the skb and > some drivers seem to honor this. At least that seems to be the intent > as the CONTROL_PORT_NO_ENCRYPT flag ends up being translated to this > somewhere in net/mac80211/tx.c. Are the drivers supposed to honor that > flag? Drivers don't have a choice, and don't need to check the flag. What happens internally in mac80211 is that either txinfo->control.key is assigned, or not, and drivers make encryption decisions based on that. > If so, can we do something like what ieee80211_process_sa_query_req in > net/mac80211/rx.c or ieee80211_tdls_prep_mgmt_packet in > net/mac80211/tdls.c do? E.g. use ieee80211_tx_skb or > __ieee80211_subif_start_xmit or similar to inject the skb with the > DONT_ENCRYPT flag? Yes, this will work - but like I said, it requires more control over the SKB than cfg80211 has today, and than you can get through the regular netdev xmit. > > Perhaps a new operation, where we pass a pre-built SKB and control > > flags? > > This would likely mean that legacy behavior would still have to be > supported for quite some time (forever?) for drivers that don't get > around to implementing this, which would be unfortunate. What do you mean by "legacy behaviour"? *Drivers* don't really need to do anything one way or the other, and mac80211 is the only thing implementing control port encrypt/ethertype right now, I suspect. johannes
Hi Johannes, <snip> >> If so, can we do something like what ieee80211_process_sa_query_req in >> net/mac80211/rx.c or ieee80211_tdls_prep_mgmt_packet in >> net/mac80211/tdls.c do? E.g. use ieee80211_tx_skb or >> __ieee80211_subif_start_xmit or similar to inject the skb with the >> DONT_ENCRYPT flag? > > Yes, this will work - but like I said, it requires more control over > the SKB than cfg80211 has today, and than you can get through the > regular netdev xmit. So, I'm a bit lost here. You're saying this will work but then it won't. Are you saying this would only work for mac80211 based devices (e.g. ones that implement ieee80211_ops) but not ones that implement cfg80211_ops? I presume because those provide their own net_device_ops? > >>> Perhaps a new operation, where we pass a pre-built SKB and control >>> flags? > >> >> This would likely mean that legacy behavior would still have to be >> supported for quite some time (forever?) for drivers that don't get >> around to implementing this, which would be unfortunate. > > What do you mean by "legacy behaviour"? *Drivers* don't really need to > do anything one way or the other, and mac80211 is the only thing > implementing control port encrypt/ethertype right now, I suspect. > When I say legacy I mean 'over netdev', e.g. not over nl80211. Okay, so what you're saying is that the current CONTROL_PORT_* stuff doesn't work on anything besides mac80211 since drivers can completely ignore stuff inside cfg80211_connect_params. And there's no way for userspace to know this ahead of time? So essentially we'd need a new operation in cfg80211_ops that would accept the control port frame data and some control flags. Do we want to pass in an skb with all the 802.11 headers set or a 802.3 formatted skb (since that is what other data frames look like initially on the netdev). Regards, -Denis
On Wed, Jan 3, 2018 at 6:17 PM, Denis Kenzior <denkenz@gmail.com> wrote: > Hi Johannes, > > <snip> > >>> If so, can we do something like what ieee80211_process_sa_query_req in >>> net/mac80211/rx.c or ieee80211_tdls_prep_mgmt_packet in >>> net/mac80211/tdls.c do? E.g. use ieee80211_tx_skb or >>> __ieee80211_subif_start_xmit or similar to inject the skb with the >>> DONT_ENCRYPT flag? >> >> >> Yes, this will work - but like I said, it requires more control over >> the SKB than cfg80211 has today, and than you can get through the >> regular netdev xmit. > > > So, I'm a bit lost here. You're saying this will work but then it won't. > Are you saying this would only work for mac80211 based devices (e.g. ones > that implement ieee80211_ops) but not ones that implement cfg80211_ops? I > presume because those provide their own net_device_ops? That is my assumption as well. >> >>>> Perhaps a new operation, where we pass a pre-built SKB and control >>>> flags? >> >> >>> >>> This would likely mean that legacy behavior would still have to be >>> supported for quite some time (forever?) for drivers that don't get >>> around to implementing this, which would be unfortunate. >> >> >> What do you mean by "legacy behaviour"? *Drivers* don't really need to >> do anything one way or the other, and mac80211 is the only thing >> implementing control port encrypt/ethertype right now, I suspect. >> > > When I say legacy I mean 'over netdev', e.g. not over nl80211. > > Okay, so what you're saying is that the current CONTROL_PORT_* stuff doesn't > work on anything besides mac80211 since drivers can completely ignore stuff > inside cfg80211_connect_params. And there's no way for userspace to know > this ahead of time? > > So essentially we'd need a new operation in cfg80211_ops that would accept > the control port frame data and some control flags. Do we want to pass in > an skb with all the 802.11 headers set or a 802.3 formatted skb (since that > is what other data frames look like initially on the netdev). Our firmware expects EAPOL stuff to come in as 802.3 packet, which probably applies to the other full-mac devices as well. Is there any reason for passing 802.11 packets. Regards, Arend
Hi, > > > If so, can we do something like what ieee80211_process_sa_query_req in > > > net/mac80211/rx.c or ieee80211_tdls_prep_mgmt_packet in > > > net/mac80211/tdls.c do? E.g. use ieee80211_tx_skb or > > > __ieee80211_subif_start_xmit or similar to inject the skb with the > > > DONT_ENCRYPT flag? > > > > Yes, this will work - but like I said, it requires more control over > > the SKB than cfg80211 has today, and than you can get through the > > regular netdev xmit. > > So, I'm a bit lost here. You're saying this will work but then it > won't. Are you saying this would only work for mac80211 based devices > (e.g. ones that implement ieee80211_ops) but not ones that implement > cfg80211_ops? I presume because those provide their own net_device_ops? Heh, yeah, badly phrased. It'll work to do it like process_sa_query or similar code, for mac80211 drivers. What will *not* work is actually building and submitting the SKB in cfg80211 though, since you can't even set the DONT_ENCRYPT flag from there. > > > This would likely mean that legacy behavior would still have to be > > > supported for quite some time (forever?) for drivers that don't get > > > around to implementing this, which would be unfortunate. > > > > What do you mean by "legacy behaviour"? *Drivers* don't really need to > > do anything one way or the other, and mac80211 is the only thing > > implementing control port encrypt/ethertype right now, I suspect. > > > > When I say legacy I mean 'over netdev', e.g. not over nl80211. We need to support that behaviour forever anyway, for existing versions of userspace software. > Okay, so what you're saying is that the current CONTROL_PORT_* stuff > doesn't work on anything besides mac80211 since drivers can completely > ignore stuff inside cfg80211_connect_params. And there's no way for > userspace to know this ahead of time? Yes, NL80211_ATTR_CONTROL_PORT_ETHERTYPE is an attribute set in the wiphy info - see WIPHY_FLAG_CONTROL_PORT_PROTOCOL in the code. > So essentially we'd need a new operation in cfg80211_ops that would > accept the control port frame data and some control flags. Do we want > to pass in an skb with all the 802.11 headers set or a 802.3 formatted > skb (since that is what other data frames look like initially on the > netdev). I think 802.3 format would be better - matches better for full-MAC devices that might do all header conversion in the device, and getting the BSSID might even be difficult from cfg80211, etc. johannes
Hi Arend, >> So essentially we'd need a new operation in cfg80211_ops that would accept >> the control port frame data and some control flags. Do we want to pass in >> an skb with all the 802.11 headers set or a 802.3 formatted skb (since that >> is what other data frames look like initially on the netdev). > > Our firmware expects EAPOL stuff to come in as 802.3 packet, which > probably applies to the other full-mac devices as well. Is there any > reason for passing 802.11 packets. > I think it was suggested earlier somewhere in this thread to use 802.11 header similar to how mgmt_tx does things. To me it makes more sense to use 802.3 headers so I thought I would double check. Regards, -Denis
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 220fe5bc57fd..d6191579f044 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -12464,6 +12464,64 @@ static int nl80211_del_pmk(struct sk_buff *skb, struct genl_info *info) return ret; } +static int nl80211_tx_control_port(struct sk_buff *skb, struct genl_info *info) +{ + struct wireless_dev *wdev = info->user_ptr[1]; + const u8 *buf; + u8 *dest; + size_t len; + struct ethhdr *ehdr; + int err; + + if (!info->attrs[NL80211_ATTR_FRAME]) + return -EINVAL; + + wdev_lock(wdev); + + switch (wdev->iftype) { + case NL80211_IFTYPE_STATION: + if (wdev->current_bss) + break; + err = -ENOTCONN; + goto out; + default: + err = -EOPNOTSUPP; + goto out; + } + + buf = nla_data(info->attrs[NL80211_ATTR_FRAME]); + len = nla_len(info->attrs[NL80211_ATTR_FRAME]); + + skb = dev_alloc_skb(sizeof(struct ethhdr) + len); + if (!skb) { + err = -ENOMEM; + goto out; + } + + skb_reserve(skb, sizeof(struct ethhdr)); + + dest = skb_put(skb, len); + memcpy(dest, buf, len); + + ehdr = skb_push(skb, sizeof(struct ethhdr)); + memcpy(ehdr->h_dest, wdev->current_bss->pub.bssid, ETH_ALEN); + memcpy(ehdr->h_source, wdev_address(wdev), ETH_ALEN); + ehdr->h_proto = cpu_to_be16(ETH_P_PAE); // TODO: How to get ethertype? + + wdev_unlock(wdev); + + skb->dev = wdev->netdev; + skb->protocol = htons(ETH_P_802_3); + skb_reset_network_header(skb); + skb_reset_mac_header(skb); + dev_queue_xmit(skb); + return 0; + + out: + wdev_unlock(wdev); + return err; +} + #define NL80211_FLAG_NEED_WIPHY 0x01 #define NL80211_FLAG_NEED_NETDEV 0x02 #define NL80211_FLAG_NEED_RTNL 0x04 @@ -13359,7 +13417,14 @@ static const struct genl_ops nl80211_ops[] = { .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | NL80211_FLAG_NEED_RTNL, }, - + { + .cmd = NL80211_CMD_CONTROL_PORT_FRAME, + .doit = nl80211_tx_control_port, + .policy = nl80211_policy, + .flags = GENL_UNS_ADMIN_PERM, + .internal_flags = NL80211_FLAG_NEED_WDEV_UP | + NL80211_FLAG_NEED_RTNL, + }, }; static struct genl_family nl80211_fam __ro_after_init = {
This commit implements the TX side of NL80211_CMD_CONTROL_PORT_FRAME. Userspace provides the raw EAPoL frame using NL80211_ATTR_FRAME. A skbuf is built and then injected onto the netdev of the wireless device. The CONTROL_PORT_ETHERTYPE_NO_ENCRYPT will still in theory be honored by the underlying TX path code. Signed-off-by: Denis Kenzior <denkenz@gmail.com> --- net/wireless/nl80211.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-)