[RFC] mac80211: advertise supported interface types for sw encryption

From: Manikanta Pubbisetty <redacted>

Extending SW_CRYPTO_CONTROL interface so that drivers can advertise
the interface types on which they can support software encryption.
Driver's job is not done by advertising the supported vif types alone,
they should also return -EOPNOTSUPP from set_key.

Mac80211 will make the fallback decision to sw ecryption based
on the return type of set_key callback and the driver's support for
software encryption.

This change is done with the sole reason of adding the support of
VLANs for drivers which enable SW_CRYPTO_CONTROL(ex:ath10k).

With the current logic, configuring GTKs for specific VLAN groups will
always fail with the drivers enabling SW_CRYPTO_CONTROL. I understand
that the driver can return 1 from set_key to enable software encryption
in mac80211, but GTKs for VLANs are never passed down to the driver.
Since the return value is initialized to -EOPNOTSUPP, with this approach,
we get away with the failure.

Signed-off-by: Manikanta Pubbisetty <redacted>
---
 include/net/mac80211.h |  5 +++++
 net/mac80211/key.c     | 12 +++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 2fd59ed..3df6bee5 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h

@@ -2237,6 +2237,10 @@ enum ieee80211_hw_flags {
  *	supported by HW.
  * @max_nan_de_entries: maximum number of NAN DE functions supported by the
  *	device.
+ * @supp_sw_crypto_iftypes: supported interface types for software crypto,
+ * 	this field is applicable for devices advertising SW_CRYPTO_CONTROL
+ * 	hwflag. Drivers may also set the interface types on which mac80211
+ * 	can fallback to software encryption if set_key returns -EOPNOTSUPP.
  */
 struct ieee80211_hw {
 	struct ieee80211_conf conf;

@@ -2272,6 +2276,7 @@ struct ieee80211_hw {
 	u8 n_cipher_schemes;
 	const struct ieee80211_cipher_scheme *cipher_schemes;
 	u8 max_nan_de_entries;
+	u16 supp_sw_crypto_iftypes;
 };
 
 static inline bool _ieee80211_hw_check(struct ieee80211_hw *hw,

diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index aee05ec..a1011c4 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c

@@ -126,7 +126,7 @@ static void decrease_tailroom_need_count(struct ieee80211_sub_if_data *sdata,
 
 static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
 {
-	struct ieee80211_sub_if_data *sdata;
+	struct ieee80211_sub_if_data *sdata = key->sdata;
 	struct sta_info *sta;
 	int ret = -EOPNOTSUPP;
 

@@ -162,7 +162,6 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
 	if (sta && !sta->uploaded)
 		goto out_unsupported;
 
-	sdata = key->sdata;
 	if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) {
 		/*
 		 * The driver doesn't know anything about VLAN interfaces.

@@ -214,8 +213,15 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
 		/* 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 (ieee80211_hw_check(&key->local->hw, SW_CRYPTO_CONTROL)) {
+			if ((ret == -EOPNOTSUPP) &&
+			    (sdata->local->hw.supp_sw_crypto_iftypes &
+			    (1 << sdata->vif.type)))
+				return 0;
 			return -EINVAL;
+		}
+
 		return 0;
 	default:
 		return -EINVAL;

-- 
2.7.4

On Fri, 2018-03-09 at 11:57 +0530, mpubbise@codeaurora.org wrote:

From: Manikanta Pubbisetty <redacted>

Extending SW_CRYPTO_CONTROL interface so that drivers can advertise
the interface types on which they can support software encryption.
Driver's job is not done by advertising the supported vif types alone,
they should also return -EOPNOTSUPP from set_key.

Mac80211 will make the fallback decision to sw ecryption based
on the return type of set_key callback and the driver's support for
software encryption.

This change is done with the sole reason of adding the support of
VLANs for drivers which enable SW_CRYPTO_CONTROL(ex:ath10k).

With the current logic, configuring GTKs for specific VLAN groups will
always fail with the drivers enabling SW_CRYPTO_CONTROL. I understand
that the driver can return 1 from set_key to enable software encryption
in mac80211, but GTKs for VLANs are never passed down to the driver.
Since the return value is initialized to -EOPNOTSUPP, with this approach,
we get away with the failure.

Is there much value in having this control to start with, rather than
saying it's *always* allowed for AP_VLAN interfaces?

I mean - if the driver wants to support (encryption on) AP_VLAN
interfaces with SW_CRYPTO_CONTROL it basically has to set this to allow
it, which is kinda pointless - it's hard to imagine a driver that wants
to support AP_VLAN only for unencrypted, so if it doesn't support this
it might as well just disable AP_VLAN support entirely.

So IMHO - just get rid of the bitmap and hard-code AP_VLAN.

johannes

On 2018-03-21 13:23, Johannes Berg wrote:

On Fri, 2018-03-09 at 11:57 +0530, mpubbise@codeaurora.org wrote:

quoted

From: Manikanta Pubbisetty <redacted>

Extending SW_CRYPTO_CONTROL interface so that drivers can advertise
the interface types on which they can support software encryption.
Driver's job is not done by advertising the supported vif types alone,
they should also return -EOPNOTSUPP from set_key.

Mac80211 will make the fallback decision to sw ecryption based
on the return type of set_key callback and the driver's support for
software encryption.

This change is done with the sole reason of adding the support of
VLANs for drivers which enable SW_CRYPTO_CONTROL(ex:ath10k).

With the current logic, configuring GTKs for specific VLAN groups will
always fail with the drivers enabling SW_CRYPTO_CONTROL. I understand
that the driver can return 1 from set_key to enable software 
encryption
in mac80211, but GTKs for VLANs are never passed down to the driver.
Since the return value is initialized to -EOPNOTSUPP, with this 
approach,
we get away with the failure.

Is there much value in having this control to start with, rather than
saying it's *always* allowed for AP_VLAN interfaces?

I mean - if the driver wants to support (encryption on) AP_VLAN
interfaces with SW_CRYPTO_CONTROL it basically has to set this to allow
it, which is kinda pointless - it's hard to imagine a driver that wants
to support AP_VLAN only for unencrypted, so if it doesn't support this
it might as well just disable AP_VLAN support entirely.

So IMHO - just get rid of the bitmap and hard-code AP_VLAN.

I agree with you only partially.

Today, I do not see any driver advertising SW_CRYPTO_CONTROL other than 
ath10k. There could be some driver which would want to advertise 
SW_CRYPTO_CONTROL and do not support the software encryption for VLAN 
devices. In that case, hard-coding doesn't seem to solve the problem 
completely right? No?

In some way driver has to advertise that it supports encryption on 
AP_VLANs, No?
Or you meant to say that driver should advertise the support for 
AP_VLANs only if it can support encryption on AP_VLAN devices? If this 
the case, then I could see some code in ieee80211_register_hw which says 
this,

         /* if low-level driver supports AP, we also support VLAN */
         if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_AP)) {
                 hw->wiphy->interface_modes |= 
BIT(NL80211_IFTYPE_AP_VLAN);
                 hw->wiphy->software_iftypes |= 
BIT(NL80211_IFTYPE_AP_VLAN);
         }

Please correct if I misinterpreted your comment.


--
mkp

On Thu, 2018-03-22 at 11:51 +0530, mpubbise@codeaurora.org wrote:

quoted

So IMHO - just get rid of the bitmap and hard-code AP_VLAN.

I agree with you only partially.

Today, I do not see any driver advertising SW_CRYPTO_CONTROL other than 
ath10k. There could be some driver which would want to advertise 
SW_CRYPTO_CONTROL and do not support the software encryption for VLAN 
devices. In that case, hard-coding doesn't seem to solve the problem 
completely right? No?

Well, my point is that such a hypothetical driver is completely
irrelevant because it doesn't make any sense to have this behaviour -
it would mean it cannot support AP_VLAN with encryption, so it might as
well not support AP_VLAN at all.

Or you meant to say that driver should advertise the support for 
AP_VLANs only if it can support encryption on AP_VLAN devices? 

Right.

If this 
the case, then I could see some code in ieee80211_register_hw which says 
this,

         /* if low-level driver supports AP, we also support VLAN */
         if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_AP)) {
                 hw->wiphy->interface_modes |= 
BIT(NL80211_IFTYPE_AP_VLAN);
                 hw->wiphy->software_iftypes |= 
BIT(NL80211_IFTYPE_AP_VLAN);
         }

Yes, but if such a driver comes along we can change this.

johannes

On 2018-03-22 13:36, Johannes Berg wrote:

On Thu, 2018-03-22 at 11:51 +0530, mpubbise@codeaurora.org wrote:

quoted

quoted

So IMHO - just get rid of the bitmap and hard-code AP_VLAN.

I agree with you only partially.

Today, I do not see any driver advertising SW_CRYPTO_CONTROL other 
than
ath10k. There could be some driver which would want to advertise
SW_CRYPTO_CONTROL and do not support the software encryption for VLAN
devices. In that case, hard-coding doesn't seem to solve the problem
completely right? No?

Well, my point is that such a hypothetical driver is completely
irrelevant because it doesn't make any sense to have this behaviour -
it would mean it cannot support AP_VLAN with encryption, so it might as
well not support AP_VLAN at all.

quoted

Or you meant to say that driver should advertise the support for
AP_VLANs only if it can support encryption on AP_VLAN devices?

Right.

quoted

If this
the case, then I could see some code in ieee80211_register_hw which 
says
this,

         /* if low-level driver supports AP, we also support VLAN */
         if (local->hw.wiphy->interface_modes & 
BIT(NL80211_IFTYPE_AP)) {
                 hw->wiphy->interface_modes |=
BIT(NL80211_IFTYPE_AP_VLAN);
                 hw->wiphy->software_iftypes |=
BIT(NL80211_IFTYPE_AP_VLAN);
         }

Yes, but if such a driver comes along we can change this.

It makes sense, I will send out the change by hard-coding only for 
AP-VLAN interface.

--
mkp