DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 738F520865
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 0EDB26020A
Subject: Re: [PATCH] {nl,mac}80211: allow 4addr AP operation on crypto
 controlled devices
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org
References: <1542798228-3293-1-git-send-email-mpubbise@codeaurora.org>
 <3ce0f8352354900cd89074bcafe6ac45c29abd78.camel@sipsolutions.net>
From:   Manikanta Pubbisetty <mpubbise@codeaurora.org>
Message-ID: <cad9cfed-92b5-bea1-a1a1-6b9c45d90056@codeaurora.org>
Date:   Mon, 26 Nov 2018 11:09:04 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <3ce0f8352354900cd89074bcafe6ac45c29abd78.camel@sipsolutions.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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