Re: [PATCH 1/2] carl9170: re-fix fortified-memset warning

From: Jiri Slaby
Date: Mon Jun 26 2023 - 02:51:55 EST


On 23. 06. 23, 19:15, Christian Lamparter wrote:
On 6/23/23 18:05, Arnd Bergmann wrote:
On Fri, Jun 23, 2023, at 17:38, Christian Lamparter wrote:
On 6/23/23 17:23, Arnd Bergmann wrote:

Wait! I want to point out this funny thing is happening in ath too!

https://lore.kernel.org/linux-wireless/TYAP286MB03154F9AAFD4C35BEEDE4A99BC4CA@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/T/#mf1b8919a000fe661803c17073f48b3c410888541

And that patch got NACK by Jiri Slaby because like me he suspects that
this is a compiler bug.

FWIW, that is one I don't see with clang-17 or gcc-13. The one I'm addressing
here is the only thing I see in ath wireless with the default set of
warning options, though this driver does have a couple of others that
are unrelated, when you enable the source data check in memcpy() by
building with W=1.

  In file included from  drivers/net/wireless/ath/ath9k/xmit.c:17:
In file included from  include/linux/dma-mapping.h:7:
In file included from include/linux/string.h:254:
/home/arnd/arm-soc/include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
                         __read_overflow2_field(q_size_field, size);
                         ^
include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
2 errors generated.
/home/arnd/arm-soc/include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
                         __read_overflow2_field(q_size_field, size);

so, what's going wrong with fortified there?

Kees might have a better answer to that, my best guess is that
the one I'm addressing stems from the confusion between different
union members.

Doing the randconfig builds with the latest compilers, carl9170 is the
only one I see with fortified-string warnings, and there are a few
dozen other drivers that I see with W=1, including one that affects
all wireless drivers.

Hm, question here (to Jiri as well). Do you think that a workaround patch
for these sort-of-obvious-but-compiler-bug-but-failed-to-make-a-simple-reproducer
would be OK to get NACKed? In my case, I fiddled around with it and replaced the
the cc_ani memset in the following way:

|        memset(&common->cc_survey, 0, sizeof(common->cc_survey));
|-       memset(&common->cc_ani, 0, sizeof(common->cc_ani));
|+       common->cc_ani.cycles = common->cc_ani.rx_busy = common->cc_ani.rx_frame = common->cc_ani.tx_frame = 0;

Nah, you are still changing the code for the compiler. And espectially this one calls for troubles later -- when cc_ani changes.

Again, work also with compiler guys, they are usually helpful. Both in helping to understand the issue (from the compiler POV) and provide a fix/workaround.

Even this carl9170 change looks very bad to me. While "memset_after(&txinfo->status, 0, rates);" means exactly what it does, those two memsets barely. It took me a while to understand what is going on and that it is the same. Don't do this.

Perhaps we need memset_no_check()?

thanks,
--
js
suse labs