| Message ID | f8d6d13943ccac22f363d7d4f53d645c@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <mpubbise@codeaurora.org> 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 <mpubbise@codeaurora.org> >>> 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 helpdesk@kernel.org and the admins >>> can fix it. >>> >>> [1] https://patchwork.kernel.org/register/ >>> >
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
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 >
> 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.
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
>>>> + * @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
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
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.
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 >
On Thu, 2018-05-24 at 06:41 +0200, Sebastian Gottschall wrote: > > 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 That was my point - if it doesn't check the capability then we can remove it without impacting backward compatibility, and add it back in another bit saying "AP-VLAN for 4-addr only". johannes
On 5/23/2018 3:20 PM, Johannes Berg wrote: > 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. Hi Johannes, I was working on splitting the 4-addr functionality from AP/VLAN iftype; I have introduced a new iftype NL80211_IFTYPE_AP_4ADDR and moved the 4-addr handling from AP/VLAN to this new iftype. But this approach breaks the backward compatibility with older userspace applications. Since I am completely moving the 4-addr handling to the new type, older applications which do not understand this new type will simply fail and 4-addr mode will be completely broken. Currently, whenever a 4-addr client attempts a connection, hostapd just creates a AP/VLAN interface and moves the 4-addr client to the AP/VLAN iface; there are no other checks. I had no other option other than going with a new iftype for 4-addr handling. Is there a way we can maintain backward compatibility with this approach? Retaining the 4-addr handling in AP/VLAN and duplicating the same functionality to the new iftype seems will work but I am not sure if this is the right approach. Thanks, Manikanta
On Tue, 2018-08-14 at 18:23 +0530, Manikanta Pubbisetty wrote: > > 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. > I was working on splitting the 4-addr functionality from AP/VLAN iftype; > I have introduced a new iftype NL80211_IFTYPE_AP_4ADDR and moved the > 4-addr handling from AP/VLAN to this new iftype. But this approach > breaks the backward compatibility with older userspace applications. Yeah ... I'm confused and no longer sure what I was thinking, nor even what we're trying to achieve here... > Since I am completely moving the 4-addr handling to the new type, older > applications which do not understand this new type will simply fail and > 4-addr mode will be completely broken. > > Currently, whenever a 4-addr client attempts a connection, hostapd just > creates a AP/VLAN interface and moves the 4-addr client to the AP/VLAN > iface; there are no other checks. I had no other option other than going > with a new iftype for 4-addr handling. > > Is there a way we can maintain backward compatibility with this > approach? Retaining the 4-addr handling in AP/VLAN and duplicating the > same functionality to the new iftype seems will work but I am not sure > if this is the right approach. I think we have to keep the 4-addr handling in AP_VLAN iftype either way, to not break existing hostapd. We could introduce a separate AP_VLAN_NO_4ADDR and then require updating hostapd to get non-4addr VLAN, but that also seems awkward. Since hostapd doesn't currently check anything... Ok, no, I'm confused now. If we just clear WIPHY_FLAG_4ADDR_AP, don't we get what we want? 4-addr AP_VLAN interfaces would no longer be permitted to be created? johannes
On 2018-08-16 13:57, Johannes Berg wrote: > On Tue, 2018-08-14 at 18:23 +0530, Manikanta Pubbisetty wrote: > >> > 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. > >> I was working on splitting the 4-addr functionality from AP/VLAN >> iftype; >> I have introduced a new iftype NL80211_IFTYPE_AP_4ADDR and moved the >> 4-addr handling from AP/VLAN to this new iftype. But this approach >> breaks the backward compatibility with older userspace applications. > > Yeah ... > > I'm confused and no longer sure what I was thinking, nor even what > we're > trying to achieve here... > I had introduced a change db3bdcb9c3ff (" mac80211: allow AP_VLAN operation on crypto controlled devices ") for supporting VLAN functionality on ath10k devices; this commit has broken 4 addr support on ath10k devices as I was advertising the AP/VLAN support conditionally. Since 4 addr operation is tied to AP/VLAN support, with this change, only the chips which support VLAN functionality can support 4 addr operation but other ath10k chips don't. From what I can understand from our previous discussions, we had to separate the 4addr support from the AP/VLAN iftype but doing so could lead to backward compatibility issues. I have the code which separates the 4addr functionality from AP/VLAN but the bigger problem is the backward compatibility. I am hoping that now I have set the context correctly :) >> Since I am completely moving the 4-addr handling to the new type, >> older >> applications which do not understand this new type will simply fail >> and >> 4-addr mode will be completely broken. >> >> Currently, whenever a 4-addr client attempts a connection, hostapd >> just >> creates a AP/VLAN interface and moves the 4-addr client to the AP/VLAN >> iface; there are no other checks. I had no other option other than >> going >> with a new iftype for 4-addr handling. >> >> Is there a way we can maintain backward compatibility with this >> approach? Retaining the 4-addr handling in AP/VLAN and duplicating the >> same functionality to the new iftype seems will work but I am not sure >> if this is the right approach. > > I think we have to keep the 4-addr handling in AP_VLAN iftype either > way, to not break existing hostapd. We could introduce a separate > AP_VLAN_NO_4ADDR and then require updating hostapd to get non-4addr > VLAN, but that also seems awkward. > > Since hostapd doesn't currently check anything... > > Ok, no, I'm confused now. If we just clear WIPHY_FLAG_4ADDR_AP, don't > we > get what we want? 4-addr AP_VLAN interfaces would no longer be > permitted > to be created? As I explained above, the agenda is to provide the driver (in this case, ath10k) a better control for advertising the device capabilities; only few ath10k chips can support VLAN functionality but all of them can support 4 addr operation. Manikanta
Manikanta Pubbisetty <mpubbise@codeaurora.org> writes: > On 8/16/2018 1:57 PM, Johannes Berg wrote: > > On Tue, 2018-08-14 at 18:23 +0530, Manikanta Pubbisetty wrote: > > > 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. > > > > I was working on splitting the 4-addr functionality from AP/VLAN iftype; > I have introduced a new iftype NL80211_IFTYPE_AP_4ADDR and moved the > 4-addr handling from AP/VLAN to this new iftype. But this approach > breaks the backward compatibility with older userspace applications. > > > Yeah ... > > I'm confused and no longer sure what I was thinking, nor even what we're > trying to achieve here... > > I had introduced a change db3bdcb9c3ff (" mac80211: allow AP_VLAN > operation on crypto controlled devices ") for supporting VLAN > functionality on ath10k devices; this commit has broken 4 addr support > on ath10k devices as I was advertising the AP/VLAN support > conditionally. Since 4 addr operation is tied to AP/VLAN support, with > this change, only the chips which support VLAN functionality can > support 4 addr operation but other ath10k chips don't. Manikanta, please set up your Thunderbird so that it uses the standard '>' quotation style. This mail is impossible to read as I don't know which part is written by you and which part by Johannes.
Hi, Sorry ... this got delayed again. > I had introduced a change db3bdcb9c3ff (" mac80211: allow AP_VLAN > operation on crypto controlled devices ") for supporting VLAN > functionality on ath10k devices; this commit has broken 4 addr support > on ath10k devices as I was advertising the AP/VLAN support > conditionally. Since 4 addr operation is tied to AP/VLAN support, with > this change, only the chips which support VLAN functionality can support > 4 addr operation but other ath10k chips don't. Right. > From what I can understand from our previous discussions, we had to > separate the 4addr support from the AP/VLAN iftype but doing so could > lead to backward compatibility issues. I have the code which separates > the 4addr functionality from AP/VLAN but the bigger problem is the > backward compatibility. Ok. > I am hoping that now I have set the context correctly :) Thanks! > > I think we have to keep the 4-addr handling in AP_VLAN iftype either > > way, to not break existing hostapd. We could introduce a separate > > AP_VLAN_NO_4ADDR and then require updating hostapd to get non-4addr > > VLAN, but that also seems awkward. > > > > Since hostapd doesn't currently check anything... > > > > Ok, no, I'm confused now. If we just clear WIPHY_FLAG_4ADDR_AP, don't > > we > > get what we want? 4-addr AP_VLAN interfaces would no longer be > > permitted > > to be created? > > As I explained above, the agenda is to provide the driver (in this case, > ath10k) a better control for advertising the device capabilities; only > few ath10k chips can support VLAN functionality but all of them can > support 4 addr operation. So the problematic part isn't actually the *4-addr* (fake) VLAN, the problematic part is the actual AP_VLAN - I suppose because that uses a separate GTK. So I guess the only, mostly backward compatible way to really separate the two would be to 1) move WIPHY_FLAG_4ADDR_{AP,STATION} to be nl80211 ext feature flags, I guess the STATION always should've been since there's nothing that indicates support for it today in the API along with one of: 2a) Add a new AP_VLAN ext feature flag that indicates the AP_VLAN is not only supported for 4-addr 2b) Allow creating an AP_VLAN interface in 4-addr mode in nl80211_new_interface() even when AP_VLAN is not in the supported interface combinations, if (and only if) WIPHY_FLAG_4ADDR_AP is set (or rather the new extended feature flag after doing 1). Update the language in the documentation somewhere indicating this. Hostapd clearly deals with both 2a and 2b since it never bothers to check the combinations. As a result, I prefer 2b rather than 2a since it doesn't add any weird combinations to the API that would be impossible. johannes
On 9/3/2018 4:09 PM, Johannes Berg wrote: > Hi, > > Sorry ... this got delayed again. > >> I had introduced a change db3bdcb9c3ff (" mac80211: allow AP_VLAN >> operation on crypto controlled devices ") for supporting VLAN >> functionality on ath10k devices; this commit has broken 4 addr support >> on ath10k devices as I was advertising the AP/VLAN support >> conditionally. Since 4 addr operation is tied to AP/VLAN support, with >> this change, only the chips which support VLAN functionality can support >> 4 addr operation but other ath10k chips don't. > Right. > >> From what I can understand from our previous discussions, we had to >> separate the 4addr support from the AP/VLAN iftype but doing so could >> lead to backward compatibility issues. I have the code which separates >> the 4addr functionality from AP/VLAN but the bigger problem is the >> backward compatibility. > Ok. > >> I am hoping that now I have set the context correctly :) > Thanks! > >>> I think we have to keep the 4-addr handling in AP_VLAN iftype either >>> way, to not break existing hostapd. We could introduce a separate >>> AP_VLAN_NO_4ADDR and then require updating hostapd to get non-4addr >>> VLAN, but that also seems awkward. >>> >>> Since hostapd doesn't currently check anything... >>> >>> Ok, no, I'm confused now. If we just clear WIPHY_FLAG_4ADDR_AP, don't >>> we >>> get what we want? 4-addr AP_VLAN interfaces would no longer be >>> permitted >>> to be created? >> As I explained above, the agenda is to provide the driver (in this case, >> ath10k) a better control for advertising the device capabilities; only >> few ath10k chips can support VLAN functionality but all of them can >> support 4 addr operation. > So the problematic part isn't actually the *4-addr* (fake) VLAN, the > problematic part is the actual AP_VLAN - I suppose because that uses a > separate GTK. > > > So I guess the only, mostly backward compatible way to really separate > the two would be to > > 1) move WIPHY_FLAG_4ADDR_{AP,STATION} to be nl80211 ext feature flags, > I guess the STATION always should've been since there's nothing that > indicates support for it today in the API > > along with one of: > > 2a) Add a new AP_VLAN ext feature flag that indicates the AP_VLAN is not > only supported for 4-addr > > 2b) Allow creating an AP_VLAN interface in 4-addr mode in > nl80211_new_interface() even when AP_VLAN is not in the supported > interface combinations, if (and only if) WIPHY_FLAG_4ADDR_AP is set > (or rather the new extended feature flag after doing 1). Update the > language in the documentation somewhere indicating this. > > > Hostapd clearly deals with both 2a and 2b since it never bothers to > check the combinations. As a result, I prefer 2b rather than 2a since it > doesn't add any weird combinations to the API that would be impossible. Sure Johannes!! Thanks, Manikanta
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,
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. 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 <mpubbise@codeaurora.org> 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 <mpubbise@codeaurora.org> >> 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 helpdesk@kernel.org and the admins >> can fix it. >> >> [1] https://patchwork.kernel.org/register/ >>