kernel-hardening - Re: [PATCH v5 02/12] array_idx: sanitize speculative array de-references

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.20.1801281931180.2126@nanos>
Date: Sun, 28 Jan 2018 19:36:38 +0100 (CET)
From: Thomas Gleixner <tglx@...utronix.de>
To: Dan Williams <dan.j.williams@...el.com>
cc: Ingo Molnar <mingo@...nel.org>, linux-arch <linux-arch@...r.kernel.org>, 
    Cyril Novikov <cnovikov@...x.com>, 
    Kernel Hardening <kernel-hardening@...ts.openwall.com>, 
    Peter Zijlstra <peterz@...radead.org>, 
    Catalin Marinas <catalin.marinas@....com>, X86 ML <x86@...nel.org>, 
    Will Deacon <will.deacon@....com>, Russell King <linux@...linux.org.uk>, 
    Ingo Molnar <mingo@...hat.com>, Greg KH <gregkh@...uxfoundation.org>, 
    "H. Peter Anvin" <hpa@...or.com>, 
    Linus Torvalds <torvalds@...ux-foundation.org>, 
    Alan Cox <alan@...ux.intel.com>, 
    Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 02/12] array_idx: sanitize speculative array
 de-references

On Sun, 28 Jan 2018, Dan Williams wrote:
> On Sun, Jan 28, 2018 at 12:55 AM, Ingo Molnar <mingo@...nel.org> wrote:
> >> + */
> >> +#define array_idx(idx, sz)                                           \
> >> +({                                                                   \
> >> +     typeof(idx) _i = (idx);                                         \
> >> +     typeof(sz) _s = (sz);                                           \
> >> +     unsigned long _mask = array_idx_mask(_i, _s);                   \
> >> +                                                                     \
> >> +     BUILD_BUG_ON(sizeof(_i) > sizeof(long));                        \
> >> +     BUILD_BUG_ON(sizeof(_s) > sizeof(long));                        \
> >> +                                                                     \
> >> +     _i &= _mask;                                                    \
> >> +     _i;                                                             \
> >> +})
> >> +#endif /* __NOSPEC_H__ */
> >
> > For heaven's sake, please name a size variable as 'size', not 'sz'. We don't have
> > a shortage of characters and can deobfuscate common primitives, can we?
> >
> > Also, beyond the nits, I also hate the namespace here. We have a new generic
> > header providing two new methods:
> >
> >         #include <linux/nospec.h>
> >
> >         array_idx_mask()
> >         array_idx()
> >
> > which is then optimized for x86 in asm/barrier.h. That's already a non-sequitor.
> >
> > Then we introduce uaccess API variants with a _nospec() postfix.
> >
> > Then we add ifence() to x86.
> >
> > There's no naming coherency to this.
> 
> Ingo, I love you, but please take the incredulity down a bit,
> especially when I had 'nospec' in all the names in v1. Thomas, Peter,
> and Alexei wanted s/nospec_barrier/ifence/ and

Sorry, I never was involved in that discussion.

> s/array_idx_nospec/array_idx/. You can always follow on with a patch
> to fix up the names and placements to your liking. While they'll pick
> on my name choices, they won't pick on yours, because I simply can't
> be bothered to care about a bikeshed color at this point after being
> bounced around for 5 revisions of this patch set.

Oh well, we really need this kind of attitude right now. We are all fed up
with that mess, but Ingo and I care about the details, consistency and
general code quality beyond the current rush to get stuff solved. It's our
damned job as maintainers.

If you decide you don't care anymore, please let me know, so I can try to
free up some cycles to pick up the stuff from where you decided to dump it.

Thanks,

	tglx
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.