Received-SPF: pass (google.com: domain of linux-wireless+bounces-4265-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <a79de0d7-f630-4a11-8cde-3444a6b93ee1@quicinc.com>
Date: Thu, 29 Feb 2024 12:10:14 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/3] [v2] wifi: ath11k: use ath11k_mac_get_ar_by_pdev_id()
 consistently
Content-Language: en-US
To: Dmitry Antipov <dmantipov@yandex.ru>
CC: Baochen Qiang <quic_bqiang@quicinc.com>, Kalle Valo <kvalo@kernel.org>,
        <ath11k@lists.infradead.org>, <linux-wireless@vger.kernel.org>,
        <lvc-project@linuxtesting.org>
References: <788f1df8-64e1-4b3c-ae8e-00c67be1c3de@quicinc.com>
 <20240229084031.51957-1-dmantipov@yandex.ru>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20240229084031.51957-1-dmantipov@yandex.ru>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 2/29/2024 12:40 AM, Dmitry Antipov wrote:
> Since 'ath11k_mac_get_ar_by_pdev_id()' can return NULL, check
> the return value in 'ath11k_wmi_tlv_rssi_chain_parse()' as well
> as in 'ath11k_wmi_tlv_fw_stats_data_parse()', and return -EINVAL
> in case of error. Compile tested only.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v2: aggregate to the series

In the future please don't use In-Reply-To: when submitting a new
version of a patch/series. Each version should start its own thread.
Having In-Reference-To: is OK

> ---
>  drivers/net/wireless/ath/ath11k/wmi.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c
> index 34ab9631ff36..2d93e4e78a37 100644
> --- a/drivers/net/wireless/ath/ath11k/wmi.c
> +++ b/drivers/net/wireless/ath/ath11k/wmi.c
> @@ -6498,6 +6498,12 @@ static int ath11k_wmi_tlv_rssi_chain_parse(struct ath11k_base *ab,
>  	rcu_read_lock();
>  
>  	ar = ath11k_mac_get_ar_by_pdev_id(ab, ev->pdev_id);
> +	if (!ar) {
> +		ath11k_warn(ab, "%s: invalid pdev_id %d\n",
> +			    __func__, ev->pdev_id);

it is currently not the convention to use __func__ in ath11k logs.

there are currently 6 instances where the NULL check is present, and
each has a custom log message

I personally would be ok having this new format, but want to get Kalle's
approval for that. But if we do that I'd also want a follow-up patch to
modify those 6 existing logs.

FWIW in all of ath11k there are only 3 instances of __func__:
mac.c:                     "%s: rsn ie found\n", __func__);
mac.c:                     "%s: wpa ie found\n", __func__);
wmi.c:                     __func__);

> +		ret = -EINVAL;
> +		goto exit;
> +	}
>  	stats->stats_id = WMI_REQUEST_RSSI_PER_CHAIN_STAT;
>  
>  	ath11k_dbg(ab, ATH11K_DBG_WMI,
> @@ -6570,6 +6576,12 @@ static int ath11k_wmi_tlv_fw_stats_data_parse(struct ath11k_base *ab,
>  	rcu_read_lock();
>  
>  	ar = ath11k_mac_get_ar_by_pdev_id(ab, ev->pdev_id);
> +	if (!ar) {
> +		ath11k_warn(ab, "%s: invalid pdev_id %d\n",
> +			    __func__, ev->pdev_id);
> +		ret = -EINVAL;
> +		goto exit;
> +	}
>  
>  	for (i = 0; i < ev->num_pdev_stats; i++) {
>  		const struct wmi_pdev_stats *src;

The actual logic is correct.

/jeff