{nl,mac}80211: allow 4addr AP operation on crypto controlled devices
diff mbox series

Message ID 1542798228-3293-1-git-send-email-mpubbise@codeaurora.org
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series
  • {nl,mac}80211: allow 4addr AP operation on crypto controlled devices
Related show

Commit Message

Manikanta Pubbisetty Nov. 21, 2018, 11:03 a.m. UTC
As per the current design, for sw crypto controlled devices, it is
the device which has to advertise the support for AP/VLAN iftype
based on it's capability to tranmsit packets encrypted in software
(In VLAN functionality, group traffic generated for a specific
VLAN group is always encrypted in software). Commit db3bdcb9c3ff
("mac80211: allow AP_VLAN operation on crypto controlled devices")
has introduced this change.

Since 4addr AP operation also uses AP/VLAN iftype, this conditional
way of advertising AP/VLAN support has broken 4addr AP mode operation on
crypto controlled devices which do not support VLAN functionality.

For example:
In the case of ath10k driver, not all firmwares have support for VLAN
functionality but all can support 4addr AP operation. Because AP/VLAN
support is not advertised for these devices, 4addr AP operations are
also blocked.

Fix this by allowing 4addr opertion on devices which do not advertise
AP/VLAN iftype but which can support 4addr operation (the desicion is
taken based on the wiphy flag WIPHY_FLAG_4ADDR_AP).

Fixes: Commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on
crypto controlled devices")
Signed-off-by: Manikanta Pubbisetty <mpubbise@codeaurora.org>
---
 include/net/cfg80211.h |  3 ++-
 net/mac80211/util.c    |  4 +++-
 net/wireless/core.c    |  9 +++++++--
 net/wireless/nl80211.c | 10 ++++++++--
 4 files changed, 20 insertions(+), 6 deletions(-)

Comments

Johannes Berg Nov. 24, 2018, 6:48 p.m. UTC | #1
This looks good to me, just a few nits below

On Wed, 2018-11-21 at 16:33 +0530, Manikanta Pubbisetty wrote:
> As per the current design, for sw crypto controlled devices, it is
> the device which has to advertise the support for AP/VLAN iftype
> based on it's capability to tranmsit packets encrypted in software
> (In VLAN functionality, group traffic generated for a specific
> VLAN group is always encrypted in software). Commit db3bdcb9c3ff
> ("mac80211: allow AP_VLAN operation on crypto controlled devices")
> has introduced this change.
> 
> Since 4addr AP operation also uses AP/VLAN iftype, this conditional
> way of advertising AP/VLAN support has broken 4addr AP mode operation on
> crypto controlled devices which do not support VLAN functionality.
> 
> For example:
> In the case of ath10k driver, not all firmwares have support for VLAN
> functionality but all can support 4addr AP operation. Because AP/VLAN
> support is not advertised for these devices, 4addr AP operations are
> also blocked.
> 
> Fix this by allowing 4addr opertion on devices which do not advertise

operation

> AP/VLAN iftype but which can support 4addr operation (the desicion is

decision

> taken based on the wiphy flag WIPHY_FLAG_4ADDR_AP).

> Fixes: Commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on
> crypto controlled devices")

I think it'd be better not to wrap this

> --- a/net/wireless/core.c
> +++ b/net/wireless/core.c
> @@ -1394,8 +1394,13 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
>  		}
>  		break;
>  	case NETDEV_PRE_UP:
> -		if (!(wdev->wiphy->interface_modes & BIT(wdev->iftype)))
> -			return notifier_from_errno(-EOPNOTSUPP);
> +		if (!(wdev->wiphy->interface_modes & BIT(wdev->iftype))) {
> +			if (!(wdev->iftype == NL80211_IFTYPE_AP_VLAN &&
> +			      rdev->wiphy.flags & WIPHY_FLAG_4ADDR_AP &&
> +			      wdev->use_4addr))
> +				return notifier_from_errno(-EOPNOTSUPP);
> +		}

Maybe that could be combined into one condition? It's kinda hard to read
either way though. I was thinking of making it positive, but then the
ERFKILL

>  		if (rfkill_blocked(rdev->rfkill))
>  			return notifier_from_errno(-ERFKILL);

would have to be _before_ it, and I'm not sure we want to change that.

So maybe make it

if (!(interface_modes & BIT() ||
      (iftype == AP_VLAN && use_4addr && wiphy.flags & 4ADDR_AP)))
	return ...(-EOPNOTSUPP);

instead?

I feel that's still easier to read than the double conditional with both
things being negated.

> +++ b/net/wireless/nl80211.c
> @@ -3383,8 +3383,7 @@ static int nl80211_new_interface(struct sk_buff *skb, struct genl_info *info)
>  	if (info->attrs[NL80211_ATTR_IFTYPE])
>  		type = nla_get_u32(info->attrs[NL80211_ATTR_IFTYPE]);
>  
> -	if (!rdev->ops->add_virtual_intf ||
> -	    !(rdev->wiphy.interface_modes & (1 << type)))
> +	if (!rdev->ops->add_virtual_intf)
>  		return -EOPNOTSUPP;
>  
>  	if ((type == NL80211_IFTYPE_P2P_DEVICE || type == NL80211_IFTYPE_NAN ||
> @@ -3403,6 +3402,13 @@ static int nl80211_new_interface(struct sk_buff *skb, struct genl_info *info)
>  			return err;
>  	}
>  
> +	if (!(rdev->wiphy.interface_modes & (1 << type))) {
> +		if (!(type == NL80211_IFTYPE_AP_VLAN &&
> +		      rdev->wiphy.flags & WIPHY_FLAG_4ADDR_AP &&
> +		      params.use_4addr))
> +			return -EOPNOTSUPP;
> +	}

I'm not sure you should insert this further down, rather tahn at the
place where the check was before? Why not just insert this part directly
after the piece you modified above?

johannes
Manikanta Pubbisetty Nov. 26, 2018, 5:39 a.m. UTC | #2
On 11/25/2018 12:18 AM, Johannes Berg wrote:

> This looks good to me, just a few nits below
>
> On Wed, 2018-11-21 at 16:33 +0530, Manikanta Pubbisetty wrote:
>> As per the current design, for sw crypto controlled devices, it is
>> the device which has to advertise the support for AP/VLAN iftype
>> based on it's capability to tranmsit packets encrypted in software
>> (In VLAN functionality, group traffic generated for a specific
>> VLAN group is always encrypted in software). Commit db3bdcb9c3ff
>> ("mac80211: allow AP_VLAN operation on crypto controlled devices")
>> has introduced this change.
>>
>> Since 4addr AP operation also uses AP/VLAN iftype, this conditional
>> way of advertising AP/VLAN support has broken 4addr AP mode operation on
>> crypto controlled devices which do not support VLAN functionality.
>>
>> For example:
>> In the case of ath10k driver, not all firmwares have support for VLAN
>> functionality but all can support 4addr AP operation. Because AP/VLAN
>> support is not advertised for these devices, 4addr AP operations are
>> also blocked.
>>
>> Fix this by allowing 4addr opertion on devices which do not advertise
> operation
>
>> AP/VLAN iftype but which can support 4addr operation (the desicion is
> decision
>
>> taken based on the wiphy flag WIPHY_FLAG_4ADDR_AP).
>> Fixes: Commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on
>> crypto controlled devices")
> I think it'd be better not to wrap this

Sure, I'll fix them all in the next version.

>> --- a/net/wireless/core.c
>> +++ b/net/wireless/core.c
>> @@ -1394,8 +1394,13 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
>>   		}
>>   		break;
>>   	case NETDEV_PRE_UP:
>> -		if (!(wdev->wiphy->interface_modes & BIT(wdev->iftype)))
>> -			return notifier_from_errno(-EOPNOTSUPP);
>> +		if (!(wdev->wiphy->interface_modes & BIT(wdev->iftype))) {
>> +			if (!(wdev->iftype == NL80211_IFTYPE_AP_VLAN &&
>> +			      rdev->wiphy.flags & WIPHY_FLAG_4ADDR_AP &&
>> +			      wdev->use_4addr))
>> +				return notifier_from_errno(-EOPNOTSUPP);
>> +		}
> Maybe that could be combined into one condition? It's kinda hard to read
> either way though. I was thinking of making it positive, but then the
> ERFKILL
>
>>   		if (rfkill_blocked(rdev->rfkill))
>>   			return notifier_from_errno(-ERFKILL);
> would have to be _before_ it, and I'm not sure we want to change that.
>
> So maybe make it
>
> if (!(interface_modes & BIT() ||
>        (iftype == AP_VLAN && use_4addr && wiphy.flags & 4ADDR_AP)))
> 	return ...(-EOPNOTSUPP);
>
> instead?
>
> I feel that's still easier to read than the double conditional with both
> things being negated.

Yeah, It makes sense; I'll change it.

>
>> +++ b/net/wireless/nl80211.c
>> @@ -3383,8 +3383,7 @@ static int nl80211_new_interface(struct sk_buff *skb, struct genl_info *info)
>>   	if (info->attrs[NL80211_ATTR_IFTYPE])
>>   		type = nla_get_u32(info->attrs[NL80211_ATTR_IFTYPE]);
>>   
>> -	if (!rdev->ops->add_virtual_intf ||
>> -	    !(rdev->wiphy.interface_modes & (1 << type)))
>> +	if (!rdev->ops->add_virtual_intf)
>>   		return -EOPNOTSUPP;
>>   
>>   	if ((type == NL80211_IFTYPE_P2P_DEVICE || type == NL80211_IFTYPE_NAN ||
>> @@ -3403,6 +3402,13 @@ static int nl80211_new_interface(struct sk_buff *skb, struct genl_info *info)
>>   			return err;
>>   	}
>>   
>> +	if (!(rdev->wiphy.interface_modes & (1 << type))) {
>> +		if (!(type == NL80211_IFTYPE_AP_VLAN &&
>> +		      rdev->wiphy.flags & WIPHY_FLAG_4ADDR_AP &&
>> +		      params.use_4addr))
>> +			return -EOPNOTSUPP;
>> +	}
> I'm not sure you should insert this further down, rather tahn at the
> place where the check was before? Why not just insert this part directly
> after the piece you modified above?

I had to move it because use_4addr is only known after the NL_ATTR_4ADDR 
check.

-Manikanta
Hauke Mehrtens Feb. 26, 2019, 10:01 p.m. UTC | #3
On 11/21/18 12:03 PM, Manikanta Pubbisetty wrote:
> As per the current design, for sw crypto controlled devices, it is
> the device which has to advertise the support for AP/VLAN iftype
> based on it's capability to tranmsit packets encrypted in software
> (In VLAN functionality, group traffic generated for a specific
> VLAN group is always encrypted in software). Commit db3bdcb9c3ff
> ("mac80211: allow AP_VLAN operation on crypto controlled devices")
> has introduced this change.
> 
> Since 4addr AP operation also uses AP/VLAN iftype, this conditional
> way of advertising AP/VLAN support has broken 4addr AP mode operation on
> crypto controlled devices which do not support VLAN functionality.
> 
> For example:
> In the case of ath10k driver, not all firmwares have support for VLAN
> functionality but all can support 4addr AP operation. Because AP/VLAN
> support is not advertised for these devices, 4addr AP operations are
> also blocked.
> 
> Fix this by allowing 4addr opertion on devices which do not advertise
> AP/VLAN iftype but which can support 4addr operation (the desicion is
> taken based on the wiphy flag WIPHY_FLAG_4ADDR_AP).
> 
> Fixes: Commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on
> crypto controlled devices")
> Signed-off-by: Manikanta Pubbisetty <mpubbise@codeaurora.org>
> ---
>  include/net/cfg80211.h |  3 ++-
>  net/mac80211/util.c    |  4 +++-
>  net/wireless/core.c    |  9 +++++++--
>  net/wireless/nl80211.c | 10 ++++++++--
>  4 files changed, 20 insertions(+), 6 deletions(-)
> 
Hi Manikanta,

Will you send an updated version of this patch to mainline?

Multiple people ran into this problem in OpenWrt and it would be nice if
such a fix would be included in upstream kernel.

See here a pull request:
https://github.com/openwrt/openwrt/pull/1640

Hauke

Patch
diff mbox series

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ede7fcd..666e5a3 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3708,7 +3708,8 @@  struct cfg80211_ops {
  *	on wiphy_new(), but can be changed by the driver if it has a good
  *	reason to override the default
  * @WIPHY_FLAG_4ADDR_AP: supports 4addr mode even on AP (with a single station
- *	on a VLAN interface)
+ *	on a VLAN interface). This flag also serves an extra purpose of
+ *	supporting 4ADDR AP mode on devices which do not support AP/VLAN iftype.
  * @WIPHY_FLAG_4ADDR_STATION: supports 4addr mode even as a station
  * @WIPHY_FLAG_CONTROL_PORT_PROTOCOL: This device supports setting the
  *	control port protocol ethertype. The device also honours the
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index bec4243..6a8571d 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -3643,7 +3643,9 @@  int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 	}
 
 	/* Always allow software iftypes */
-	if (local->hw.wiphy->software_iftypes & BIT(iftype)) {
+	if (local->hw.wiphy->software_iftypes & BIT(iftype) ||
+	    (iftype == NL80211_IFTYPE_AP_VLAN &&
+	     local->hw.wiphy->flags & WIPHY_FLAG_4ADDR_AP)) {
 		if (radar_detect)
 			return -EINVAL;
 		return 0;
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 623dfe5..93085fb 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -1394,8 +1394,13 @@  static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 		}
 		break;
 	case NETDEV_PRE_UP:
-		if (!(wdev->wiphy->interface_modes & BIT(wdev->iftype)))
-			return notifier_from_errno(-EOPNOTSUPP);
+		if (!(wdev->wiphy->interface_modes & BIT(wdev->iftype))) {
+			if (!(wdev->iftype == NL80211_IFTYPE_AP_VLAN &&
+			      rdev->wiphy.flags & WIPHY_FLAG_4ADDR_AP &&
+			      wdev->use_4addr))
+				return notifier_from_errno(-EOPNOTSUPP);
+		}
+
 		if (rfkill_blocked(rdev->rfkill))
 			return notifier_from_errno(-ERFKILL);
 		break;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index e20329b..2863d9a 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -3383,8 +3383,7 @@  static int nl80211_new_interface(struct sk_buff *skb, struct genl_info *info)
 	if (info->attrs[NL80211_ATTR_IFTYPE])
 		type = nla_get_u32(info->attrs[NL80211_ATTR_IFTYPE]);
 
-	if (!rdev->ops->add_virtual_intf ||
-	    !(rdev->wiphy.interface_modes & (1 << type)))
+	if (!rdev->ops->add_virtual_intf)
 		return -EOPNOTSUPP;
 
 	if ((type == NL80211_IFTYPE_P2P_DEVICE || type == NL80211_IFTYPE_NAN ||
@@ -3403,6 +3402,13 @@  static int nl80211_new_interface(struct sk_buff *skb, struct genl_info *info)
 			return err;
 	}
 
+	if (!(rdev->wiphy.interface_modes & (1 << type))) {
+		if (!(type == NL80211_IFTYPE_AP_VLAN &&
+		      rdev->wiphy.flags & WIPHY_FLAG_4ADDR_AP &&
+		      params.use_4addr))
+			return -EOPNOTSUPP;
+	}
+
 	err = nl80211_parse_mon_options(rdev, type, info, &params);
 	if (err < 0)
 		return err;