Re: [PATCH] mac80211: Honor SW_CRYPTO_CONTROL in AP VLAN mode

[Johannes Berg <johannes@sipsolutions.net>]

On Mon, 2019-04-08 at 21:58 +0200, Alexander Wetzel wrote:

> Here how I understand the situation:
> 
> The intent of db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on crypto 
> controlled devices") was, to allow devices setting SW_CRYPTO_CONTROL to 
> still support AP_VLAN group keys.

Yes, I think so.

> It unintentionally allowed more than it should for that...

I guess you're going to explain that in the text below :-)

> Prior to the commit db3bdcb9c3ff installing a group key when in AP_VLAN 
> mode could not work. Mac80211 was initializing ret to -EOPNOTSUPP and 
> then jumped to "out_unsupported" when the interface was in AP_VLAN mode 
> without trying to install the key and giving the driver a chance to 
> allow SW crypto. Software fallback was then blocked by ret != 1 with 
> SW_CRYPTO_CONTROL set by the driver.

Right.

> But any driver setting NL80211_IFTYPE_AP_VLAN already has confirmed to 
> be fine with SW crypto for group keys, since these CAN only be handled 
> with SW crypto currently. 

Well, NL80211_IFTYPE_AP_VLAN was set by mac80211 at the time still. Said
commit also changed that to only set it if SW_CRYPTO_CONTROL isn't set.

But yes, with the commit I agree - the driver is now setting
NL80211_IFTYPE_AP_VLAN so it's its own problem if something's wrong.

> The net effect was that ath10k - the only 
> driver in tree setting SW_CRYPTO_CONTROL and also NL80211_IFTYPE_AP - 
> was not able to send out frames encrypted with the group key at all.

I don't think I follow here. Before the commit in question, ath10k
would've rejected the key and not been able to. After the commit, and of
course also after 4920ce3bf7e0d ("ath10k: add dynamic vlan support")
(which you linked to below) ath10k again sets the NL80211_IFTYPE_AP_VLAN
bit under certain conditions, and thus *can* do software-crypto of such
frames.

> Now the fix for that was commit db3bdcb9c3ff. Unfortunately this went 
> too far:
> We now not only allows fallback to SW crypto for group keys (which the 
> driver allowed by setting NL80211_IFTYPE_AP_VLAN) but for ANY keys as 
> long as the interface is in AP_VLAN mode.

Aha, now I follow.

> So if for some reasons ath10k is not able to install a pairwise key 
> mac80211 now incorrectly allows the fallback to SW crypto...

Indeed, I agree.

> This patch here tries to fix that and excludes pairwise keys from the 
> special AP_VLAN handling again (but keeps it for group keys).

Right, ok, makes sense now. I'll probably edit the commit message, but
yeah, I can follow your patch now.

Thanks,
johannes