| Message ID | 0a59ae2f-448e-610d-e8a2-a7c3f9f3918f@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/PV: avoid speculation abuse through guest accessors | expand |
On Wed, Feb 17, 2021 at 09:23:33AM +0100, Jan Beulich wrote: > The former expands to a single (memory accessing) insn, which the latter > does not guarantee. Yet we'd prefer to read consistent PTEs rather than > risking a split read racing with an update done elsewhere. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Albeit I wonder why the __builtin_constant_p check done in copy_from_unsafe is not enough to take the get_unsafe_size branch in there. Doesn't sizeof(l{1,2}_pgentry_t) qualify as a built time constant? Or the fact that n it's a parameter to an inline function hides this, in which case the __builtin_constant_p is pointless? Thanks, Roger.
On 23.02.2021 12:59, Roger Pau Monné wrote: > On Wed, Feb 17, 2021 at 09:23:33AM +0100, Jan Beulich wrote: >> The former expands to a single (memory accessing) insn, which the latter >> does not guarantee. Yet we'd prefer to read consistent PTEs rather than >> risking a split read racing with an update done elsewhere. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Albeit I wonder why the __builtin_constant_p check done in > copy_from_unsafe is not enough to take the get_unsafe_size branch in > there. Doesn't sizeof(l{1,2}_pgentry_t) qualify as a built time > constant? > > Or the fact that n it's a parameter to an inline function hides this, > in which case the __builtin_constant_p is pointless? Without (enough) optimization, __builtin_constant_p() may indeed yield false in such cases. But that wasn't actually what I had in mind when making this change (and the original similar on in shadow code). Instead, at the time I made the shadow side change, I had removed this optimization from the new function flavors. With that removal, things are supposed to still be correct - it's an optimization only, after all. Meanwhile the optimizations are back, so there's no immediate problem as long as the optimizer doesn't decide to out-of-line the function invocations (we shouldn't forget that even always_inline is not a guarantee for inlining to actually occur). Jan
On Tue, Feb 23, 2021 at 04:25:00PM +0100, Jan Beulich wrote: > On 23.02.2021 12:59, Roger Pau Monné wrote: > > On Wed, Feb 17, 2021 at 09:23:33AM +0100, Jan Beulich wrote: > >> The former expands to a single (memory accessing) insn, which the latter > >> does not guarantee. Yet we'd prefer to read consistent PTEs rather than > >> risking a split read racing with an update done elsewhere. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > > > Albeit I wonder why the __builtin_constant_p check done in > > copy_from_unsafe is not enough to take the get_unsafe_size branch in > > there. Doesn't sizeof(l{1,2}_pgentry_t) qualify as a built time > > constant? > > > > Or the fact that n it's a parameter to an inline function hides this, > > in which case the __builtin_constant_p is pointless? > > Without (enough) optimization, __builtin_constant_p() may indeed > yield false in such cases. But that wasn't actually what I had > in mind when making this change (and the original similar on in > shadow code). Instead, at the time I made the shadow side change, > I had removed this optimization from the new function flavors. > With that removal, things are supposed to still be correct - it's > an optimization only, after all. Meanwhile the optimizations are > back, so there's no immediate problem as long as the optimizer > doesn't decide to out-of-line the function invocations (we > shouldn't forget that even always_inline is not a guarantee for > inlining to actually occur). I'm fine with you switching those use cases to get_unsafe, but I think the commit message should be slightly adjusted to notice that copy_from_unsafe will likely do the right thing, but that it's simply clearer to call get_unsafe directly, also in case copy_from_unsafe gets changed in the future to drop the get_unsafe paths. Thanks, Roger.
On 23.02.2021 16:37, Roger Pau Monné wrote: > On Tue, Feb 23, 2021 at 04:25:00PM +0100, Jan Beulich wrote: >> On 23.02.2021 12:59, Roger Pau Monné wrote: >>> On Wed, Feb 17, 2021 at 09:23:33AM +0100, Jan Beulich wrote: >>>> The former expands to a single (memory accessing) insn, which the latter >>>> does not guarantee. Yet we'd prefer to read consistent PTEs rather than >>>> risking a split read racing with an update done elsewhere. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> >>> >>> Albeit I wonder why the __builtin_constant_p check done in >>> copy_from_unsafe is not enough to take the get_unsafe_size branch in >>> there. Doesn't sizeof(l{1,2}_pgentry_t) qualify as a built time >>> constant? >>> >>> Or the fact that n it's a parameter to an inline function hides this, >>> in which case the __builtin_constant_p is pointless? >> >> Without (enough) optimization, __builtin_constant_p() may indeed >> yield false in such cases. But that wasn't actually what I had >> in mind when making this change (and the original similar on in >> shadow code). Instead, at the time I made the shadow side change, >> I had removed this optimization from the new function flavors. >> With that removal, things are supposed to still be correct - it's >> an optimization only, after all. Meanwhile the optimizations are >> back, so there's no immediate problem as long as the optimizer >> doesn't decide to out-of-line the function invocations (we >> shouldn't forget that even always_inline is not a guarantee for >> inlining to actually occur). > > I'm fine with you switching those use cases to get_unsafe, but I think > the commit message should be slightly adjusted to notice that > copy_from_unsafe will likely do the right thing, but that it's simply > clearer to call get_unsafe directly, also in case copy_from_unsafe > gets changed in the future to drop the get_unsafe paths. How about this then? "The former expands to a single (memory accessing) insn, which the latter does not guarantee (the __builtin_constant_p() based switch() statement there is just an optimization). Yet we'd prefer to read consistent PTEs rather than risking a split read racing with an update done elsewhere." Jan
On Tue, Feb 23, 2021 at 05:13:21PM +0100, Jan Beulich wrote: > On 23.02.2021 16:37, Roger Pau Monné wrote: > > On Tue, Feb 23, 2021 at 04:25:00PM +0100, Jan Beulich wrote: > >> On 23.02.2021 12:59, Roger Pau Monné wrote: > >>> On Wed, Feb 17, 2021 at 09:23:33AM +0100, Jan Beulich wrote: > >>>> The former expands to a single (memory accessing) insn, which the latter > >>>> does not guarantee. Yet we'd prefer to read consistent PTEs rather than > >>>> risking a split read racing with an update done elsewhere. > >>>> > >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >>> > >>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > >>> > >>> Albeit I wonder why the __builtin_constant_p check done in > >>> copy_from_unsafe is not enough to take the get_unsafe_size branch in > >>> there. Doesn't sizeof(l{1,2}_pgentry_t) qualify as a built time > >>> constant? > >>> > >>> Or the fact that n it's a parameter to an inline function hides this, > >>> in which case the __builtin_constant_p is pointless? > >> > >> Without (enough) optimization, __builtin_constant_p() may indeed > >> yield false in such cases. But that wasn't actually what I had > >> in mind when making this change (and the original similar on in > >> shadow code). Instead, at the time I made the shadow side change, > >> I had removed this optimization from the new function flavors. > >> With that removal, things are supposed to still be correct - it's > >> an optimization only, after all. Meanwhile the optimizations are > >> back, so there's no immediate problem as long as the optimizer > >> doesn't decide to out-of-line the function invocations (we > >> shouldn't forget that even always_inline is not a guarantee for > >> inlining to actually occur). > > > > I'm fine with you switching those use cases to get_unsafe, but I think > > the commit message should be slightly adjusted to notice that > > copy_from_unsafe will likely do the right thing, but that it's simply > > clearer to call get_unsafe directly, also in case copy_from_unsafe > > gets changed in the future to drop the get_unsafe paths. > > How about this then? > > "The former expands to a single (memory accessing) insn, which the latter > does not guarantee (the __builtin_constant_p() based switch() statement > there is just an optimization). Yet we'd prefer to read consistent PTEs > rather than risking a split read racing with an update done elsewhere." LGTM, thanks. Roger.
--- a/xen/arch/x86/pv/mm.c +++ b/xen/arch/x86/pv/mm.c @@ -41,9 +41,7 @@ l1_pgentry_t *map_guest_l1e(unsigned lon return NULL; /* Find this l1e and its enclosing l1mfn in the linear map. */ - if ( copy_from_unsafe(&l2e, - &__linear_l2_table[l2_linear_offset(linear)], - sizeof(l2_pgentry_t)) ) + if ( get_unsafe(l2e, &__linear_l2_table[l2_linear_offset(linear)]) ) return NULL; /* Check flags that it will be safe to read the l1e. */ --- a/xen/arch/x86/pv/mm.h +++ b/xen/arch/x86/pv/mm.h @@ -22,9 +22,7 @@ static inline l1_pgentry_t guest_get_eff toggle_guest_pt(curr); if ( unlikely(!__addr_ok(linear)) || - copy_from_unsafe(&l1e, - &__linear_l1_table[l1_linear_offset(linear)], - sizeof(l1_pgentry_t)) ) + get_unsafe(l1e, &__linear_l1_table[l1_linear_offset(linear)]) ) l1e = l1e_empty(); if ( user_mode )
The former expands to a single (memory accessing) insn, which the latter does not guarantee. Yet we'd prefer to read consistent PTEs rather than risking a split read racing with an update done elsewhere. Signed-off-by: Jan Beulich <jbeulich@suse.com>