| Message ID | 20220221122638.7971-1-franciman12@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 49ffac5907a8ff30c2cfc6ff9d56fe5c81abb059 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] ath10k: fix pointer arithmetic error in trace call | expand |
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
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
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.
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
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
(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?
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
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
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)
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(-)