Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7b0d857f-90f4-6ee6-2186-01cadeda998d@quicinc.com>
Date:   Sat, 30 Jul 2022 11:17:40 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [RFC] cfg80211: Add link_id to various key operations for MLO
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>
CC:     <linux-wireless@vger.kernel.org>
References: <1657518327-17390-1-git-send-email-quic_vjakkam@quicinc.com>
 <2d90dfefd79fb2d4c942026856ef11ae6b4279cb.camel@sipsolutions.net>
From:   Veerendranath Jakkam <quic_vjakkam@quicinc.com>
In-Reply-To: <2d90dfefd79fb2d4c942026856ef11ae6b4279cb.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 7/14/2022 1:52 AM, Johannes Berg wrote:
>
>> + *	%NL80211_ATTR_MLO_LINK_ID, If %NL80211_ATTR_MLO_LINK_ID is not present
>> + *	the specified key index data should be deleted for all the links.
> really? for all the links seems... complicated?


Thought delete key operation can be optimized in some cases with single 
request to delete group keys

on all MLO links.  I agree that it is not inline with other key 
operations and makes it complicated, dropped this comment in v2 patch.

>
>> --- a/net/wireless/util.c
>> +++ b/net/wireless/util.c
>> @@ -935,13 +935,13 @@ void cfg80211_upload_connect_keys(struct wireless_dev *wdev)
>>   	for (i = 0; i < CFG80211_MAX_WEP_KEYS; i++) {
>>   		if (!wdev->connect_keys->params[i].cipher)
>>   			continue;
>> -		if (rdev_add_key(rdev, dev, i, false, NULL,
>> +		if (rdev_add_key(rdev, dev, -1, i, false, NULL,
>>   				 &wdev->connect_keys->params[i])) {
>>   			netdev_err(dev, "failed to set key %d\n", i);
>>   			continue;
>>   		}
>>   		if (wdev->connect_keys->def == i &&
>> -		    rdev_set_default_key(rdev, dev, i, true, true)) {
>> +		    rdev_set_default_key(rdev, dev, -1, i, true, true)) {
>>   			netdev_err(dev, "failed to set defkey %d\n", i);
>>   			continue;
>>   		}
> Now in this case we probably couldn't have made an MLO connection, can
> we? Or maybe we can with full offload firmware? And then should this
> really be -1 towards the driver?
>
> Actually - these are WEP keys, so no MLO?


yes, MLO connection shouldn't happen in this case since these are WEP keys.

>
>> diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c
>> index a9767bf..3dd0946 100644
>> --- a/net/wireless/wext-compat.c
>> +++ b/net/wireless/wext-compat.c
>> @@ -470,7 +470,7 @@ static int __cfg80211_set_encryption(struct cfg80211_registered_device *rdev,
>>   			    !(rdev->wiphy.flags & WIPHY_FLAG_IBSS_RSN))
>>   				err = -ENOENT;
>>   			else
>> -				err = rdev_del_key(rdev, dev, idx, pairwise,
>> +				err = rdev_del_key(rdev, dev, -1, idx, pairwise,
>>   						   addr);
>>   		}
>>   		wdev->wext.connect.privacy = false;
>
> All of this stuff has me thinking though - now you still need driver
> validation, because on an MLO connection the WEXT handlers could be
> called?! YUCK!
>
> Maybe we should just prevent all of these handlers on interfaces that
> are MLDs, i.e. have valid_links != 0?
>
> I guess that's not really specific to your patch though.
>
> johannes

Yeah, already in most of the cases WEXT specific code is 
skipped/rejected for MLO.

addressed other comments from you and added few checks for WEP and WEXT 
in v2 patch series. Please review.

- veeru