Re: [PATCH] {nl,mac}80211: allow 4addr AP operation on crypto controlled devices

[Manikanta Pubbisetty <mpubbise@codeaurora.org>]

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