Re: [PATCH] mac80211: allow AP_VLAN operation on crypto controlled devices

[Sebastian Gottschall <s.gottschall@newmedia-net.de>]

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
>