On 2/6/19 15:52, Jan Beulich wrote: >>>> On 29.01.19 at 15:43, <nmant...@amazon.de> wrote: >> @@ -963,6 +965,9 @@ map_grant_ref( >> PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n", >> op->ref, rgt->domain->domain_id); >> >> + /* Make sure the above check is not bypassed speculatively */ >> + op->ref = array_index_nospec(op->ref, nr_grant_entries(rgt)); >> + >> act = active_entry_acquire(rgt, op->ref); >> shah = shared_entry_header(rgt, op->ref); >> status = rgt->gt_version == 1 ? &shah->flags : &status_entry(rgt, >> op->ref); > Just FTR - this is a case where the change, according to prior > discussion, is pretty unlikely to help at all. The compiler will have > a hard time realizing that it could keep the result in a register past > the active_entry_acquire() invocation, as that - due to the spin > lock acquired there - acts as a compiler barrier. And looking at > generated code (gcc 8.2) confirms that there's a reload from the > stack. I could change this back to a prior version that protects each read operation. >> @@ -2026,6 +2031,9 @@ gnttab_prepare_for_transfer( >> goto fail; >> } >> >> + /* Make sure the above check is not bypassed speculatively */ >> + ref = array_index_nospec(ref, nr_grant_entries(rgt)); >> + >> sha = shared_entry_header(rgt, ref); >> >> scombo.word = *(u32 *)&sha->flags; >> @@ -2223,7 +2231,8 @@ gnttab_transfer( >> okay = gnttab_prepare_for_transfer(e, d, gop.ref); >> spin_lock(&e->page_alloc_lock); >> >> - if ( unlikely(!okay) || unlikely(e->is_dying) ) >> + /* Make sure this check is not bypassed speculatively */ >> + if ( evaluate_nospec(unlikely(!okay) || unlikely(e->is_dying)) ) > I'm still not really happy about this. The comment isn't helpful in > connecting the use of evaluate_nospec() to the problem site > (in the earlier hunk, which I've left in context), and I still don't > understand why the e->is_dying is getting wrapped as well. > Plus it occurs to me now that you're liable to render unlikely() > ineffective here. So how about > > if ( unlikely(evaluate_nospec(!okay)) || unlikely(e->is_dying) ) > > ?
I will move the evaluate_nospec closer to the evaluation of okay, and will improve the comment mentioning that the okay variable represents whether the current reference is actually valid. Best, Norbert Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich Ust-ID: DE 289 237 879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
