Re-sending primarily for the purpose of getting a release ack, an
explicit release nak, or an indication of there not being a need,
all for at least the first three patches here (which are otherwise
ready to go in). I've dropped the shadow part of the series from
this re-submission, because it has all got reviewed by Tim already
and is intended for 4.16 only anyway. I'm re-including the follow
up patches getting the code base in consistent shape again, as I
continue to think this consistency goal is at least worth a
consideration towards a freeze exception.

1: split __{get,put}_user() into "guest" and "unsafe" variants
2: split __copy_{from,to}_user() into "guest" and "unsafe" variants
3: PV: harden guest memory accesses against speculative abuse
4: rename {get,put}_user() to {get,put}_guest()
5: gdbsx: convert "user" to "guest" accesses
6: rename copy_{from,to}_user() to copy_{from,to}_guest_pv()
7: move stac()/clac() from {get,put}_unsafe_asm() ...
8: PV: use get_unsafe() instead of copy_from_unsafe()

Jan

Jan Beulich writes ("[PATCH v2 0/8] x86/PV: avoid speculation abuse through guest accessors"):
> Re-sending primarily for the purpose of getting a release ack, an
> explicit release nak, or an indication of there not being a need,
> all for at least the first three patches here (which are otherwise
> ready to go in). I've dropped the shadow part of the series from
> this re-submission, because it has all got reviewed by Tim already
> and is intended for 4.16 only anyway. I'm re-including the follow
> up patches getting the code base in consistent shape again, as I
> continue to think this consistency goal is at least worth a
> consideration towards a freeze exception.
> 
> 1: split __{get,put}_user() into "guest" and "unsafe" variants
> 2: split __copy_{from,to}_user() into "guest" and "unsafe" variants
> 3: PV: harden guest memory accesses against speculative abuse

These three:

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

On the grounds that this is probably severe enough to be a blocking
issue for 4.15.

> 4: rename {get,put}_user() to {get,put}_guest()
> 5: gdbsx: convert "user" to "guest" accesses
> 6: rename copy_{from,to}_user() to copy_{from,to}_guest_pv()
> 7: move stac()/clac() from {get,put}_unsafe_asm() ...
> 8: PV: use get_unsafe() instead of copy_from_unsafe()

These have not got a maintainer review yet.  To grant a release-ack
I'd like an explanation of the downsides and upsides of taking this
series in 4.15 ?

You say "consistency" but in practical terms, what will happen if the
code is not "conxistent" in this sense ?

I'd also like to hear from aother hypervisor maintainer.

Thanks,
Ian.

On 19.02.2021 16:50, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH v2 0/8] x86/PV: avoid speculation abuse through guest accessors"):
>> Re-sending primarily for the purpose of getting a release ack, an
>> explicit release nak, or an indication of there not being a need,
>> all for at least the first three patches here (which are otherwise
>> ready to go in). I've dropped the shadow part of the series from
>> this re-submission, because it has all got reviewed by Tim already
>> and is intended for 4.16 only anyway. I'm re-including the follow
>> up patches getting the code base in consistent shape again, as I
>> continue to think this consistency goal is at least worth a
>> consideration towards a freeze exception.
>>
>> 1: split __{get,put}_user() into "guest" and "unsafe" variants
>> 2: split __copy_{from,to}_user() into "guest" and "unsafe" variants
>> 3: PV: harden guest memory accesses against speculative abuse
> 
> These three:
> 
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> 
> On the grounds that this is probably severe enough to be a blocking
> issue for 4.15.

Thanks.

>> 4: rename {get,put}_user() to {get,put}_guest()
>> 5: gdbsx: convert "user" to "guest" accesses
>> 6: rename copy_{from,to}_user() to copy_{from,to}_guest_pv()
>> 7: move stac()/clac() from {get,put}_unsafe_asm() ...
>> 8: PV: use get_unsafe() instead of copy_from_unsafe()
> 
> These have not got a maintainer review yet.  To grant a release-ack
> I'd like an explanation of the downsides and upsides of taking this
> series in 4.15 ?
> 
> You say "consistency" but in practical terms, what will happen if the
> code is not "conxistent" in this sense ?

Patches 4-6: The code is harder to understand with the mix of names.
Backports from future versions to 4.15 may require more attention to
get right (and then again the same level of attention when moving to
4.14).

Patches 7 is simply a minor improvement. Patch 8 is an equivalent
of the one patch of the earlier version which has already gone in.
Just like that other one it's more to avoid a latent issue than any
active one.

Jan

Jan Beulich writes ("Re: [PATCH v2 0/8] x86/PV: avoid speculation abuse through guest accessors"):
> On 19.02.2021 16:50, Ian Jackson wrote:
> > You say "consistency" but in practical terms, what will happen if the
> > code is not "conxistent" in this sense ?
> 
> Patches 4-6: The code is harder to understand with the mix of names.
> Backports from future versions to 4.15 may require more attention to
> get right (and then again the same level of attention when moving to
> 4.14).
> 
> Patches 7 is simply a minor improvement. Patch 8 is an equivalent
> of the one patch of the earlier version which has already gone in.
> Just like that other one it's more to avoid a latent issue than any
> active one.

Thank you for this clear explanation.

I think 4-6 and 8 are good candidates for the reasons you give, and
because they seem low risk to me.  Have you used any automatic
techniques to check that there is no unintentional codegen change ?
(Eg, binary diffs, diffing sedderied versions, or something.)

To my naive eye patch 7 looks scary because it might be moving the
scope of a critical section.  Am I wrong about that ?

Ian.

On 19.02.2021 17:13, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v2 0/8] x86/PV: avoid speculation abuse through guest accessors"):
>> On 19.02.2021 16:50, Ian Jackson wrote:
>>> You say "consistency" but in practical terms, what will happen if the
>>> code is not "conxistent" in this sense ?
>>
>> Patches 4-6: The code is harder to understand with the mix of names.
>> Backports from future versions to 4.15 may require more attention to
>> get right (and then again the same level of attention when moving to
>> 4.14).
>>
>> Patches 7 is simply a minor improvement. Patch 8 is an equivalent
>> of the one patch of the earlier version which has already gone in.
>> Just like that other one it's more to avoid a latent issue than any
>> active one.
> 
> Thank you for this clear explanation.
> 
> I think 4-6 and 8 are good candidates for the reasons you give, and
> because they seem low risk to me.  Have you used any automatic
> techniques to check that there is no unintentional codegen change ?
> (Eg, binary diffs, diffing sedderied versions, or something.)

I did some manual inspection at the time of putting together that
work, but nothing further to be honest.

> To my naive eye patch 7 looks scary because it might be moving the
> scope of a critical section.  Am I wrong about that ?

At the source level it moves things, yes. Generated code, again as
per manual inspection, doesn't change, due to the pieces that the
compiler is able to eliminate. So I guess it's not as scary as it
may look.

Jan

Jan Beulich writes ("Re: [PATCH v2 0/8] x86/PV: avoid speculation abuse through guest accessors"):
> On 19.02.2021 17:13, Ian Jackson wrote:
> > Jan Beulich writes ("Re: [PATCH v2 0/8] x86/PV: avoid speculation abuse through guest accessors"):
> > I think 4-6 and 8 are good candidates for the reasons you give, and
> > because they seem low risk to me.  Have you used any automatic
> > techniques to check that there is no unintentional codegen change ?
> > (Eg, binary diffs, diffing sedderied versions, or something.)
> 
> I did some manual inspection at the time of putting together that
> work, but nothing further to be honest.

I think that something automatic might be worthwhile, but I would like
an opinion from another hypervisor maintainer about the level of risk
posed by the possibility of manual slips.  Eg, how likely it would be
for the compiler to catch them.

> > To my naive eye patch 7 looks scary because it might be moving the
> > scope of a critical section.  Am I wrong about that ?
> 
> At the source level it moves things, yes. Generated code, again as
> per manual inspection, doesn't change, due to the pieces that the
> compiler is able to eliminate. So I guess it's not as scary as it
> may look.

Oh, you eyeballed the generated code ?  Cool.

Ian.

On 19.02.2021 16:50, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH v2 0/8] x86/PV: avoid speculation abuse through guest accessors"):
>> Re-sending primarily for the purpose of getting a release ack, an
>> explicit release nak, or an indication of there not being a need,
>> all for at least the first three patches here (which are otherwise
>> ready to go in). I've dropped the shadow part of the series from
>> this re-submission, because it has all got reviewed by Tim already
>> and is intended for 4.16 only anyway. I'm re-including the follow
>> up patches getting the code base in consistent shape again, as I
>> continue to think this consistency goal is at least worth a
>> consideration towards a freeze exception.
>>
>> 1: split __{get,put}_user() into "guest" and "unsafe" variants
>> 2: split __copy_{from,to}_user() into "guest" and "unsafe" variants
>> 3: PV: harden guest memory accesses against speculative abuse
> 
> These three:
> 
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> 
> On the grounds that this is probably severe enough to be a blocking
> issue for 4.15.
> 
>> 4: rename {get,put}_user() to {get,put}_guest()
>> 5: gdbsx: convert "user" to "guest" accesses
>> 6: rename copy_{from,to}_user() to copy_{from,to}_guest_pv()
>> 7: move stac()/clac() from {get,put}_unsafe_asm() ...
>> 8: PV: use get_unsafe() instead of copy_from_unsafe()
> 
> These have not got a maintainer review yet.  To grant a release-ack
> I'd like an explanation of the downsides and upsides of taking this
> series in 4.15 ?
> 
> You say "consistency" but in practical terms, what will happen if the
> code is not "conxistent" in this sense ?
> 
> I'd also like to hear from aother hypervisor maintainer.

Meanwhile they have been reviewed by Roger. Are you willing to
give them, perhaps with the exception of 7, a release ack as
well?

Jan

Jan Beulich writes ("Re: [PATCH v2 0/8] x86/PV: avoid speculation abuse through guest accessors"):
> On 19.02.2021 16:50, Ian Jackson wrote:
> > Jan Beulich writes ("[PATCH v2 0/8] x86/PV: avoid speculation abuse through guest accessors"):
> >> 4: rename {get,put}_user() to {get,put}_guest()
> >> 5: gdbsx: convert "user" to "guest" accesses
> >> 6: rename copy_{from,to}_user() to copy_{from,to}_guest_pv()
> >> 7: move stac()/clac() from {get,put}_unsafe_asm() ...
> >> 8: PV: use get_unsafe() instead of copy_from_unsafe()
> > 
> > These have not got a maintainer review yet.  To grant a release-ack
> > I'd like an explanation of the downsides and upsides of taking this
> > series in 4.15 ?
> > 
> > You say "consistency" but in practical terms, what will happen if the
> > code is not "conxistent" in this sense ?
> > 
> > I'd also like to hear from aother hypervisor maintainer.
> 
> Meanwhile they have been reviewed by Roger. Are you willing to
> give them, perhaps with the exception of 7, a release ack as
> well?

Sorry, yes.

I found these explanations convincing  Thank you.

For all except 7,
Release-Acked-by: Ian Jackson <iwj@xenproject.org>

For 7, I remember what I think was an IRC conversation where someone
(you, I think) said you had examined the generated asm and it was
unchanged.

If I have remembered that correctly, then for 7 as well:
Release-Acked-by: Ian Jackson <iwj@xenproject.org>

If I have misremembered please do say.

Ian.

On 24.02.2021 14:08, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v2 0/8] x86/PV: avoid speculation abuse through guest accessors"):
>> On 19.02.2021 16:50, Ian Jackson wrote:
>>> Jan Beulich writes ("[PATCH v2 0/8] x86/PV: avoid speculation abuse through guest accessors"):
>>>> 4: rename {get,put}_user() to {get,put}_guest()
>>>> 5: gdbsx: convert "user" to "guest" accesses
>>>> 6: rename copy_{from,to}_user() to copy_{from,to}_guest_pv()
>>>> 7: move stac()/clac() from {get,put}_unsafe_asm() ...
>>>> 8: PV: use get_unsafe() instead of copy_from_unsafe()
>>>
>>> These have not got a maintainer review yet.  To grant a release-ack
>>> I'd like an explanation of the downsides and upsides of taking this
>>> series in 4.15 ?
>>>
>>> You say "consistency" but in practical terms, what will happen if the
>>> code is not "conxistent" in this sense ?
>>>
>>> I'd also like to hear from aother hypervisor maintainer.
>>
>> Meanwhile they have been reviewed by Roger. Are you willing to
>> give them, perhaps with the exception of 7, a release ack as
>> well?
> 
> Sorry, yes.
> 
> I found these explanations convincing  Thank you.
> 
> For all except 7,
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Thanks.

> For 7, I remember what I think was an IRC conversation where someone
> (you, I think) said you had examined the generated asm and it was
> unchanged.

It was in email, and I've inspected only some example of the
generated asm, not all instances. I would hope that was
sufficient, but since I'm not entirely certain ...

> If I have remembered that correctly, then for 7 as well:
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>

... I'll better wait for explicit confirmation of this.

Jan

Jan Beulich writes ("Re: [PATCH v2 0/8] x86/PV: avoid speculation abuse through guest accessors"):
> On 24.02.2021 14:08, Ian Jackson wrote:
> > For 7, I remember what I think was an IRC conversation where someone
> > (you, I think) said you had examined the generated asm and it was
> > unchanged.
> 
> It was in email, and I've inspected only some example of the
> generated asm, not all instances. I would hope that was
> sufficient, but since I'm not entirely certain ...
> 
> > If I have remembered that correctly, then for 7 as well:
> > Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> 
> ... I'll better wait for explicit confirmation of this.

I think that's convincing enough.  Thank you.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>