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

  From: Arnd Bergmann <arnd@arndb.de>

The carl9170_tx_release() function sometimes triggers a fortified-memset
warning in my randconfig builds:

In file included from include/linux/string.h:254,
                 from drivers/net/wireless/ath/carl9170/tx.c:40:
In function 'fortify_memset_chk',
    inlined from 'carl9170_tx_release' at drivers/net/wireless/ath/carl9170/tx.c:283:2,
    inlined from 'kref_put' at include/linux/kref.h:65:3,
    inlined from 'carl9170_tx_put_skb' at drivers/net/wireless/ath/carl9170/tx.c:342:9:
include/linux/fortify-string.h:493:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
  493 |                         __write_overflow_field(p_size_field, size);

Kees previously tried to avoid this by using memset_after(), but it seems
this does not fully address the problem. I noticed that the memset_after()
here is done on a different part of the union (status) than the original
cast was from (rate_driver_data), which may confuse the compiler.

Unfortunately, the memset_after() trick does not work on driver_rates[]
because that is part of an anonymous struct, and I could not get
struct_group() to do this either. Using two separate memset() calls
on the two members does address the warning though.

Fixes: fb5f6a0e8063b ("mac80211: Use memset_after() to clear tx status")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/wireless/ath/carl9170/tx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

On 6/23/23 17:23, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The carl9170_tx_release() function sometimes triggers a fortified-memset
> warning in my randconfig builds:
> 
> In file included from include/linux/string.h:254,
>                   from drivers/net/wireless/ath/carl9170/tx.c:40:
> In function 'fortify_memset_chk',
>      inlined from 'carl9170_tx_release' at drivers/net/wireless/ath/carl9170/tx.c:283:2,
>      inlined from 'kref_put' at include/linux/kref.h:65:3,
>      inlined from 'carl9170_tx_put_skb' at drivers/net/wireless/ath/carl9170/tx.c:342:9:
> include/linux/fortify-string.h:493:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>    493 |                         __write_overflow_field(p_size_field, size);
> 
> Kees previously tried to avoid this by using memset_after(), but it seems
> this does not fully address the problem. I noticed that the memset_after()
> here is done on a different part of the union (status) than the original
> cast was from (rate_driver_data), which may confuse the compiler.
> 
> Unfortunately, the memset_after() trick does not work on driver_rates[]
> because that is part of an anonymous struct, and I could not get
> struct_group() to do this either. Using two separate memset() calls
> on the two members does address the warning though.
> 
> Fixes: fb5f6a0e8063b ("mac80211: Use memset_after() to clear tx status")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>


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.

so, what's going wrong with fortified there?

Thanks,
Christian
  

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.

     Arnd
  

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;

(Note here: cc_survey and cc_ani are of the same struct ath_cycle_counters!
and they are right next to each other! Even better: reordering the memset
in the code does not help. Reordering it in the ath_common does!)

This is less intrusive since it only replaces one line... but I'm afraid it
too will get flagged for the same reason... Maybe can someone give Shiji Yang,
or me, if he has lost interest, some guidance on how this can be addressed?

Best Regards,
Christian
  

On Fri, Jun 23, 2023 at 05:23:59PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The carl9170_tx_release() function sometimes triggers a fortified-memset
> warning in my randconfig builds:
> 
> In file included from include/linux/string.h:254,
>                  from drivers/net/wireless/ath/carl9170/tx.c:40:
> In function 'fortify_memset_chk',
>     inlined from 'carl9170_tx_release' at drivers/net/wireless/ath/carl9170/tx.c:283:2,
>     inlined from 'kref_put' at include/linux/kref.h:65:3,
>     inlined from 'carl9170_tx_put_skb' at drivers/net/wireless/ath/carl9170/tx.c:342:9:
> include/linux/fortify-string.h:493:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>   493 |                         __write_overflow_field(p_size_field, size);
> 
> Kees previously tried to avoid this by using memset_after(), but it seems
> this does not fully address the problem. I noticed that the memset_after()
> here is done on a different part of the union (status) than the original
> cast was from (rate_driver_data), which may confuse the compiler.
> 
> Unfortunately, the memset_after() trick does not work on driver_rates[]
> because that is part of an anonymous struct, and I could not get
> struct_group() to do this either. Using two separate memset() calls
> on the two members does address the warning though.
> 
> Fixes: fb5f6a0e8063b ("mac80211: Use memset_after() to clear tx status")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/net/wireless/ath/carl9170/tx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c
> index 6bb9aa2bfe654..88ef6e023f826 100644
> --- a/drivers/net/wireless/ath/carl9170/tx.c
> +++ b/drivers/net/wireless/ath/carl9170/tx.c
> @@ -280,7 +280,8 @@ static void carl9170_tx_release(struct kref *ref)
>  	 * carl9170_tx_fill_rateinfo() has filled the rate information
>  	 * before we get to this point.
>  	 */
> -	memset_after(&txinfo->status, 0, rates);
> +	memset(&txinfo->pad, 0, sizeof(txinfo->pad));
> +	memset(&txinfo->rate_driver_data, 0, sizeof(txinfo->rate_driver_data));

This is "accidentally" equivalent, which makes me nervous. It was
designed to clear everything after "rates", regardless of padding, etc.

What I don't get is why the warning is being emitted. It boils down to
an expansion of this:

        memset(__ptr + offsetofend(typeof(*(obj)), member), __val,
               sizeof(*(obj)) - offsetofend(typeof(*(obj)), member));

into:

	memset(&txinfo->status + offsetofend(struct ieee80211_tx_info, rates),
	       0, sizeof(txinfo->status - offsetofend(struct ieee80211_tx_info, rates)))

Is offsetofend() broken?
  

On Fri, Jun 23, 2023 at 06:05:57PM +0200, Arnd Bergmann wrote:
> 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.

Can you post the config that triggers this? I can't reproduce this
warning...

-Kees
  

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,