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

[Alexander Wetzel <alexander@wetzel-home.de>]

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