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: <e32de43c-e00a-5766-e988-fe192d36d719@quicinc.com>
Date:   Tue, 22 Feb 2022 12:52:27 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
Subject: Re: [PATCH v2] ath10k: fix pointer arithmetic error in trace call
Content-Language: en-US
To:     Francesco Magliocca <franciman12@gmail.com>,
        <ath10k@lists.infradead.org>
CC:     <dan.carpenter@oracle.com>, <rmanohar@qti.qualcomm.com>,
        <linux-wireless@vger.kernel.org>
References: <20220221122638.7971-1-franciman12@gmail.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20220221122638.7971-1-franciman12@gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/21/2022 4:26 AM, Francesco Magliocca wrote:
> Reading through the commit history, it looks like
> there is no special need why we must skip the first 4 bytes
> in this trace call:
> 
> trace_ath10k_htt_rx_desc(ar, (void*)rx_desc + sizeof(u32),
> 			 hw->rx_desc_ops->rx_desc_size - sizeof(u32));
> 
> found in the function ath10k_htt_rx_amsdu_pop in the file htt_rx.c
> 
> i think the original author
> (who is also the one who added rx_desc tracing capabilities
> in a0883cf7e75a) just wanted to trace the rx_desc contents,
> ignoring the fw_rx_desc_base info field
> (which is the part being skipped over).
> But the trace_ath10k_htt_rx_desc later added
> don't care about skipping it, so it may be good
> to uniform this call to the others in the file.
> But this would change the output of the trace and
> thus it may be a problem for tools that rely on it.
> Therefore I propose until further discussion
> to just keep it as it is and just fix the pointer arithmetic bug.
> 
> Add missing void* cast to rx descriptor pointer in order to
> properly skip the initial 4 bytes of the rx descriptor
> when passing it to trace_ath10k_htt_rx_desc trace function.
> 
> This fixes the pointer arithmetic error detected
> by Dan Carpenter's static analysis tool.
> 
> Fixes: 6bae9de622d3 ("ath10k: abstract htt_rx_desc structure")
> 
> Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00157-QCARMSWPZ-1
> 
> Signed-off-by: Francesco Magliocca <franciman12@gmail.com>
> Link: https://lore.kernel.org/ath10k/20220201130900.GD22458@kili/
> ---
>   drivers/net/wireless/ath/ath10k/htt_rx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
> index 9ad64ca84beb..e01efcd2ce06 100644
> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
> @@ -429,7 +429,7 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
>   				RX_MSDU_END_INFO0_LAST_MSDU;
>   
>   		/* FIXME: why are we skipping the first part of the rx_desc? */
> -		trace_ath10k_htt_rx_desc(ar, rx_desc + sizeof(u32),
> +		trace_ath10k_htt_rx_desc(ar, (void*)rx_desc + sizeof(u32),

since void pointer arithmetic is undefined in C99 would it be "better" 
to cast as u8 *? I realize that gcc has an extension to support this 
[1], but this usage will cause a warning when -Wpointer-arith is used.

[1] https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html