| Message ID | 1520576822-1954-1-git-send-email-mpubbise@codeaurora.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
On Fri, 2018-03-09 at 11:57 +0530, mpubbise@codeaurora.org wrote: > From: Manikanta Pubbisetty <mpubbise@codeaurora.org> > > 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: >> From: Manikanta Pubbisetty <mpubbise@codeaurora.org> >> >> 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: > > > 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: >> >> > 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. > It makes sense, I will send out the change by hard-coding only for AP-VLAN interface. -- mkp
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;