[PATCH v2] ath10k: fix pointer arithmetic error in trace call

[PATCH v2] ath10k: fix pointer arithmetic error in trace call

[Francesco Magliocca]

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),
 					 hw->rx_desc_ops->rx_desc_size - sizeof(u32));
 
 		if (last_msdu)
-- 
2.35.1

Re: [PATCH v2] ath10k: fix pointer arithmetic error in trace call

[Jeff Johnson]

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

Re: [PATCH v2] ath10k: fix pointer arithmetic error in trace call

[Francesco Magliocca]

Hi, I picked (void*) to be conformant with the other examples in htt_rx.c
For example at line 1431:
>    rxd = HTT_RX_BUF_TO_RX_DESC(hw,
>                    (void *)msdu->data - hw->rx_desc_ops->rx_desc_size);

But for me it is ok. Maybe we should fix all the occurrences of this kind.

Greetings,
FM

Il giorno mar 22 feb 2022 alle ore 21:52 Jeff Johnson
<quic_jjohnson@quicinc.com> ha scritto:
>
> 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

Re: [PATCH v2] ath10k: fix pointer arithmetic error in trace call

[Kalle Valo]

Francesco Magliocca <franciman12@gmail.com> writes:

> Hi, I picked (void*) to be conformant with the other examples in htt_rx.c
> For example at line 1431:
>>    rxd = HTT_RX_BUF_TO_RX_DESC(hw,
>>                    (void *)msdu->data - hw->rx_desc_ops->rx_desc_size);
>
> But for me it is ok. Maybe we should fix all the occurrences of this kind.

Yeah, it would be good to fix the void pointer arithmetic in a separate
patch. I have planning to enable -Wpointer-arith in my ath10k-check and
ath11k-check scripts, so patches are very welcome.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2] ath10k: fix pointer arithmetic error in trace call

[Dan Carpenter]

On Thu, Feb 24, 2022 at 09:34:31AM +0200, Kalle Valo wrote:
> Francesco Magliocca <franciman12@gmail.com> writes:
> 
> > Hi, I picked (void*) to be conformant with the other examples in htt_rx.c
> > For example at line 1431:
> >>    rxd = HTT_RX_BUF_TO_RX_DESC(hw,
> >>                    (void *)msdu->data - hw->rx_desc_ops->rx_desc_size);
> >
> > But for me it is ok. Maybe we should fix all the occurrences of this kind.
> 
> Yeah, it would be good to fix the void pointer arithmetic in a separate
> patch. I have planning to enable -Wpointer-arith in my ath10k-check and
> ath11k-check scripts, so patches are very welcome.

Void * casts simplify a lot of code.  Less noise.  More readable.
They're more accurate in a sense because it's not a u8 at all.  The
kernel can't compile with other compilers besides GCC and Clang so why
care about that the C standard hasn't caught up?

What does -Wpointer-arith buy us?

regards,
dan carpenter

Re: [PATCH v2] ath10k: fix pointer arithmetic error in trace call

[Kalle Valo]

Francesco Magliocca <franciman12@gmail.com> 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/
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

49ffac5907a8 ath10k: fix pointer arithmetic error in trace call

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220221122638.7971-1-franciman12@gmail.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Use of void pointer arithmetic?

[Kalle Valo]

(Changing subject, adding Linus and linux-kernel)

Dan Carpenter <dan.carpenter@oracle.com> writes:

> On Thu, Feb 24, 2022 at 09:34:31AM +0200, Kalle Valo wrote:
>> Francesco Magliocca <franciman12@gmail.com> writes:
>> 
>> > Hi, I picked (void*) to be conformant with the other examples in htt_rx.c
>> > For example at line 1431:
>> >>    rxd = HTT_RX_BUF_TO_RX_DESC(hw,
>> >>                    (void *)msdu->data - hw->rx_desc_ops->rx_desc_size);
>> >
>> > But for me it is ok. Maybe we should fix all the occurrences of this kind.
>> 
>> Yeah, it would be good to fix the void pointer arithmetic in a separate
>> patch. I have planning to enable -Wpointer-arith in my ath10k-check and
>> ath11k-check scripts, so patches are very welcome.
>
> Void * casts simplify a lot of code.  Less noise.  More readable.
> They're more accurate in a sense because it's not a u8 at all.  The
> kernel can't compile with other compilers besides GCC and Clang so why
> care about that the C standard hasn't caught up?
>
> What does -Wpointer-arith buy us?

A good question. I have always just thought we should avoid void pointer
arithmetic due to the C standard, but now that you mention it void
pointers can indeed simplify the code. So I'm not so sure anymore.

Any opinions? Is there a kernel wide recommendation for this?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: Use of void pointer arithmetic?

[Johannes Berg]

On Thu, 2022-02-24 at 11:59 +0200, Kalle Valo wrote:
> 
> A good question. I have always just thought we should avoid void pointer
> arithmetic due to the C standard, but now that you mention it void
> pointers can indeed simplify the code. So I'm not so sure anymore.
> 
> Any opinions? Is there a kernel wide recommendation for this?

The kernel only enables it with W=3, which I guess nobody uses anyway
... Originally it came from commit 4a5838ad9d2d ("kbuild: Add extra gcc
checks") with a pointer to

http://marc.info/?l=kernel-janitors&m=129802065918147&w=2

(which is offline right now due to an expired certificate ...)

but setting back my clock it seems to point to

https://lore.kernel.org/all/20110218091716.GA4384@bicker/

but the thread kind of revolves around -Wconversion.


FreeBSD does enable -Wpointer-arith which is why we've been trying to
keep iwlwifi clean as a courtesy to them, but for really Linux-only code
I dunno if there's much point. Although of course that applies also to
FreeBSD ...

johannes

Re: Use of void pointer arithmetic?

[Linus Torvalds]

On Thu, Feb 24, 2022 at 1:59 AM Kalle Valo <kvalo@kernel.org> wrote:
>
> > What does -Wpointer-arith buy us?
>
> A good question. I have always just thought we should avoid void pointer
> arithmetic due to the C standard, but now that you mention it void
> pointers can indeed simplify the code. So I'm not so sure anymore.
>
> Any opinions? Is there a kernel wide recommendation for this?

We consciously use arithmetic on 'void *' in some places, although
it's not exactly _hugely_ common.

It's much more common to turn a pointer into an 'unsigned long' and
then doing basic operations on that.

The advantage of 'void *' is that it does avoid the need to cast the
pointer back.

But at the same time it will never replace the 'cast to an actual
integer type', because the 'void *' arithmetic only does simple
addition and subtraction, and many pointer operations need more (ie
alignment needs to do the bitops).

So I think it's mostly a personal preference. I *do* think that doing
arithmetic on 'void *' is generally superior to doing it the
old-fashioned C way on 'char *' - unless, of course, 'char *' is
simply the native type in question.

So if 'char *' casts (and casting back) is the alternative, then by
all means use 'void *' as a kind of generic and type-independent "byte
pointer". That is how it is meant to be used in the gcc extension.

And no, nobody should ever use -Wpointer-arith on the kernel in
general. Our use of it is not _hugely_ common, but it's does exist,
and it's not really frowned upon.

                    Linus