On Wed, May 23, 2018 at 11:08:40AM +0200, Peter Zijlstra wrote:
>
> Sorry for being late to the party..
Likewise!
> On Wed, May 23, 2018 at 12:03:57AM -0500, Gustavo A. R. Silva wrote:
> > +#define validate_index_nospec(index, size) \
> > +({ \
> > + bool ret = true; \
> > + typeof(index) *ptr = &(index); \
> > + typeof(size) _s = (size); \
> > + \
> > + BUILD_BUG_ON(sizeof(*ptr) > sizeof(long)); \
> > + BUILD_BUG_ON(sizeof(_s) > sizeof(long)); \
> > + \
> > + if (*ptr >= size) \
> > + ret = false; \
> > + \
> > + *ptr = array_index_nospec(*ptr, _s); \
> > + \
> > + ret; \
> > +})
>
> Would not something like:
>
> bool ret = false;
>
> ....
>
> if (*ptr < _s) {
> *ptr = array_index_nospec(*ptr, _s);
> ret = true;
> }
>
> ret;
>
> be more obvious?
I think that either way, we have a potential problem if the compiler
generates a branch dependent on the result of validate_index_nospec().
In that case, we could end up with codegen approximating:
bool safe = false;
if (idx < bound) {
idx = array_index_nospec(idx, bound);
safe = true;
}
// this branch can be mispredicted
if (safe) {
foo = array[idx];
}
... and thus we lose the nospec protection.
I also suspect that compiler transformations mean that this might
already be the case for patterns like:
if (idx < bound) {
safe_idx = array_index_nospec(idx, bound)];
...
foo = array[safe_idx];
}
... if the compiler can transform that to something like:
if (idx < bound) {
idx = array_index_nospec(idx, bound);
}
// can be mispredicted
if (idx < bound) {
foo = array[idx];
}
... which I think a compiler might be capable of, depending on the rest
of the function body (e.g. if there's a common portion shared with the
else case).
I'll see if I can trigger that in a test case. :/
Thanks,
Mark.
