linux-kernel - Re: [PATCH] include/linux/compiler*.h: Use feature checking instead of version checks for attributes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 28 Aug 2018 22:33:10 +0200
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Eli Friedman <efriedma@...eaurora.org>,
        Christopher Li <sparse@...isli.org>,
        Kees Cook <keescook@...omium.org>,
        Ingo Molnar <mingo@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Arnd Bergmann <arnd@...db.de>,
        Greg KH <gregkh@...uxfoundation.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Joe Perches <joe@...ches.com>,
        Dominique Martinet <asmadeus@...ewreck.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] include/linux/compiler*.h: Use feature checking instead
 of version checks for attributes

Hi Nick,

On Tue, Aug 28, 2018 at 7:05 PM, Nick Desaulniers
<ndesaulniers@...gle.com> wrote:
> On Tue, Aug 28, 2018 at 8:10 AM Miguel Ojeda
> <miguel.ojeda.sandonis@...il.com> wrote:
>>
>> I addressed that in the email I sent afterwards:
>>
>> """
>> Note that:
>>   - assume_aligned came with gcc 4.9
>>   - no_sanitize_address came with gcc 4.8
>>
>> So if we feel it is important to have them there (before gcc 5), we
>> would need here a quick version check here.
>> """
>>
>> The idea is that, in the future, whenever gcc 5 or later is the
>> minimum version, we just get rid of the #ifdef block without touching
>> the rest of the code :-)
>
> So if __has_attribute came with gcc 5, then that means that this patch
> will break assume_aligned for gcc-4.9 users and no_sanitize_address
> for gcc-4.8 and gcc-4.9 users?  The slab allocator uses
> assume_aligned, and no_sanitize_address for CONFIG_KASAN.  Should this
> patch ever come back through stable, Android and ChromeOS
> gcc-4.9/KASAN builds will break.
>

Indeed, KASAN requires it:

          This is strictly a debugging feature and it requires a gcc version
          of 4.9.2 or later. Detection of out of bounds accesses to stack or
          global variables requires gcc 5.0 or later.

So we should just support it. However, __no_sanitize_address is only
used when CONFIG_KASAN is enabled (to define __no_kasan_or_inline). So
I would say it is an attribute for a particular CONFIG (like those of
sparse). Therefore, I think we should simply remove
__no_sanitize_address for general use (let's see how it looks).

For __assume_aligned, it is "only" an optimization, but I think it is
a general one, so we should keep it in attributes.h; I will simply add
the gcc 4.9 support knowledge.

On that topic: actually, some of the attributes that we have that are
"required" are not really "required" in the strict sense: we could
test for them; but I wanted to minimize the amount of noise for gcc <
5 since we have to manually write the support table (and anyway most
compilers support them). Whenever we are past gcc 5 in a few years, we
could actually test for the non-strictly-required attribute if we want
to be extra nice to new compilers :-)

> I don't think we should leave that for a follow up; I would like to
> see it as part of this patch.  It's ok to have explicit version checks
> for those 2 attributes since it's not possible to feature detect them
> for the versions of gcc that we support in this code base.  I think
> you should add them in a v2 of this patch.  Then we can point to this
> commit as the *shining example* of how to do proper feature detection,
> falling back to version checks only as a last resort.

Thanks for the kind words!

I also read your other comments in the previous email -- no comments
on those. I will prepare v2.

Cheers,
Miguel