Hello,
From: Manikanta Pubbisetty <[email protected]>
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
