lkml.org 
[lkml]   [2023]   [Jun]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 1/2] carl9170: re-fix fortified-memset warning
    From
    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

    \
     
     \ /
      Last update: 2023-06-26 08:52    [W:2.817 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site