Received-SPF: pass (google.com: domain of linux-wireless+bounces-8769-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <d62d9e76-86ca-497f-bc34-e6dc104be896@quicinc.com>
Date: Tue, 11 Jun 2024 10:01:17 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] wifi: ath11k: fix wrong handling of CCMP256 and GCMP
 ciphers
To: Kalle Valo <kvalo@kernel.org>
CC: <ath11k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
References: <20240605014826.22498-1-quic_bqiang@quicinc.com>
 <87zfrsohea.fsf@kernel.org>
Content-Language: en-US
From: Baochen Qiang <quic_bqiang@quicinc.com>
In-Reply-To: <87zfrsohea.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit


On 6/11/2024 1:07 AM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang@quicinc.com> writes:
> 
>> Currently for CCMP256, GCMP128 and GCMP256 ciphers, in ath11k_install_key()
>> IEEE80211_KEY_FLAG_GENERATE_IV_MGMT is not set. And in ath11k_mac_mgmt_tx_wmi()
>> a length of IEEE80211_CCMP_MIC_LEN is reserved for all ciphers.
>>
>> This results in unexpected management frame drop in case either of above 3 ciphers
>> is used. The reason is, without IEEE80211_KEY_FLAG_GENERATE_IV_MGMT set, mac80211
>> will not generate CCMP/GCMP headers in frame for ath11k. Also MIC length reserved
>> is wrong. Such frame is dropped later by hardware:
>>
>> ath11k_pci 0000:5a:00.0: mac tx mgmt frame, buf id 0
>> ath11k_pci 0000:5a:00.0: mgmt tx compl ev pdev_id 1, desc_id 0, status 1
>>
>> >From user point of view, we have observed very low throughput due to this issue:
>> action frames are all dropped so ADDBA response from DUT never reaches AP. AP
>> can not use aggregation thus throughput is low.
>>
>> Fix this by setting IEEE80211_KEY_FLAG_GENERATE_IV_MGMT flag and by reserving proper
>> MIC length for those ciphers.
>>
>> Tested-on: WCN6855 hw2.0 PCI
>> WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
>> Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.7.0.1-01744-QCAHKSWPL_SILICONZ-1
>>
>> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
>> Reported-by: Yaroslav Isakov <yaroslav.isakov@gmail.com>
>> Tested-by: Yaroslav Isakov <yaroslav.isakov@gmail.com>
>> Closes:
>> https://lore.kernel.org/all/CADS+iDX5=JtJr0apAtAQ02WWBxgOFEv8G063vuGYwDTC8AVZaw@mail.gmail.com
>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
>> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> 
> [...]
> 
>> @@ -5927,7 +5929,10 @@ static int ath11k_mac_mgmt_tx_wmi(struct ath11k *ar, struct ath11k_vif *arvif,
>>  		     ieee80211_is_deauth(hdr->frame_control) ||
>>  		     ieee80211_is_disassoc(hdr->frame_control)) &&
>>  		     ieee80211_has_protected(hdr->frame_control)) {
>> -			skb_put(skb, IEEE80211_CCMP_MIC_LEN);
>> +			WARN_ON(!(skb_cb->flags & ATH11K_SKB_CIPHER_SET));
> 
> Using WARN_ON() in the data path is not advisable as it's not rate
> limited and quite spammy, in the worst case it can lead to kernel
> crashing (I have experienced this even myself). ath11k_warn() is safer
> in this regard so I changed it to this:
> 
> 			if (!(skb_cb->flags & ATH11K_SKB_CIPHER_SET))
> 				ath11k_warn(ab, "WMI management tx frame without ATH11K_SKB_CIPHER_SET");
> 
> Please check:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=aeadb08d7b4acced84a45812f1285c8cd3ed853a\
LGTM, thanks.

>