2023-06-02 04:57:21

by Shiji Yang

[permalink] [raw]
Subject: [PATCH] wifi: ath: add struct_group for struct ath_cycle_counters

From: Shiji Yang <[email protected]>

Add a struct_group to around all members in struct ath_cycle_counters.
It can help the compiler detect the intended bounds of the memcpy() and
memset().

This patch fixes the following build warning:

In function 'fortify_memset_chk',
inlined from 'ath9k_ps_wakeup' at drivers/net/wireless/ath/ath9k/main.c:140:3:
./include/linux/fortify-string.h:314:25: error: call to '__write_overflow_field' declared with attribute warning:
detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
314 | __write_overflow_field(p_size_field, size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Shiji Yang <[email protected]>
---
More discussion on: https://github.com/openwrt/openwrt/pull/12764
---
drivers/net/wireless/ath/ath.h | 10 ++++++----
drivers/net/wireless/ath/ath5k/ani.c | 2 +-
drivers/net/wireless/ath/ath5k/base.c | 4 ++--
drivers/net/wireless/ath/ath5k/mac80211-ops.c | 2 +-
drivers/net/wireless/ath/ath9k/link.c | 2 +-
drivers/net/wireless/ath/ath9k/main.c | 4 ++--
drivers/net/wireless/ath/hw.c | 2 +-
7 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index f02a308a9..4b42e401a 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -43,10 +43,12 @@ struct ath_ani {
};

struct ath_cycle_counters {
- u32 cycles;
- u32 rx_busy;
- u32 rx_frame;
- u32 tx_frame;
+ struct_group(cnts,
+ u32 cycles;
+ u32 rx_busy;
+ u32 rx_frame;
+ u32 tx_frame;
+ );
};

enum ath_device_state {
diff --git a/drivers/net/wireless/ath/ath5k/ani.c b/drivers/net/wireless/ath/ath5k/ani.c
index 850c608b4..fa95f0f0f 100644
--- a/drivers/net/wireless/ath/ath5k/ani.c
+++ b/drivers/net/wireless/ath/ath5k/ani.c
@@ -379,7 +379,7 @@ ath5k_hw_ani_get_listen_time(struct ath5k_hw *ah, struct ath5k_ani_state *as)
spin_lock_bh(&common->cc_lock);

ath_hw_cycle_counters_update(common);
- memcpy(&as->last_cc, &common->cc_ani, sizeof(as->last_cc));
+ memcpy(&as->last_cc.cnts, &common->cc_ani.cnts, sizeof(as->last_cc.cnts));

/* clears common->cc_ani */
listen = ath_hw_get_listen_time(common);
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index c59c14483..efe072e7a 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -2985,8 +2985,8 @@ ath5k_reset(struct ath5k_hw *ah, struct ieee80211_channel *chan,
memset(&ah->survey, 0, sizeof(ah->survey));
spin_lock_bh(&common->cc_lock);
ath_hw_cycle_counters_update(common);
- memset(&common->cc_survey, 0, sizeof(common->cc_survey));
- memset(&common->cc_ani, 0, sizeof(common->cc_ani));
+ memset(&common->cc_survey.cnts, 0, sizeof(common->cc_survey.cnts));
+ memset(&common->cc_ani.cnts, 0, sizeof(common->cc_ani.cnts));
spin_unlock_bh(&common->cc_lock);

/*
diff --git a/drivers/net/wireless/ath/ath5k/mac80211-ops.c b/drivers/net/wireless/ath/ath5k/mac80211-ops.c
index 11ed30d6b..eb62115b1 100644
--- a/drivers/net/wireless/ath/ath5k/mac80211-ops.c
+++ b/drivers/net/wireless/ath/ath5k/mac80211-ops.c
@@ -665,7 +665,7 @@ ath5k_get_survey(struct ieee80211_hw *hw, int idx, struct survey_info *survey)
ah->survey.time_rx += cc->rx_frame / div;
ah->survey.time_tx += cc->tx_frame / div;
}
- memset(cc, 0, sizeof(*cc));
+ memset(&cc->cnts, 0, sizeof(cc->cnts));
spin_unlock_bh(&common->cc_lock);

memcpy(survey, &ah->survey, sizeof(*survey));
diff --git a/drivers/net/wireless/ath/ath9k/link.c b/drivers/net/wireless/ath/ath9k/link.c
index 9d84003db..0d557e6b6 100644
--- a/drivers/net/wireless/ath/ath9k/link.c
+++ b/drivers/net/wireless/ath/ath9k/link.c
@@ -536,7 +536,7 @@ int ath_update_survey_stats(struct ath_softc *sc)
if (cc->cycles > 0)
ret = cc->rx_busy * 100 / cc->cycles;

- memset(cc, 0, sizeof(*cc));
+ memset(&cc->cnts, 0, sizeof(cc->cnts));

ath_update_survey_nf(sc, pos);

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index a4197c14f..8608a29a1 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -135,8 +135,8 @@ void ath9k_ps_wakeup(struct ath_softc *sc)
if (power_mode != ATH9K_PM_AWAKE) {
spin_lock(&common->cc_lock);
ath_hw_cycle_counters_update(common);
- memset(&common->cc_survey, 0, sizeof(common->cc_survey));
- memset(&common->cc_ani, 0, sizeof(common->cc_ani));
+ memset(&common->cc_survey.cnts, 0, sizeof(common->cc_survey.cnts));
+ memset(&common->cc_ani.cnts, 0, sizeof(common->cc_ani.cnts));
spin_unlock(&common->cc_lock);
}

diff --git a/drivers/net/wireless/ath/hw.c b/drivers/net/wireless/ath/hw.c
index 85955572a..be8bfed9d 100644
--- a/drivers/net/wireless/ath/hw.c
+++ b/drivers/net/wireless/ath/hw.c
@@ -183,7 +183,7 @@ int32_t ath_hw_get_listen_time(struct ath_common *common)
listen_time = (cc->cycles - cc->rx_frame - cc->tx_frame) /
(common->clockrate * 1000);

- memset(cc, 0, sizeof(*cc));
+ memset(&cc->cnts, 0, sizeof(cc->cnts));

return listen_time;
}
--
2.30.2



2023-06-02 05:05:07

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] wifi: ath: add struct_group for struct ath_cycle_counters

On 02. 06. 23, 6:36, Shiji Yang wrote:
> From: Shiji Yang <[email protected]>
>
> Add a struct_group to around all members in struct ath_cycle_counters.
> It can help the compiler detect the intended bounds of the memcpy() and
> memset().
>
> This patch fixes the following build warning:
>
> In function 'fortify_memset_chk',
> inlined from 'ath9k_ps_wakeup' at drivers/net/wireless/ath/ath9k/main.c:140:3:
> ./include/linux/fortify-string.h:314:25: error: call to '__write_overflow_field' declared with attribute warning:
> detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
> 314 | __write_overflow_field(p_size_field, size);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>

Hi,

what compiler/version is this with?

> Signed-off-by: Shiji Yang <[email protected]>
> ---
> More discussion on: https://github.com/openwrt/openwrt/pull/12764

No "__write_overflow_field" there. Is this the right link?

> ---
> drivers/net/wireless/ath/ath.h | 10 ++++++----
> drivers/net/wireless/ath/ath5k/ani.c | 2 +-
> drivers/net/wireless/ath/ath5k/base.c | 4 ++--
> drivers/net/wireless/ath/ath5k/mac80211-ops.c | 2 +-
> drivers/net/wireless/ath/ath9k/link.c | 2 +-
> drivers/net/wireless/ath/ath9k/main.c | 4 ++--
> drivers/net/wireless/ath/hw.c | 2 +-
> 7 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
> index f02a308a9..4b42e401a 100644
> --- a/drivers/net/wireless/ath/ath.h
> +++ b/drivers/net/wireless/ath/ath.h
> @@ -43,10 +43,12 @@ struct ath_ani {
> };
>
> struct ath_cycle_counters {
> - u32 cycles;
> - u32 rx_busy;
> - u32 rx_frame;
> - u32 tx_frame;
> + struct_group(cnts,
> + u32 cycles;
> + u32 rx_busy;
> + u32 rx_frame;
> + u32 tx_frame;
> + );

This is horrid.

> };
>
> enum ath_device_state {
> diff --git a/drivers/net/wireless/ath/ath5k/ani.c b/drivers/net/wireless/ath/ath5k/ani.c
> index 850c608b4..fa95f0f0f 100644
> --- a/drivers/net/wireless/ath/ath5k/ani.c
> +++ b/drivers/net/wireless/ath/ath5k/ani.c
> @@ -379,7 +379,7 @@ ath5k_hw_ani_get_listen_time(struct ath5k_hw *ah, struct ath5k_ani_state *as)
> spin_lock_bh(&common->cc_lock);
>
> ath_hw_cycle_counters_update(common);
> - memcpy(&as->last_cc, &common->cc_ani, sizeof(as->last_cc));
> + memcpy(&as->last_cc.cnts, &common->cc_ani.cnts, sizeof(as->last_cc.cnts));

So is this.

Care to elaborate why this is needed at all, provided we copy/zero a
whole structure? And describe it in the commit log, not in random
external sources.

thanks,
--
js
suse labs


2023-06-02 05:25:12

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] wifi: ath: add struct_group for struct ath_cycle_counters

Jiri Slaby <[email protected]> writes:

> On 02. 06. 23, 6:36, Shiji Yang wrote:
>> From: Shiji Yang <[email protected]>
>>
>> Add a struct_group to around all members in struct ath_cycle_counters.
>> It can help the compiler detect the intended bounds of the memcpy() and
>> memset().
>>
>> This patch fixes the following build warning:
>>
>> In function 'fortify_memset_chk',
>> inlined from 'ath9k_ps_wakeup' at drivers/net/wireless/ath/ath9k/main.c:140:3:
>> ./include/linux/fortify-string.h:314:25: error: call to '__write_overflow_field' declared with attribute warning:
>> detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>> 314 | __write_overflow_field(p_size_field, size);
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>
> Hi,
>
> what compiler/version is this with?
>
>> Signed-off-by: Shiji Yang <[email protected]>
>> ---
>> More discussion on: https://github.com/openwrt/openwrt/pull/12764
>
> No "__write_overflow_field" there. Is this the right link?

Also it's better to include links like this in the actual commit log so
it's archived to git.

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

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

2023-06-04 01:47:29

by Shiji Yang

[permalink] [raw]
Subject: Re: [PATCH] wifi: ath: add struct_group for struct ath_cycle_counters

>Also it's better to include links like this in the actual commit log so
>it's archived to git.

Thanks for the tips. If necessary, I will update it in v2.

2023-06-04 01:48:13

by Shiji Yang

[permalink] [raw]
Subject: Re: [PATCH] wifi: ath: add struct_group for struct ath_cycle_counters

Thank you for your reply.
>On 02. 06. 23, 6:36, Shiji Yang wrote:
>> From: Shiji Yang <[email protected]>
>>
>> Add a struct_group to around all members in struct ath_cycle_counters.
>> It can help the compiler detect the intended bounds of the memcpy() and
>> memset().
>>
>> This patch fixes the following build warning:
>>
>> In function 'fortify_memset_chk',
>> inlined from 'ath9k_ps_wakeup' at drivers/net/wireless/ath/ath9k/main.c:140:3:
>> ./include/linux/fortify-string.h:314:25: error: call to '__write_overflow_field' declared with attribute warning:
>> detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>> 314 | __write_overflow_field(p_size_field, size);
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>
>Hi,
>
>what compiler/version is this with?

I'm using the mips gcc 12.3 (for arc ath79). It seems to be related
to the compiler, and arm gcc 12.3 does not show compilation warnings.

>> Signed-off-by: Shiji Yang <[email protected]>
>> ---
>> More discussion on: https://github.com/openwrt/openwrt/pull/12764
>
>No "__write_overflow_field" there. Is this the right link?

Yes. However, only few of the discussion here is related to compilation errors.

The full log:

make[4]: Entering directory '/home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24'
make[5]: 'Kconfig.versions' is up to date.
make[7]: 'Kconfig.versions' is up to date.
make[8]: 'conf' is up to date.
boolean symbol CRYPTO_LIB_ARC4 tested for 'm'? test forced to 'n'
#
# configuration written to .config
#
Building backport-include/backport/autoconf.h ... done.
CC [M] /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k/main.o
In file included from ./include/linux/string.h:253,
from /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/backport-include/linux/string.h:3,
from ./include/linux/bitmap.h:11,
from ./include/linux/cpumask.h:12,
from ./arch/mips/include/asm/processor.h:15,
from ./arch/mips/include/asm/thread_info.h:16,
from ./include/linux/thread_info.h:60,
from ./include/asm-generic/current.h:5,
from ./arch/mips/include/generated/asm/current.h:1,
from ./include/linux/sched.h:12,
from /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/backport-include/linux/sched.h:4,
from ./include/linux/delay.h:23,
from /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/backport-include/linux/delay.h:3,
from /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k/main.c:18:
In function 'fortify_memset_chk',
inlined from 'ath9k_ps_wakeup' at /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k/main.c:140:3:
./include/linux/fortify-string.h:314:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
314 | __write_overflow_field(p_size_field, size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[11]: *** [scripts/Makefile.build:250: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k/main.o] Error 1
make[10]: *** [scripts/Makefile.build:500: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k] Error 2
make[9]: *** [scripts/Makefile.build:500: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath] Error 2
make[8]: *** [scripts/Makefile.build:500: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless] Error 2
make[7]: *** [Makefile:2012: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24] Error 2
make[6]: *** [Makefile.build:13: modules] Error 2
make[5]: *** [Makefile.real:93: modules] Error 2
make[4]: *** [Makefile:121: modules] Error 2
make[4]: Leaving directory '/home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24'
make[3]: *** [Makefile:401: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/.built] Error 2
make[3]: Leaving directory '/home/db/openwrt/package/kernel/mac80211'
time: package/kernel/mac80211/regular/compile#4.94#0.95#21.75
ERROR: package/kernel/mac80211 failed to build (build variant: regular).
make[2]: *** [package/Makefile:120: package/kernel/mac80211/compile] Error 1
make[2]: Leaving directory '/home/db/openwrt'
make[1]: *** [package/Makefile:114: /home/db/openwrt/staging_dir/target-mips_24kc_musl/stamp/.package_compile] Error 2
make[1]: Leaving directory '/home/db/openwrt'
make: *** [/home/db/openwrt/include/toplevel.mk:231: world] Error 2

>> ---
>> drivers/net/wireless/ath/ath.h | 10 ++++++----
>> drivers/net/wireless/ath/ath5k/ani.c | 2 +-
>> drivers/net/wireless/ath/ath5k/base.c | 4 ++--
>> drivers/net/wireless/ath/ath5k/mac80211-ops.c | 2 +-
>> drivers/net/wireless/ath/ath9k/link.c | 2 +-
>> drivers/net/wireless/ath/ath9k/main.c | 4 ++--
>> drivers/net/wireless/ath/hw.c | 2 +-
>> 7 files changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
>> index f02a308a9..4b42e401a 100644
>> --- a/drivers/net/wireless/ath/ath.h
>> +++ b/drivers/net/wireless/ath/ath.h
>> @@ -43,10 +43,12 @@ struct ath_ani {
>> };
>>
>> struct ath_cycle_counters {
>> - u32 cycles;
>> - u32 rx_busy;
>> - u32 rx_frame;
>> - u32 tx_frame;
>> + struct_group(cnts,
>> + u32 cycles;
>> + u32 rx_busy;
>> + u32 rx_frame;
>> + u32 tx_frame;
>> + );
>
>This is horrid.

Yes, I agree, but this can avoid making more changes.

>> };
>>
>> enum ath_device_state {
>> diff --git a/drivers/net/wireless/ath/ath5k/ani.c b/drivers/net/wireless/ath/ath5k/ani.c
>> index 850c608b4..fa95f0f0f 100644
>> --- a/drivers/net/wireless/ath/ath5k/ani.c
>> +++ b/drivers/net/wireless/ath/ath5k/ani.c
>> @@ -379,7 +379,7 @@ ath5k_hw_ani_get_listen_time(struct ath5k_hw *ah, struct ath5k_ani_state *as)
>> spin_lock_bh(&common->cc_lock);
>>
>> ath_hw_cycle_counters_update(common);
>> - memcpy(&as->last_cc, &common->cc_ani, sizeof(as->last_cc));
>> + memcpy(&as->last_cc.cnts, &common->cc_ani.cnts, sizeof(as->last_cc.cnts));
>
>So is this.
>
>Care to elaborate why this is needed at all, provided we copy/zero a
>whole structure? And describe it in the commit log, not in random
>external sources.

Thank you for the prompt. I will pay attention to the format of the patch next time.

The compiler did not complain here. But in

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index a4197c14f..8608a29a1 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -135,8 +135,8 @@ void ath9k_ps_wakeup(struct ath_softc *sc)
if (power_mode != ATH9K_PM_AWAKE) {
spin_lock(&common->cc_lock);
ath_hw_cycle_counters_update(common);
- memset(&common->cc_survey, 0, sizeof(common->cc_survey));
- memset(&common->cc_ani, 0, sizeof(common->cc_ani));
+ memset(&common->cc_survey.cnts, 0, sizeof(common->cc_survey.cnts));
+ memset(&common->cc_ani.cnts, 0, sizeof(common->cc_ani.cnts));
spin_unlock(&common->cc_lock);
}

Here, memset() is used to zero the entire structure. The compiler will only warn
the second memset() `memset(&common->cc_ani, 0, sizeof(common->cc_ani));` However,
`cc_survey` and `cc_survey` are the same structure.

>
>thanks,
>--
>js
>suse labs

2023-06-04 02:10:18

by Shiji Yang

[permalink] [raw]
Subject: Re: [PATCH] wifi: ath: add struct_group for struct ath_cycle_counters

Thank you all,
This patch may not look so beautiful, but its main purpose is to raise
some awareness about this strange compilation warning.

This problem only occurs in mips gcc 12.3 (maybe only on arc mips/ath79).
In:

>--- a/drivers/net/wireless/ath/ath9k/main.c
>+++ b/drivers/net/wireless/ath/ath9k/main.c
>@@ -135,8 +135,8 @@ void ath9k_ps_wakeup(struct ath_softc *sc)
> if (power_mode != ATH9K_PM_AWAKE) {
> spin_lock(&common->cc_lock);
> ath_hw_cycle_counters_update(common);
>- memset(&common->cc_survey, 0, sizeof(common->cc_survey));
>- memset(&common->cc_ani, 0, sizeof(common->cc_ani));
>+ memset(&common->cc_survey.cnts, 0, sizeof(common->cc_survey.cnts));
>+ memset(&common->cc_ani.cnts, 0, sizeof(common->cc_ani.cnts));
> spin_unlock(&common->cc_lock);
> }

The compiler will only warn the second memset() `memset(&common->cc_ani, 0, sizeof(common->cc_ani));`
detected write beyond size of field. However, `cc_survey` and `cc_survey` are the same structure.
I have no idea about this warning. I will be very grateful if someone can provide some tips or a
better solution.

The full log:
make[4]: Entering directory '/home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24'
make[5]: 'Kconfig.versions' is up to date.
make[7]: 'Kconfig.versions' is up to date.
make[8]: 'conf' is up to date.
boolean symbol CRYPTO_LIB_ARC4 tested for 'm'? test forced to 'n'
#
# configuration written to .config
#
Building backport-include/backport/autoconf.h ... done.
CC [M] /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k/main.o
In file included from ./include/linux/string.h:253,
from /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/backport-include/linux/string.h:3,
from ./include/linux/bitmap.h:11,
from ./include/linux/cpumask.h:12,
from ./arch/mips/include/asm/processor.h:15,
from ./arch/mips/include/asm/thread_info.h:16,
from ./include/linux/thread_info.h:60,
from ./include/asm-generic/current.h:5,
from ./arch/mips/include/generated/asm/current.h:1,
from ./include/linux/sched.h:12,
from /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/backport-include/linux/sched.h:4,
from ./include/linux/delay.h:23,
from /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/backport-include/linux/delay.h:3,
from /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k/main.c:18:
In function 'fortify_memset_chk',
inlined from 'ath9k_ps_wakeup' at /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k/main.c:140:3:
./include/linux/fortify-string.h:314:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
314 | __write_overflow_field(p_size_field, size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[11]: *** [scripts/Makefile.build:250: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k/main.o] Error 1
make[10]: *** [scripts/Makefile.build:500: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k] Error 2
make[9]: *** [scripts/Makefile.build:500: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath] Error 2
make[8]: *** [scripts/Makefile.build:500: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless] Error 2
make[7]: *** [Makefile:2012: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24] Error 2
make[6]: *** [Makefile.build:13: modules] Error 2
make[5]: *** [Makefile.real:93: modules] Error 2
make[4]: *** [Makefile:121: modules] Error 2
make[4]: Leaving directory '/home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24'
make[3]: *** [Makefile:401: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/.built] Error 2
make[3]: Leaving directory '/home/db/openwrt/package/kernel/mac80211'
time: package/kernel/mac80211/regular/compile#4.94#0.95#21.75
ERROR: package/kernel/mac80211 failed to build (build variant: regular).
make[2]: *** [package/Makefile:120: package/kernel/mac80211/compile] Error 1
make[2]: Leaving directory '/home/db/openwrt'
make[1]: *** [package/Makefile:114: /home/db/openwrt/staging_dir/target-mips_24kc_musl/stamp/.package_compile] Error 2
make[1]: Leaving directory '/home/db/openwrt'
make: *** [/home/db/openwrt/include/toplevel.mk:231: world] Error 2

Best Regards,
Shiji Yang

2023-06-04 11:38:35

by Christian Lamparter

[permalink] [raw]
Subject: Re: [PATCH] wifi: ath: add struct_group for struct ath_cycle_counters

On 6/4/23 03:48, Shiji Yang wrote:
> Thank you all,
> This patch may not look so beautiful, but its main purpose is to raise
> some awareness about this strange compilation warning.
>
> This problem only occurs in mips gcc 12.3 (maybe only on arc mips/ath79).

It also occures with PPC464/APM82181. Could it be that that it occurs on
"big-endian" archs?


> In:
>
>> --- a/drivers/net/wireless/ath/ath9k/main.c
>> +++ b/drivers/net/wireless/ath/ath9k/main.c
>> @@ -135,8 +135,8 @@ void ath9k_ps_wakeup(struct ath_softc *sc)
>> if (power_mode != ATH9K_PM_AWAKE) {
>> spin_lock(&common->cc_lock);
>> ath_hw_cycle_counters_update(common);
>> - memset(&common->cc_survey, 0, sizeof(common->cc_survey));
>> - memset(&common->cc_ani, 0, sizeof(common->cc_ani));
>> + memset(&common->cc_survey.cnts, 0, sizeof(common->cc_survey.cnts));
>> + memset(&common->cc_ani.cnts, 0, sizeof(common->cc_ani.cnts));
>> spin_unlock(&common->cc_lock);
>> }
>
> The compiler will only warn the second memset() `memset(&common->cc_ani, 0, sizeof(common->cc_ani));`
> detected write beyond size of field. However, `cc_survey` and `cc_survey` are the same structure.
> I have no idea about this warning. I will be very grateful if someone can provide some tips or a
> better solution.
>
> The full log:
> make[4]: Entering directory '/home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24'
> make[5]: 'Kconfig.versions' is up to date.
> make[7]: 'Kconfig.versions' is up to date.
> make[8]: 'conf' is up to date.
> boolean symbol CRYPTO_LIB_ARC4 tested for 'm'? test forced to 'n'
> #
> # configuration written to .config
> #
> Building backport-include/backport/autoconf.h ... done.
> CC [M] /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k/main.o
> In file included from ./include/linux/string.h:253,
> from /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/backport-include/linux/string.h:3,
> from ./include/linux/bitmap.h:11,
> from ./include/linux/cpumask.h:12,
> from ./arch/mips/include/asm/processor.h:15,
> from ./arch/mips/include/asm/thread_info.h:16,
> from ./include/linux/thread_info.h:60,
> from ./include/asm-generic/current.h:5,
> from ./arch/mips/include/generated/asm/current.h:1,
> from ./include/linux/sched.h:12,
> from /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/backport-include/linux/sched.h:4,
> from ./include/linux/delay.h:23,
> from /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/backport-include/linux/delay.h:3,
> from /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k/main.c:18:
> In function 'fortify_memset_chk',
> inlined from 'ath9k_ps_wakeup' at /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k/main.c:140:3:
> ./include/linux/fortify-string.h:314:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
> 314 | __write_overflow_field(p_size_field, size);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make[11]: *** [scripts/Makefile.build:250: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k/main.o] Error 1
> make[10]: *** [scripts/Makefile.build:500: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath/ath9k] Error 2
> make[9]: *** [scripts/Makefile.build:500: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless/ath] Error 2
> make[8]: *** [scripts/Makefile.build:500: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/drivers/net/wireless] Error 2
> make[7]: *** [Makefile:2012: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24] Error 2
> make[6]: *** [Makefile.build:13: modules] Error 2
> make[5]: *** [Makefile.real:93: modules] Error 2
> make[4]: *** [Makefile:121: modules] Error 2
> make[4]: Leaving directory '/home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24'
> make[3]: *** [Makefile:401: /home/db/openwrt/build_dir/target-mips_24kc_musl/linux-ath79_nand/backports-6.1.24/.built] Error 2
> make[3]: Leaving directory '/home/db/openwrt/package/kernel/mac80211'
> time: package/kernel/mac80211/regular/compile#4.94#0.95#21.75
> ERROR: package/kernel/mac80211 failed to build (build variant: regular).
> make[2]: *** [package/Makefile:120: package/kernel/mac80211/compile] Error 1
> make[2]: Leaving directory '/home/db/openwrt'
> make[1]: *** [package/Makefile:114: /home/db/openwrt/staging_dir/target-mips_24kc_musl/stamp/.package_compile] Error 2
> make[1]: Leaving directory '/home/db/openwrt'
> make: *** [/home/db/openwrt/include/toplevel.mk:231: world] Error 2
>
> Best Regards,
> Shiji Yang


2023-06-05 06:06:04

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] wifi: ath: add struct_group for struct ath_cycle_counters

On 04. 06. 23, 13:11, Christian Lamparter wrote:
> On 6/4/23 03:48, Shiji Yang wrote:
>> Thank you all,
>> This patch may not look so beautiful, but its main purpose is to raise
>> some awareness about this strange compilation warning.
>>
>> This problem only occurs in mips gcc 12.3 (maybe only on arc mips/ath79).
>
> It also occures with PPC464/APM82181. Could it be that that it occurs on
> "big-endian" archs?

Whatever, to me this very looks like a compiler bug.

So NACK to this one for now.

If this still happens on the latest tree (e.g. 6.4-rc) and with gcc 13,
we might need to fix fortify or report to the gcc bugzilla [1] instead
-- attach a preprocessed source there (the output of your make
CROSSblabla=xyz drivers/net/wireless/ath/ath9k/main.i).

[1] https://gcc.gnu.org/bugzilla/enter_bug.cgi

thanks,
--
js
suse labs