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.