Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <eba3d9ce-389b-b4cf-1af2-6a5ee9ca5049@kernel.org>
Date:   Mon, 26 Jun 2023 08:51:39 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH 1/2] carl9170: re-fix fortified-memset warning
Content-Language: en-US
To:     Christian Lamparter <chunkeey@gmail.com>,
        Arnd Bergmann <arnd@arndb.de>, Arnd Bergmann <arnd@kernel.org>,
        Kalle Valo <kvalo@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Johannes Berg <johannes.berg@intel.com>,
        Shiji Yang <yangshiji66@outlook.com>,
        Nick Kossifidis <mickflemm@gmail.com>,
        Christian Marangi <ansuelsmth@gmail.com>
Cc:     linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230623152443.2296825-1-arnd@kernel.org>
 <7c4622e7-d7a8-ae5d-e381-f726cb511228@gmail.com>
 <24986b5e-5cd1-4cd5-aff3-b5eab2c0fdde@app.fastmail.com>
 <3bb839fe-1dfd-57f5-a5b0-be5adac57a4c@gmail.com>
From:   Jiri Slaby <jirislaby@kernel.org>
In-Reply-To: <3bb839fe-1dfd-57f5-a5b0-be5adac57a4c@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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