| Message ID | 1522242259-16166-1-git-send-email-mpubbise@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
Hello, > From: Manikanta Pubbisetty <mpubbise@codeaurora.org> > > In the current implementation, mac80211 advertises the support of > AP_VLANs based on the driver's support for AP mode; it also > blocks encrypted AP_VLAN operation on devices advertising > SW_CRYPTO_CONTROL. > > The implementation seems weird in it's current form and could be > often confusing, this is because there can be drivers advertising > both SW_CRYPTO_CONTROL and AP mode support (ex: ath10k) in which case > AP_VLAN will still be supported but only in open BSS and not in > secured BSS. > > When SW_CRYPTO_CONTROL is enabled, it makes more sense if the decision > to support AP_VLANs is left to the driver. Mac80211 can then allow > AP_VLAN operations depending on the driver support. This first part of the patch contradicts my current understanding of how Software crypto fallback can be triggered: We have a driver actively telling us to only fall back to sw crypto when it returns 1 on set_key, BUT we ignore that when the interface is set to @NL80211_IFTYPE_AP_VLAN and allow software encryption unconditionally? Here the code: case WLAN_CIPHER_SUITE_GCMP: case WLAN_CIPHER_SUITE_GCMP_256: /* all of these we can do in software - if driver can */ if (ret == 1) return 0; if (ieee80211_hw_check(&key->local->hw, SW_CRYPTO_CONTROL)) { if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) return 0; return -EINVAL; } return 0; default: return -EINVAL; } Wouldn't it be preferable to just return "ret" or "-EINVAL" instead of "0" when the interface has @NL80211_IFTYPE_AP_VLAN set? As it is this basically overrides SW_CRYPTO_CONTROL in AP Vlan mode! For me it looks like the old behavior in this section was already fine and does not hurt the intention of this patch: A driver setting SW_CRYPTO_CONTROL won't get support for AP VLANs as long as the driver is not opting in to it. Therefore I would like to undo this part of the patch again: - if (ieee80211_hw_check(&key->local->hw, SW_CRYPTO_CONTROL)) + if (ieee80211_hw_check(&key->local->hw, SW_CRYPTO_CONTROL)) { + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) + return 0; return -EINVAL; + } Do I miss something here and would anyone have issues when I revert that in another patch? Alexander
yes you just missed that ap_vlan is used for 4addr ap's / wds too so that might be related to the weired handling Sebastian Am 02.12.2018 um 14:02 schrieb Alexander Wetzel: > Hello, > >> From: Manikanta Pubbisetty <mpubbise@codeaurora.org> >> >> In the current implementation, mac80211 advertises the support of >> AP_VLANs based on the driver's support for AP mode; it also >> blocks encrypted AP_VLAN operation on devices advertising >> SW_CRYPTO_CONTROL. >> >> The implementation seems weird in it's current form and could be >> often confusing, this is because there can be drivers advertising >> both SW_CRYPTO_CONTROL and AP mode support (ex: ath10k) in which case >> AP_VLAN will still be supported but only in open BSS and not in >> secured BSS. >> >> When SW_CRYPTO_CONTROL is enabled, it makes more sense if the decision >> to support AP_VLANs is left to the driver. Mac80211 can then allow >> AP_VLAN operations depending on the driver support. > > This first part of the patch contradicts my current understanding of > how Software crypto fallback can be triggered: We have a driver > actively telling us to only fall back to sw crypto when it returns 1 > on set_key, BUT we ignore that when the interface is set to > @NL80211_IFTYPE_AP_VLAN and allow software encryption unconditionally? > > Here the code: > case WLAN_CIPHER_SUITE_GCMP: > case WLAN_CIPHER_SUITE_GCMP_256: > /* all of these we can do in software - if driver can */ > if (ret == 1) > return 0; > if (ieee80211_hw_check(&key->local->hw, > SW_CRYPTO_CONTROL)) { > if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) > return 0; > return -EINVAL; > } > return 0; > default: > return -EINVAL; > } > > > Wouldn't it be preferable to just return "ret" or "-EINVAL" instead of > "0" when the interface has @NL80211_IFTYPE_AP_VLAN set? > As it is this basically overrides SW_CRYPTO_CONTROL in AP Vlan mode! > > For me it looks like the old behavior in this section was already fine > and does not hurt the intention of this patch: A driver setting > SW_CRYPTO_CONTROL won't get support for AP VLANs as long as the driver > is not opting in to it. > > Therefore I would like to undo this part of the patch again: > > - if (ieee80211_hw_check(&key->local->hw, > SW_CRYPTO_CONTROL)) > + if (ieee80211_hw_check(&key->local->hw, > SW_CRYPTO_CONTROL)) { > + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) > + return 0; > return -EINVAL; > + } > > > Do I miss something here and would anyone have issues when I revert > that in another patch? > > Alexander >
> yes you just missed that ap_vlan is used for 4addr ap's / wds too so > that might be related to the weired handling > For my understanding ath10k is supporting 4addr mode with HW ciphers only. (As long as you do now switch it to raw mode manually and forcing SW crypto only.) The code in question seems to be only reachable if: 1) set_key returned something other than 0 or 1 2) The driver has set SW_CRYPTO_CONTROL Once triggered it will overwrite the information from the driver, that it's not able to handle this key with either hardware or software encryption and try SW encryption anyway. The packets should then be dropped/scrambled by the card and not be useful in any way. Or the other way round: 4addr frames are still QoS data frames, correct? So why should SW crypto working after the driver told us it will not? Alexander > Sebastian > > Am 02.12.2018 um 14:02 schrieb Alexander Wetzel: >> Hello, >> >>> From: Manikanta Pubbisetty <mpubbise@codeaurora.org> >>> >>> In the current implementation, mac80211 advertises the support of >>> AP_VLANs based on the driver's support for AP mode; it also >>> blocks encrypted AP_VLAN operation on devices advertising >>> SW_CRYPTO_CONTROL. >>> >>> The implementation seems weird in it's current form and could be >>> often confusing, this is because there can be drivers advertising >>> both SW_CRYPTO_CONTROL and AP mode support (ex: ath10k) in which case >>> AP_VLAN will still be supported but only in open BSS and not in >>> secured BSS. >>> >>> When SW_CRYPTO_CONTROL is enabled, it makes more sense if the decision >>> to support AP_VLANs is left to the driver. Mac80211 can then allow >>> AP_VLAN operations depending on the driver support. >> >> This first part of the patch contradicts my current understanding of >> how Software crypto fallback can be triggered: We have a driver >> actively telling us to only fall back to sw crypto when it returns 1 >> on set_key, BUT we ignore that when the interface is set to >> @NL80211_IFTYPE_AP_VLAN and allow software encryption unconditionally? >> >> Here the code: >> case WLAN_CIPHER_SUITE_GCMP: >> case WLAN_CIPHER_SUITE_GCMP_256: >> /* all of these we can do in software - if driver can */ >> if (ret == 1) >> return 0; >> if (ieee80211_hw_check(&key->local->hw, >> SW_CRYPTO_CONTROL)) { >> if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) >> return 0; >> return -EINVAL; >> } >> return 0; >> default: >> return -EINVAL; >> } >> >> >> Wouldn't it be preferable to just return "ret" or "-EINVAL" instead of >> "0" when the interface has @NL80211_IFTYPE_AP_VLAN set? >> As it is this basically overrides SW_CRYPTO_CONTROL in AP Vlan mode! >> >> For me it looks like the old behavior in this section was already fine >> and does not hurt the intention of this patch: A driver setting >> SW_CRYPTO_CONTROL won't get support for AP VLANs as long as the driver >> is not opting in to it. >> >> Therefore I would like to undo this part of the patch again: >> >> - if (ieee80211_hw_check(&key->local->hw, >> SW_CRYPTO_CONTROL)) >> + if (ieee80211_hw_check(&key->local->hw, >> SW_CRYPTO_CONTROL)) { >> + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) >> + return 0; >> return -EINVAL; >> + } >> >> >> Do I miss something here and would anyone have issues when I revert >> that in another patch? >> >> Alexander >>
diff --git a/net/mac80211/key.c b/net/mac80211/key.c index aee05ec..ee0d0cc 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -126,7 +126,7 @@ static void decrease_tailroom_need_count(struct ieee80211_sub_if_data *sdata, static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key) { - struct ieee80211_sub_if_data *sdata; + struct ieee80211_sub_if_data *sdata = key->sdata; struct sta_info *sta; int ret = -EOPNOTSUPP; @@ -162,7 +162,6 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key) if (sta && !sta->uploaded) goto out_unsupported; - sdata = key->sdata; if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) { /* * The driver doesn't know anything about VLAN interfaces. @@ -214,8 +213,11 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key) /* all of these we can do in software - if driver can */ if (ret == 1) return 0; - if (ieee80211_hw_check(&key->local->hw, SW_CRYPTO_CONTROL)) + if (ieee80211_hw_check(&key->local->hw, SW_CRYPTO_CONTROL)) { + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) + return 0; return -EINVAL; + } return 0; default: return -EINVAL; diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 0785d04..8d0333b 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -930,8 +930,12 @@ int ieee80211_register_hw(struct ieee80211_hw *hw) IEEE80211_HT_CAP_SM_PS_SHIFT; } - /* if low-level driver supports AP, we also support VLAN */ - if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_AP)) { + /* if low-level driver supports AP, we also support VLAN. + * drivers advertising SW_CRYPTO_CONTROL should enable AP_VLAN + * based on their support to transmit SW encrypted packets. + */ + if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_AP) && + !ieee80211_hw_check(&local->hw, SW_CRYPTO_CONTROL)) { hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_AP_VLAN); hw->wiphy->software_iftypes |= BIT(NL80211_IFTYPE_AP_VLAN); }