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

[Jiri Slaby <jirislaby@kernel.org>]

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