diff mbox series

[v2,8/8] x86/PV: use get_unsafe() instead of copy_from_unsafe()

Message ID 0a59ae2f-448e-610d-e8a2-a7c3f9f3918f@suse.com (mailing list archive)
State New
Headers show
Series x86/PV: avoid speculation abuse through guest accessors | expand

Commit Message

Jan Beulich Feb. 17, 2021, 8:23 a.m. UTC
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>

Comments

Roger Pau Monne Feb. 23, 2021, 11:59 a.m. UTC | #1
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.
Jan Beulich Feb. 23, 2021, 3:25 p.m. UTC | #2
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
Roger Pau Monne Feb. 23, 2021, 3:37 p.m. UTC | #3
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.
Jan Beulich Feb. 23, 2021, 4:13 p.m. UTC | #4
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
Roger Pau Monne Feb. 23, 2021, 6:03 p.m. UTC | #5
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.
diff mbox series

Patch

--- 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 )