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 10:05:30 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     miguel.ojeda.sandonis@...il.com
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        efriedma@...eaurora.org, 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@...ches.com, 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

On Tue, Aug 28, 2018 at 8:10 AM Miguel Ojeda
<miguel.ojeda.sandonis@...il.com> wrote:
>
> Hi Nick,
>
> On Mon, Aug 27, 2018 at 7:48 PM, Nick Desaulniers
> <ndesaulniers@...gle.com> wrote:
> > On Mon, Aug 27, 2018 at 10:43 AM Nick Desaulniers
> >> > +
> >> > +/*
> >> > + * Optional attributes: your compiler may or may not support them.
> >> > + *
> >> > + * To check for them, we use __has_attribute, which is supported on gcc >= 5,
> >> > + * clang >= 2.9 and icc >= 17. In the meantime, to support 4.6 <= gcc < 5,
> >> > + * we implement it by hand.
> >> > + */
> >> > +#ifndef __has_attribute
> >> > +#define __has_attribute(x) __GCC46_has_attribute_##x
> >> > +#define __GCC46_has_attribute_assume_aligned 0
> >> > +#define __GCC46_has_attribute_designated_init 0
> >> > +#define __GCC46_has_attribute_externally_visible 1
> >> > +#define __GCC46_has_attribute_noclone 1
> >> > +#define __GCC46_has_attribute_optimize 1
> >> > +#define __GCC46_has_attribute_no_sanitize_address 0
> >> > +#endif
> >
> > And a follow up; I'm trying to understand what will happen in the case
> > of say gcc 4.9 here.  Were any of these supported between gcc 4.6 and
> > 5.0?  If so, then this code will not use them.  It's simpler than
> > explicit version checks, but it won't use features that are supported.
> >
>
> 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.

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,
~Nick Desaulniers