Re: [PATCH] ath10k: add dynamic vlan support
On Mon, 2018-04-23 at 21:18 +0200, Sebastian Gottschall wrote: > this patch makes no sense at some points. AP_VLAN must be enabled always > (it is enabled by mac80211 by default, but is now disabled in very > latest git version for drivers which announce sw_crypto support) > if its disabled wds ap / wds sta operation will not work anymore since > mac80211 uses AP_VLAN for the local wds sta interfaces You'd do you well to learn the correct terminology used in Linux if you try to communicate with us... What you say there makes no sense, WDS is a separate mode. Maybe you mean 4-addr mode? johannes
Re: [PATCH] ath10k: add dynamic vlan support
On Fri, 2018-05-04 at 12:20 +0530, Manikanta Pubbisetty wrote: > Johannes, > > It seems like commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on > crypto controlled devices") has broken 4-addr operation on crypto > controlled devices as reported by sebastian. > The commit was mainly focused in addressing the problem in supporting > VLANs on crypto controlled devices but since 4-addr mode is also > dependent on AP_VLAN interface, this commit breaks the 4-addr AP mode. Ok. Do you know why it actually broke it? I mean, we should've turned off the strict requirement for sw crypto control only for the GTK, and that shouldn't matter so much? > I have couple of ideas on how to address the problem, > > 1) Add a new hw_flag and based on the hardware flag, allow/disallow the > creation of AP_VLAN interface. > > + * @IEEE80211_HW_SUPPORTS_SW_ENCRYPT: Device is capable of transmitting > + * frames encrypted in software, only valid when SW_CRYPTO_CONTROL > + * is enabled. Based on this flag, mac80211 can allow/disallow VLAN > + * operations in the BSS. Based on the name and initial description, this sounds equivalent to just turning off SW_CRYPTO_CONTROL. I think that's not the intent, but would need some renaming. > 2) Allow mac80211 to call set_key for GTKs on AP_VLAN interfaces for > crypto controlled devices, let the driver decide whether to return '1' > or some error code based on their support for transmitting sw encrypted > frames. I am little skeptical with this approach as drivers are totally > unaware of AP_VLAN interfaces. No, that won't work. I'm unsure how 4-addr VLAN can work with ath10k either way? Maybe it just doesn't normally need a GTK, so nothing broke before, but your other patch changed things to remove VLAN and then of course it's no longer available? But then I don't understand the complaint that So maybe the solution should be to add a separate flag for whether or not 4-addr VLAN is supported? johannes
Re: [PATCH] ath10k: add dynamic vlan support
Am 18.05.2018 um 11:53 schrieb Johannes Berg: On Mon, 2018-04-23 at 21:18 +0200, Sebastian Gottschall wrote: this patch makes no sense at some points. AP_VLAN must be enabled always (it is enabled by mac80211 by default, but is now disabled in very latest git version for drivers which announce sw_crypto support) if its disabled wds ap / wds sta operation will not work anymore since mac80211 uses AP_VLAN for the local wds sta interfaces You'd do you well to learn the correct terminology used in Linux if you try to communicate with us... What you say there makes no sense, WDS is a separate mode. Maybe you mean 4-addr mode? yes 4-addr mode which is common known as WDS mode. (terminology used by all sorts of vendors) Sebastian johannes
Re: [PATCH] ath10k: add dynamic vlan support
Am 18.05.2018 um 11:54 schrieb Johannes Berg: On Fri, 2018-05-04 at 12:20 +0530, Manikanta Pubbisetty wrote: Johannes, It seems like commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on crypto controlled devices") has broken 4-addr operation on crypto controlled devices as reported by sebastian. The commit was mainly focused in addressing the problem in supporting VLANs on crypto controlled devices but since 4-addr mode is also dependent on AP_VLAN interface, this commit breaks the 4-addr AP mode. Ok. Do you know why it actually broke it? I mean, we should've turned off the strict requirement for sw crypto control only for the GTK, and that shouldn't matter so much? I have couple of ideas on how to address the problem, 1) Add a new hw_flag and based on the hardware flag, allow/disallow the creation of AP_VLAN interface. + * @IEEE80211_HW_SUPPORTS_SW_ENCRYPT: Device is capable of transmitting + * frames encrypted in software, only valid when SW_CRYPTO_CONTROL + * is enabled. Based on this flag, mac80211 can allow/disallow VLAN + * operations in the BSS. Based on the name and initial description, this sounds equivalent to just turning off SW_CRYPTO_CONTROL. I think that's not the intent, but would need some renaming. 2) Allow mac80211 to call set_key for GTKs on AP_VLAN interfaces for crypto controlled devices, let the driver decide whether to return '1' or some error code based on their support for transmitting sw encrypted frames. I am little skeptical with this approach as drivers are totally unaware of AP_VLAN interfaces. No, that won't work. I'm unsure how 4-addr VLAN can work with ath10k either way? Maybe it just doesn't normally need a GTK, so nothing broke before, but your other patch changed things to remove VLAN and then of course it's no longer available? But then I don't understand the complaint that So maybe the solution should be to add a separate flag for whether or not 4-addr VLAN is supported? johannes let me explain. the vlan mode is used to create local interfaces in 4addr mode like wlan0.sta0, wlan0.sta1 per peer. this is required to put these peers into the local linux bridge since the local ap interface cannot handle the bridging capabilities like correct forwarding, stp or even filtering. this is a long term behaviour since the beginning of ath9k. so the ap_vlan feature is used to pass the frames per peer in a indepenened way. you may ask felix fietkau, since he developed it originally in madwifi and later in mac80211 / ath9k. so ap_vlan capability is a requiredment for all 4addr capable wireless drivers. example of a 4addr capable ap in ath10k with 2 connected 4addr stations root@apreithalle:~# brctl show bridge name bridge id STP enabled interfaces br0 8000.dcef09e4ce07 no ath0 ath0.1 ath0.2 ath0.sta1 ath0.sta4 ath1 ath1.2 eth0 eth1
Re: [PATCH] ath10k: add dynamic vlan support
Do you know why it actually broke it? I mean, we should've turned off the strict requirement for sw crypto control only for the GTK, and that shouldn't matter so much? With the change db3bdcb9c3ff, AP/VLAN support is advertised by the driver conditionally; the primary reason for doing this is to support VLAN operations on sw crypto controlled devices. AP also creates AP/VLAN devices for supporting 4-addr clients and since the driver now advertises AP/VLAN support conditionally, the 4-addr operation which has no relation to the VLANs(Per VLAN GTKs) was broken on some ath10k devices. I have couple of ideas on how to address the problem, 1) Add a new hw_flag and based on the hardware flag, allow/disallow the creation of AP_VLAN interface. + * @IEEE80211_HW_SUPPORTS_SW_ENCRYPT: Device is capable of transmitting + * frames encrypted in software, only valid when SW_CRYPTO_CONTROL + * is enabled. Based on this flag, mac80211 can allow/disallow VLAN + * operations in the BSS. Based on the name and initial description, this sounds equivalent to just turning off SW_CRYPTO_CONTROL. I think that's not the intent, but would need some renaming. I can rename it to something which is very specific to VLAN support on sw crypto controlled devices if that is okay. 2) Allow mac80211 to call set_key for GTKs on AP_VLAN interfaces for crypto controlled devices, let the driver decide whether to return '1' or some error code based on their support for transmitting sw encrypted frames. I am little skeptical with this approach as drivers are totally unaware of AP_VLAN interfaces. No, that won't work. I'm unsure how 4-addr VLAN can work with ath10k either way? Maybe it just doesn't normally need a GTK, so nothing broke before, but your other patch changed things to remove VLAN and then of course it's no longer available? But then I don't understand the complaint that So maybe the solution should be to add a separate flag for whether or not 4-addr VLAN is supported? IIUC, the combination of 4-addr and VLAN doesn't work even without this change db3bdcb9c3ff (" mac80211: allow AP_VLAN operation on crypto controlled devices "). AP/VLAN devices are used separately to either support 4-addr operation or VLAN (separating the clients into VLAN groups each having unique GTK), I believe both are mutually exclusive. One reason why the combination of 4-addr+VLAN doesn't work could be that a single AP/VLAN interface can cater to several wireless clients but to support 4-addr operation every client device should have an exclusive AP/VLAN interface. It is possible where few clients in a specific VLAN group can use 4-addr mode and few might not, if this is the case then AP has to create individual AP/VLAN device for each 4-addr client and since these clients belong to specific VLAN group, all of these clients should be tied to AP/VLAN device created for VLAN operation(Created for maintaining unique GTKs). I am not sure if the current stack supports this complex combination, I could not make it work in my testing though.
Re: [PATCH] ath10k: add dynamic vlan support
On Mon, 2018-05-21 at 12:12 +0530, Manikanta Pubbisetty wrote: > > Do you know why it actually broke it? I mean, we should've turned off > > the strict requirement for sw crypto control only for the GTK, and that > > shouldn't matter so much? > > With the change db3bdcb9c3ff, AP/VLAN support is advertised by the > driver conditionally; the primary reason for doing this is to support > VLAN operations on sw crypto controlled devices. Right, or, well *not* supporting it. > AP also creates AP/VLAN devices for supporting 4-addr clients and since > the driver now advertises AP/VLAN support conditionally, the 4-addr > operation which has no relation to the VLANs(Per VLAN GTKs) was broken > on some ath10k devices. Right. Like I said, splitting those two capabilities somehow would be best. > > > + * @IEEE80211_HW_SUPPORTS_SW_ENCRYPT: Device is capable of transmitting > > > + * frames encrypted in software, only valid when SW_CRYPTO_CONTROL > > > + * is enabled. Based on this flag, mac80211 can allow/disallow VLAN > > > + * operations in the BSS. > > > > Based on the name and initial description, this sounds equivalent to > > just turning off SW_CRYPTO_CONTROL. I think that's not the intent, but > > would need some renaming. > > I can rename it to something which is very specific to VLAN support on > sw crypto controlled devices if that is okay. I don't think that makes sense. If we split the capability of AP_VLAN and AP_VLAN_FOR_4ADDR at the "root", then we don't need to play with all these things. Yes, this is slightly awkward for userspace, and perhaps with the interface combination checks, but I'd like you to look at that. johannes
Re: [PATCH] ath10k: add dynamic vlan support
+ * @IEEE80211_HW_SUPPORTS_SW_ENCRYPT: Device is capable of transmitting + * frames encrypted in software, only valid when SW_CRYPTO_CONTROL + * is enabled. Based on this flag, mac80211 can allow/disallow VLAN + * operations in the BSS. Based on the name and initial description, this sounds equivalent to just turning off SW_CRYPTO_CONTROL. I think that's not the intent, but would need some renaming. I can rename it to something which is very specific to VLAN support on sw crypto controlled devices if that is okay. I don't think that makes sense. If we split the capability of AP_VLAN and AP_VLAN_FOR_4ADDR at the "root", then we don't need to play with all these things. Yes, this is slightly awkward for userspace, and perhaps with the interface combination checks, but I'd like you to look at that. Makes sense, I had this thought to split the VLAN and 4-addr functionality, but since we need to fiddle with userspace, I refrained. Anyways, I will check all the possibilities of splitting these functionalities. Thanks, Manikanta
Re: [PATCH] ath10k: add dynamic vlan support
On Wed, 2018-05-23 at 16:09 +0530, Manikanta Pubbisetty wrote: > > > > > + * @IEEE80211_HW_SUPPORTS_SW_ENCRYPT: Device is capable of > > > > > transmitting > > > > > + * frames encrypted in software, only valid when > > > > > SW_CRYPTO_CONTROL > > > > > + * is enabled. Based on this flag, mac80211 can allow/disallow > > > > > VLAN > > > > > + * operations in the BSS. > > > > > > > > Based on the name and initial description, this sounds equivalent to > > > > just turning off SW_CRYPTO_CONTROL. I think that's not the intent, but > > > > would need some renaming. > > > > > > I can rename it to something which is very specific to VLAN support on > > > sw crypto controlled devices if that is okay. > > > > I don't think that makes sense. If we split the capability of AP_VLAN > > and AP_VLAN_FOR_4ADDR at the "root", then we don't need to play with all > > these things. Yes, this is slightly awkward for userspace, and perhaps > > with the interface combination checks, but I'd like you to look at that. > > > > Makes sense, I had this thought to split the VLAN and 4-addr > functionality, but since we need to fiddle with userspace, I refrained. > Anyways, I will check all the possibilities of splitting these > functionalities. I'm not sure we really have to - it's likely that wpa_s/hostapd don't check the capabilities? johannes
Re: [PATCH] ath10k: add dynamic vlan support
On 5/23/2018 4:09 PM, Johannes Berg wrote: On Wed, 2018-05-23 at 16:09 +0530, Manikanta Pubbisetty wrote: + * @IEEE80211_HW_SUPPORTS_SW_ENCRYPT: Device is capable of transmitting + * frames encrypted in software, only valid when SW_CRYPTO_CONTROL + * is enabled. Based on this flag, mac80211 can allow/disallow VLAN + * operations in the BSS. Based on the name and initial description, this sounds equivalent to just turning off SW_CRYPTO_CONTROL. I think that's not the intent, but would need some renaming. I can rename it to something which is very specific to VLAN support on sw crypto controlled devices if that is okay. I don't think that makes sense. If we split the capability of AP_VLAN and AP_VLAN_FOR_4ADDR at the "root", then we don't need to play with all these things. Yes, this is slightly awkward for userspace, and perhaps with the interface combination checks, but I'd like you to look at that. Makes sense, I had this thought to split the VLAN and 4-addr functionality, but since we need to fiddle with userspace, I refrained. Anyways, I will check all the possibilities of splitting these functionalities. I'm not sure we really have to - it's likely that wpa_s/hostapd don't check the capabilities? johannes IIUC, hostapd will just invoke a NL command to create the AP/VLAN interface, the capabilities are checked mostly in cfg80211. If we are planning to split the functionality, possibly something like having two separate IF-TYPES(one for VLAN and one for 4-addr) instead of one(which is the case now), we should probably change the relevant areas in hostapd as well, not really sure of the complexity of the work involved in userspace though.
Re: [PATCH] ath10k: add dynamic vlan support
Am 23.05.2018 um 12:39 schrieb Johannes Berg: On Wed, 2018-05-23 at 16:09 +0530, Manikanta Pubbisetty wrote: + * @IEEE80211_HW_SUPPORTS_SW_ENCRYPT: Device is capable of transmitting + * frames encrypted in software, only valid when SW_CRYPTO_CONTROL + * is enabled. Based on this flag, mac80211 can allow/disallow VLAN + * operations in the BSS. Based on the name and initial description, this sounds equivalent to just turning off SW_CRYPTO_CONTROL. I think that's not the intent, but would need some renaming. I can rename it to something which is very specific to VLAN support on sw crypto controlled devices if that is okay. I don't think that makes sense. If we split the capability of AP_VLAN and AP_VLAN_FOR_4ADDR at the "root", then we don't need to play with all these things. Yes, this is slightly awkward for userspace, and perhaps with the interface combination checks, but I'd like you to look at that. Makes sense, I had this thought to split the VLAN and 4-addr functionality, but since we need to fiddle with userspace, I refrained. Anyways, I will check all the possibilities of splitting these functionalities. I'm not sure we really have to - it's likely that wpa_s/hostapd don't check the capabilities? mmh not sure. it might not check the capabilitiy, but it sets the interface type to IFTYPE_AP_VLAN for wds sta interfaces. that might collide see the following snippet taken from hostapd code static int i802_set_wds_sta(void *priv, const u8 *addr, int aid, int val, const char *bridge_ifname, char *ifname_wds) { struct i802_bss *bss = priv; struct wpa_driver_nl80211_data *drv = bss->drv; char name[IFNAMSIZ + 1]; os_snprintf(name, sizeof(name), "%s.sta%d", bss->ifname, aid); if (ifname_wds) os_strlcpy(ifname_wds, name, IFNAMSIZ + 1); wpa_printf(MSG_DEBUG, "nl80211: Set WDS STA addr=" MACSTR " aid=%d val=%d name=%s", MAC2STR(addr), aid, val, name); if (val) { if (!if_nametoindex(name)) { if (nl80211_create_iface(drv, name, NL80211_IFTYPE_AP_VLAN, bss->addr, 1, NULL, NULL, 0) < 0) return -1; if (bridge_ifname && linux_br_add_if(drv->global->ioctl_sock, bridge_ifname, name) < 0) return -1; } if (linux_set_iface_flags(drv->global->ioctl_sock, name, 1)) { wpa_printf(MSG_ERROR, "nl80211: Failed to set WDS STA " "interface %s up", name); } return i802_set_sta_vlan(priv, addr, name, 0); } else { if (bridge_ifname) linux_br_del_if(drv->global->ioctl_sock, bridge_ifname, name); i802_set_sta_vlan(priv, addr, bss->ifname, 0); nl80211_remove_iface(drv, if_nametoindex(name)); return 0; } } johannes
Re: [PATCH] ath10k: add dynamic vlan support
this patch makes no sense at some points. AP_VLAN must be enabled always (it is enabled by mac80211 by default, but is now disabled in very latest git version for drivers which announce sw_crypto support) if its disabled wds ap / wds sta operation will not work anymore since mac80211 uses AP_VLAN for the local wds sta interfaces Sebastian Am 20.04.2018 um 15:57 schrieb Manikanta Pubbisetty: Mutlicast/broadcast traffic destined for a particular vlan group will always be encrypted in software. To enable dynamic VLANs, it requires driver support for sending software encrypted packets. In ath10k, sending sw encrypted frames is allowed only when we insmod the driver with cryptmode param set to 1, this configuration disables hardware crypto and enables RAW mode implicitly. Since, enabling raw mode has performance impact, this cannot be considered as an ideal solution for supporting VLANs in the driver. As an alternative take, in this approach, cryptographic keys for unicast traffic(per peer PTKs) and keys for non-vlan group traffic will be configured in hardware, allowing hardware encryption for unicast and non-vlan group traffic. Only vlan group traffic will be encrypted in software and pushed to the target with encap mode set to RAW in the TX descriptors. Not all firmwares can support this type of key configuration(having few keys installed in hardware and few only in software); for this purpose a new WMI service flag "WMI_SERVICE_PER_PACKET_SW_ENCRYPT" is introduced to advertise this support. Also, adding the logic required to send sw encrypted frames in raw mode. Tested this change on QCA9984(firmware version 10.4-3.5.3-00057). Signed-off-by: Manikanta Pubbisetty --- drivers/net/wireless/ath/ath10k/core.h | 1 + drivers/net/wireless/ath/ath10k/mac.c | 26 -- drivers/net/wireless/ath/ath10k/wmi.h | 21 + 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h index e4ac8f2..105438d 100644 --- a/drivers/net/wireless/ath/ath10k/core.h +++ b/drivers/net/wireless/ath/ath10k/core.h @@ -122,6 +122,7 @@ enum ath10k_skb_flags { ATH10K_SKB_F_DELIVER_CAB = BIT(2), ATH10K_SKB_F_MGMT = BIT(3), ATH10K_SKB_F_QOS = BIT(4), + ATH10K_SKB_F_RAW_TX = BIT(5), }; struct ath10k_skb_cb { diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index fc3320f..694c0aa 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -3362,6 +3362,7 @@ ath10k_mac_tx_h_get_txmode(struct ath10k *ar, struct sk_buff *skb) { const struct ieee80211_hdr *hdr = (void *)skb->data; + const struct ath10k_skb_cb *skb_cb = ATH10K_SKB_CB(skb); __le16 fc = hdr->frame_control; if (!vif || vif->type == NL80211_IFTYPE_MONITOR) @@ -3403,7 +3404,8 @@ ath10k_mac_tx_h_get_txmode(struct ath10k *ar, if (ieee80211_is_data_present(fc) && sta && sta->tdls) return ATH10K_HW_TXRX_ETHERNET; - if (test_bit(ATH10K_FLAG_RAW_MODE, &ar->dev_flags)) + if (test_bit(ATH10K_FLAG_RAW_MODE, &ar->dev_flags) || + skb_cb->flags & ATH10K_SKB_F_RAW_TX) return ATH10K_HW_TXRX_RAW; return ATH10K_HW_TXRX_NATIVE_WIFI; @@ -3513,6 +3515,9 @@ static void ath10k_mac_tx_h_fill_cb(struct ath10k *ar, { struct ieee80211_hdr *hdr = (void *)skb->data; struct ath10k_skb_cb *cb = ATH10K_SKB_CB(skb); + const struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb); + bool is_data = ieee80211_is_data(hdr->frame_control) || + ieee80211_is_data_qos(hdr->frame_control); cb->flags = 0; if (!ath10k_tx_h_use_hwcrypto(vif, skb)) @@ -3524,6 +3529,16 @@ static void ath10k_mac_tx_h_fill_cb(struct ath10k *ar, if (ieee80211_is_data_qos(hdr->frame_control)) cb->flags |= ATH10K_SKB_F_QOS; + /* Data frames encrypted in software will be posted to firmware +* with tx encap mode set to RAW. One such case would be the +* multicast traffic generated for a VLAN group. +*/ + if (is_data && ieee80211_has_protected(hdr->frame_control) && + !info->control.hw_key) { + cb->flags |= ATH10K_SKB_F_NO_HWCRYPT; + cb->flags |= ATH10K_SKB_F_RAW_TX; + } + cb->vif = vif; cb->txq = txq; } @@ -3632,6 +3647,7 @@ static int ath10k_mac_tx(struct ath10k *ar, { struct ieee80211_hw *hw = ar->hw; struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb); + const struct ath10k_skb_cb *skb_cb = ATH10K_SKB_CB(skb); int ret; /* We should disable CCK RATE due to P2P */ @@ -3649,7 +3665,8 @@ static int ath10k_mac_tx(struct ath10k *ar, ath10k_tx_h_8023(skb); break; case ATH10K_HW_TXRX_RAW: - if (!test
Re: [PATCH] ath10k: add dynamic vlan support
Manikanta Pubbisetty writes: > Mutlicast/broadcast traffic destined for a particular vlan group will > always be encrypted in software. To enable dynamic VLANs, it requires > driver support for sending software encrypted packets. > > In ath10k, sending sw encrypted frames is allowed only when we insmod > the driver with cryptmode param set to 1, this configuration disables > hardware crypto and enables RAW mode implicitly. Since, enabling raw > mode has performance impact, this cannot be considered as an ideal > solution for supporting VLANs in the driver. > > As an alternative take, in this approach, cryptographic keys for > unicast traffic(per peer PTKs) and keys for non-vlan group traffic > will be configured in hardware, allowing hardware encryption for unicast > and non-vlan group traffic. Only vlan group traffic will be encrypted in > software and pushed to the target with encap mode set to RAW in the TX > descriptors. > > Not all firmwares can support this type of key configuration(having few > keys installed in hardware and few only in software); for this purpose a > new WMI service flag "WMI_SERVICE_PER_PACKET_SW_ENCRYPT" is introduced to > advertise this support. > > Also, adding the logic required to send sw encrypted frames in raw mode. > > Tested this change on QCA9984(firmware version 10.4-3.5.3-00057). > > Signed-off-by: Manikanta Pubbisetty Your name in patchwork is wrong and hence my script uses the wrong name. Please fix it by registering to patchwork[1] where it's possible to change your name during registration, but only one time. If that doesn't work then send a request to helpd...@kernel.org and the admins can fix it. [1] https://patchwork.kernel.org/register/ -- Kalle Valo
Re: [PATCH] ath10k: add dynamic vlan support
consider my comment regarding vlan_ap. this patch will break wds ap / wds sta support with latest mac80211 (see also my post on the wireless mailing list about the breaking patch in mac80211) so AP_VLAN must be masked always for all chipsets. otherwise wds breaks and this is not just a guess. i tested it yesterday using this patch and found the cause of the issue the following lines + if (test_bit(WMI_SERVICE_PER_PACKET_SW_ENCRYPT, ar->wmi.svc_map)) { + ar->hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_AP_VLAN); + ar->hw->wiphy->software_iftypes |= BIT(NL80211_IFTYPE_AP_VLAN); + } must be just + ar->hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_AP_VLAN); + ar->hw->wiphy->software_iftypes |= BIT(NL80211_IFTYPE_AP_VLAN); everthing else will cause a regression Am 24.04.2018 um 10:09 schrieb Kalle Valo: Manikanta Pubbisetty writes: Mutlicast/broadcast traffic destined for a particular vlan group will always be encrypted in software. To enable dynamic VLANs, it requires driver support for sending software encrypted packets. In ath10k, sending sw encrypted frames is allowed only when we insmod the driver with cryptmode param set to 1, this configuration disables hardware crypto and enables RAW mode implicitly. Since, enabling raw mode has performance impact, this cannot be considered as an ideal solution for supporting VLANs in the driver. As an alternative take, in this approach, cryptographic keys for unicast traffic(per peer PTKs) and keys for non-vlan group traffic will be configured in hardware, allowing hardware encryption for unicast and non-vlan group traffic. Only vlan group traffic will be encrypted in software and pushed to the target with encap mode set to RAW in the TX descriptors. Not all firmwares can support this type of key configuration(having few keys installed in hardware and few only in software); for this purpose a new WMI service flag "WMI_SERVICE_PER_PACKET_SW_ENCRYPT" is introduced to advertise this support. Also, adding the logic required to send sw encrypted frames in raw mode. Tested this change on QCA9984(firmware version 10.4-3.5.3-00057). Signed-off-by: Manikanta Pubbisetty Your name in patchwork is wrong and hence my script uses the wrong name. Please fix it by registering to patchwork[1] where it's possible to change your name during registration, but only one time. If that doesn't work then send a request to helpd...@kernel.org and the admins can fix it. [1] https://patchwork.kernel.org/register/ -- Mit freundlichen Grüssen / Regards Sebastian Gottschall / CTO NewMedia-NET GmbH - DD-WRT Firmensitz: Stubenwaldallee 21a, 64625 Bensheim Registergericht: Amtsgericht Darmstadt, HRB 25473 Geschäftsführer: Peter Steinhäuser, Christian Scheele http://www.dd-wrt.com email: s.gottsch...@dd-wrt.com Tel.: +496251-582650 / Fax: +496251-5826565
Re: [PATCH] ath10k: add dynamic vlan support
Yes Sebastian, Your point is valid. This would break the 4-addr operation on other ath10k devices which does not support the new WMI service. I have another approach to solve this problem, I will come with a small patch and see what johannes has to say on that approach. Manikanta On 4/24/2018 2:39 PM, Sebastian Gottschall wrote: consider my comment regarding vlan_ap. this patch will break wds ap / wds sta support with latest mac80211 (see also my post on the wireless mailing list about the breaking patch in mac80211) so AP_VLAN must be masked always for all chipsets. otherwise wds breaks and this is not just a guess. i tested it yesterday using this patch and found the cause of the issue the following lines + if (test_bit(WMI_SERVICE_PER_PACKET_SW_ENCRYPT, ar->wmi.svc_map)) { + ar->hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_AP_VLAN); + ar->hw->wiphy->software_iftypes |= BIT(NL80211_IFTYPE_AP_VLAN); + } must be just + ar->hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_AP_VLAN); + ar->hw->wiphy->software_iftypes |= BIT(NL80211_IFTYPE_AP_VLAN); everthing else will cause a regression Am 24.04.2018 um 10:09 schrieb Kalle Valo: Manikanta Pubbisetty writes: Mutlicast/broadcast traffic destined for a particular vlan group will always be encrypted in software. To enable dynamic VLANs, it requires driver support for sending software encrypted packets. In ath10k, sending sw encrypted frames is allowed only when we insmod the driver with cryptmode param set to 1, this configuration disables hardware crypto and enables RAW mode implicitly. Since, enabling raw mode has performance impact, this cannot be considered as an ideal solution for supporting VLANs in the driver. As an alternative take, in this approach, cryptographic keys for unicast traffic(per peer PTKs) and keys for non-vlan group traffic will be configured in hardware, allowing hardware encryption for unicast and non-vlan group traffic. Only vlan group traffic will be encrypted in software and pushed to the target with encap mode set to RAW in the TX descriptors. Not all firmwares can support this type of key configuration(having few keys installed in hardware and few only in software); for this purpose a new WMI service flag "WMI_SERVICE_PER_PACKET_SW_ENCRYPT" is introduced to advertise this support. Also, adding the logic required to send sw encrypted frames in raw mode. Tested this change on QCA9984(firmware version 10.4-3.5.3-00057). Signed-off-by: Manikanta Pubbisetty Your name in patchwork is wrong and hence my script uses the wrong name. Please fix it by registering to patchwork[1] where it's possible to change your name during registration, but only one time. If that doesn't work then send a request to helpd...@kernel.org and the admins can fix it. [1] https://patchwork.kernel.org/register/
Re: [PATCH] ath10k: add dynamic vlan support
Sebastian Gottschall writes: > consider my comment regarding vlan_ap. I am considering comment, I just go one issue at a time and haven't had a time to look at your comment. But PLEASE do not top post, it's very annoying and creates a mess in patchwork. https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#do_not_top_post_and_edit_your_quotes -- Kalle Valo
Re: [PATCH] ath10k: add dynamic vlan support
Am 24.04.2018 um 11:52 schrieb Kalle Valo: Sebastian Gottschall writes: consider my comment regarding vlan_ap. I am considering comment, I just go one issue at a time and haven't had a time to look at your comment. But PLEASE do not top post, it's very annoying and creates a mess in patchwork. https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#do_not_top_post_and_edit_your_quotes i almost do top posts for a single reason. you have to scroll down a long time sometimes to get the essential information. i dont know why most people in my country prefer top posting. i will try to remember and change it in future -- Mit freundlichen Grüssen / Regards Sebastian Gottschall / CTO NewMedia-NET GmbH - DD-WRT Firmensitz: Stubenwaldallee 21a, 64625 Bensheim Registergericht: Amtsgericht Darmstadt, HRB 25473 Geschäftsführer: Peter Steinhäuser, Christian Scheele http://www.dd-wrt.com email: s.gottsch...@dd-wrt.com Tel.: +496251-582650 / Fax: +496251-5826565
Re: [PATCH] ath10k: add dynamic vlan support
Johannes, It seems like commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on crypto controlled devices") has broken 4-addr operation on crypto controlled devices as reported by sebastian. The commit was mainly focused in addressing the problem in supporting VLANs on crypto controlled devices but since 4-addr mode is also dependent on AP_VLAN interface, this commit breaks the 4-addr AP mode. I have couple of ideas on how to address the problem, 1) Add a new hw_flag and based on the hardware flag, allow/disallow the creation of AP_VLAN interface. diff --git a/include/net/mac80211.h b/include/net/mac80211.h index d2279b2..301d9c38 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -2084,6 +2084,11 @@ struct ieee80211_txq { * @IEEE80211_HW_DOESNT_SUPPORT_QOS_NDP: The driver (or firmware) doesn't * support QoS NDP for AP probing - that's most likely a driver bug. * + * @IEEE80211_HW_SUPPORTS_SW_ENCRYPT: Device is capable of transmitting + * frames encrypted in software, only valid when SW_CRYPTO_CONTROL + * is enabled. Based on this flag, mac80211 can allow/disallow VLAN + * operations in the BSS. + * * @NUM_IEEE80211_HW_FLAGS: number of hardware flags, used for sizing arrays */ enum ieee80211_hw_flags { @@ -2129,6 +2134,7 @@ enum ieee80211_hw_flags { IEEE80211_HW_SUPPORTS_TDLS_BUFFER_STA, IEEE80211_HW_DEAUTH_NEED_MGD_TX_PREP, IEEE80211_HW_DOESNT_SUPPORT_QOS_NDP, + IEEE80211_HW_SUPPORTS_SW_ENCRYPT, /* keep last, obviously */ NUM_IEEE80211_HW_FLAGS diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 555e389..c825d27 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -1736,6 +1736,11 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name, ASSERT_RTNL(); + if ((type == NL80211_IFTYPE_AP_VLAN) && !params->use_4addr && + ieee80211_hw_check(local->hw, SW_CRYPTO_CONTROL) && + !ieee80211_hw_check(local->hw, SUPPORTS_SW_ENCRYPT)) + return -EOPNOTSUPP; + if (type == NL80211_IFTYPE_P2P_DEVICE || type == NL80211_IFTYPE_NAN) { struct wireless_dev *wdev; 2) Allow mac80211 to call set_key for GTKs on AP_VLAN interfaces for crypto controlled devices, let the driver decide whether to return '1' or some error code based on their support for transmitting sw encrypted frames. I am little skeptical with this approach as drivers are totally unaware of AP_VLAN interfaces. diff --git a/net/mac80211/key.c b/net/mac80211/key.c index ee0d0cc..0ff5597 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -167,7 +167,8 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key) * The driver doesn't know anything about VLAN interfaces. * Hence, don't send GTKs for VLAN interfaces to the driver. */ - if (!(key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE)) + if ((!(key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE) && + !ieee80211_hw_check(&key->local->hw, SW_CRYPTO_CONTROL))) goto out_unsupported; } Please let me know which is the better way to deal the problem. Thanks, Manikanta On 2018-04-24 14:39, Sebastian Gottschall wrote: consider my comment regarding vlan_ap. this patch will break wds ap / wds sta support with latest mac80211 (see also my post on the wireless mailing list about the breaking patch in mac80211) so AP_VLAN must be masked always for all chipsets. otherwise wds breaks and this is not just a guess. i tested it yesterday using this patch and found the cause of the issue the following lines + if (test_bit(WMI_SERVICE_PER_PACKET_SW_ENCRYPT, ar->wmi.svc_map)) { + ar->hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_AP_VLAN); + ar->hw->wiphy->software_iftypes |= BIT(NL80211_IFTYPE_AP_VLAN); + } must be just + ar->hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_AP_VLAN); + ar->hw->wiphy->software_iftypes |= BIT(NL80211_IFTYPE_AP_VLAN); everthing else will cause a regression Am 24.04.2018 um 10:09 schrieb Kalle Valo: Manikanta Pubbisetty writes: Mutlicast/broadcast traffic destined for a particular vlan group will always be encrypted in software. To enable dynamic VLANs, it requires driver support for sending software encrypted packets. In ath10k, sending sw encrypted frames is allowed only when we insmod the driver with cryptmode param set to 1, this configuration disables hardware crypto and enables RAW mode implicitly. Since, enabling raw mode has performance impact, this cannot be considered as an ideal solution for supporting VLANs in the driver. As an alternative take, in this approach, cryptographic keys for unicast traffic(per peer PTKs) and keys for non-vlan group traffic will be configured in hardware, allowing hardware encryption for unicast and non-vlan group traffic. Only vlan gr
Re: [PATCH] ath10k: add dynamic vlan support
did someone notice that GTK rekeying is broken in 4addr mode for 10.4. firmwares since a long time. i have a test with 2 9984 devices. one is 4addr ap, the second 4addr sta. it disconnects on rekeying and reauthenticates. (reproducable with 99x0 as well) this does not occur on 10.2 based chipsets like 988x. it seems to be a internal firmware problem since the beginning. i wrote already a long time ago about it, but no solution was provided. maybe its also finally time to take care about this issue (cause unknown) Sebastian Am 04.05.2018 um 08:50 schrieb Manikanta Pubbisetty: Johannes, It seems like commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on crypto controlled devices") has broken 4-addr operation on crypto controlled devices as reported by sebastian. The commit was mainly focused in addressing the problem in supporting VLANs on crypto controlled devices but since 4-addr mode is also dependent on AP_VLAN interface, this commit breaks the 4-addr AP mode. I have couple of ideas on how to address the problem, 1) Add a new hw_flag and based on the hardware flag, allow/disallow the creation of AP_VLAN interface. diff --git a/include/net/mac80211.h b/include/net/mac80211.h index d2279b2..301d9c38 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -2084,6 +2084,11 @@ struct ieee80211_txq { * @IEEE80211_HW_DOESNT_SUPPORT_QOS_NDP: The driver (or firmware) doesn't * support QoS NDP for AP probing - that's most likely a driver bug. * + * @IEEE80211_HW_SUPPORTS_SW_ENCRYPT: Device is capable of transmitting + * frames encrypted in software, only valid when SW_CRYPTO_CONTROL + * is enabled. Based on this flag, mac80211 can allow/disallow VLAN + * operations in the BSS. + * * @NUM_IEEE80211_HW_FLAGS: number of hardware flags, used for sizing arrays */ enum ieee80211_hw_flags { @@ -2129,6 +2134,7 @@ enum ieee80211_hw_flags { IEEE80211_HW_SUPPORTS_TDLS_BUFFER_STA, IEEE80211_HW_DEAUTH_NEED_MGD_TX_PREP, IEEE80211_HW_DOESNT_SUPPORT_QOS_NDP, + IEEE80211_HW_SUPPORTS_SW_ENCRYPT, /* keep last, obviously */ NUM_IEEE80211_HW_FLAGS diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 555e389..c825d27 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -1736,6 +1736,11 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name, ASSERT_RTNL(); + if ((type == NL80211_IFTYPE_AP_VLAN) && !params->use_4addr && + ieee80211_hw_check(local->hw, SW_CRYPTO_CONTROL) && + !ieee80211_hw_check(local->hw, SUPPORTS_SW_ENCRYPT)) + return -EOPNOTSUPP; + if (type == NL80211_IFTYPE_P2P_DEVICE || type == NL80211_IFTYPE_NAN) { struct wireless_dev *wdev; 2) Allow mac80211 to call set_key for GTKs on AP_VLAN interfaces for crypto controlled devices, let the driver decide whether to return '1' or some error code based on their support for transmitting sw encrypted frames. I am little skeptical with this approach as drivers are totally unaware of AP_VLAN interfaces. diff --git a/net/mac80211/key.c b/net/mac80211/key.c index ee0d0cc..0ff5597 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -167,7 +167,8 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key) * The driver doesn't know anything about VLAN interfaces. * Hence, don't send GTKs for VLAN interfaces to the driver. */ - if (!(key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE)) + if ((!(key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE) && + !ieee80211_hw_check(&key->local->hw, SW_CRYPTO_CONTROL))) goto out_unsupported; } Please let me know which is the better way to deal the problem. Thanks, Manikanta On 2018-04-24 14:39, Sebastian Gottschall wrote: consider my comment regarding vlan_ap. this patch will break wds ap / wds sta support with latest mac80211 (see also my post on the wireless mailing list about the breaking patch in mac80211) so AP_VLAN must be masked always for all chipsets. otherwise wds breaks and this is not just a guess. i tested it yesterday using this patch and found the cause of the issue the following lines + if (test_bit(WMI_SERVICE_PER_PACKET_SW_ENCRYPT, ar->wmi.svc_map)) { + ar->hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_AP_VLAN); + ar->hw->wiphy->software_iftypes |= BIT(NL80211_IFTYPE_AP_VLAN); + } must be just + ar->hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_AP_VLAN); + ar->hw->wiphy->software_iftypes |= BIT(NL80211_IFTYPE_AP_VLAN); everthing else will cause a regression Am 24.04.2018 um 10:09 schrieb Kalle Valo: Manikanta Pubbisetty writes: Mutlicast/broadcast traffic destined for a particular vlan group will always be encrypted in software. To enable dynamic VLANs, it requires driver support for sending software en