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@xxxxxxxxxxxxxx>
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
