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

[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@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