Messages in this thread | | | Date | Mon, 26 Jun 2023 08:51:39 +0200 | Subject | Re: [PATCH 1/2] carl9170: re-fix fortified-memset warning | From | Jiri Slaby <> |
| |
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@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM/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
| |