| Message ID | 1554832427-30567-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/msr: Fix fallout from mostly c/s 832c180 | expand |
On Tue, Apr 09, 2019 at 06:53:47PM +0100, Andrew Cooper wrote: > The series 832c1803^..f61685a6 was committed without adequate review. > > * Fix the shim build by providing a !CONFIG_HVM declaration for > hvm_get_guest_bndcfgs() > * Revert the bogus de-const'ing of the vcpu pointer in > vmx_get_guest_bndcfgs(). vmx_vmcs_enter() really does mutate the vcpu, and > may cause it to undergo a full de/reschedule, which is in violation of the > ABI described by hvm_get_guest_bndcfgs(). guest_rdmsr() was always going > to need to lose its const parameter, and this was the correct time for it > to happen. Enlighten me -- why does guest_rdmsr need to lose its const parameter? This reads as if the code was buggy from the get-go. Wei.
> -----Original Message----- > From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > Sent: 09 April 2019 18:54 > To: Xen-devel <xen-devel@lists.xen.org> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Wei Liu > <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; > Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com> > Subject: [PATCH] x86/msr: Fix fallout from mostly c/s 832c180 > > The series 832c1803^..f61685a6 was committed without adequate review. > v1 of the series was posted on 7th Jan and v4 on 14th March. It was committed yesterday. I'd say that there was certainly adequate time for review. > * Fix the shim build by providing a !CONFIG_HVM declaration for > hvm_get_guest_bndcfgs() > * Revert the bogus de-const'ing of the vcpu pointer in > vmx_get_guest_bndcfgs(). vmx_vmcs_enter() really does mutate the vcpu, and > may cause it to undergo a full de/reschedule, which is in violation of the > ABI described by hvm_get_guest_bndcfgs(). guest_rdmsr() was always going > to need to lose its const parameter, and this was the correct time for it > to happen. In the case of vmx_get_guest_bndcfgs() is there actually possibility of a re-schedule? It's either going to be in current context (in which case IIUC vmx_vmcs_enter() is going to be largely a no-op) or the vcpu in question should have already been paused. I'm no > * Remove the introduced ASSERT(is_hvm_domain(d)) and check the predicate > directly. While we expect it to be true, the result is potential type > confusion in release builds based on several subtle aspects of the CPUID > feature derivation logic with no other safety checks. This also fixes the > a linker error in the release build of the shim, again for !CONFIG_HVM > reasons. Again, digging back in mail... ----- [From Jan] > + case MSR_IA32_BNDCFGS: > + if ( !is_hvm_domain(d) || !cp->feat.mpx || > + !hvm_set_guest_bndcfgs(v, val) ) > + goto gp_fault; In both cases the is_hvm_*() check looks to be redundant, as for PV guests cp->feat.mpx can't be set. Personally I'd prefer this to be an ASSERT() instead, but I'd listen to Andrew (as the main author of this code) saying otherwise. ----- ...and I do recall asking for your opinion at the time. I guess you changed your mind. > * The MSRs in vcpu_msrs are in numeric order. Re-position XSS to match. > That was not at all obvious. If this is the case then there should be comment above the declaration of struct vcpu_msrs. > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> However, the changes look ok to me and brings things closer to my earlier code anyway so, with the comment requested above added... Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> -----Original Message----- > From: Paul Durrant > Sent: 10 April 2019 09:28 > To: Andrew Cooper <Andrew.Cooper3@citrix.com>; Xen-devel <xen-devel@lists.xen.org> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Wei Liu > <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; > Kevin Tian <kevin.tian@intel.com> > Subject: RE: [PATCH] x86/msr: Fix fallout from mostly c/s 832c180 > > > -----Original Message----- > > From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > > Sent: 09 April 2019 18:54 > > To: Xen-devel <xen-devel@lists.xen.org> > > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Wei Liu > > <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant > <Paul.Durrant@citrix.com>; > > Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com> > > Subject: [PATCH] x86/msr: Fix fallout from mostly c/s 832c180 > > > > The series 832c1803^..f61685a6 was committed without adequate review. > > > > v1 of the series was posted on 7th Jan and v4 on 14th March. It was committed yesterday. I'd say that > there was certainly adequate time for review. > > > * Fix the shim build by providing a !CONFIG_HVM declaration for > > hvm_get_guest_bndcfgs() > > * Revert the bogus de-const'ing of the vcpu pointer in > > vmx_get_guest_bndcfgs(). vmx_vmcs_enter() really does mutate the vcpu, and > > may cause it to undergo a full de/reschedule, which is in violation of the > > ABI described by hvm_get_guest_bndcfgs(). guest_rdmsr() was always going > > to need to lose its const parameter, and this was the correct time for it > > to happen. > > In the case of vmx_get_guest_bndcfgs() is there actually possibility of a re-schedule? It's either > going to be in current context (in which case IIUC vmx_vmcs_enter() is going to be largely a no-op) or > the vcpu in question should have already been paused. I'm no ^^^ that got truncated thanks to exchange going bye bye... I was just about to say I was no particular fan of the de-consting trick but the mail thread was: ----- >> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Jan Beulich >> Sent: 13 March 2019 15:55 >> >> >>> On 11.03.19 at 19:09, <paul.durrant@citrix.com> wrote: >> > --- a/xen/include/asm-x86/msr.h >> > +++ b/xen/include/asm-x86/msr.h >> > @@ -328,7 +328,7 @@ int init_vcpu_msr_policy(struct vcpu *v); >> > * These functions are also used by the migration logic, so need to cope with >> > * being used outside of v's context. >> > */ >> > -int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val); >> > +int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val); >> >> I find this pretty undesirable, and I'd like to at least put out >> for discussion a means how to avoid it: Any entity being >> passed a const struct vcpu *cv can get hold of a non-const >> one by doing >> >> struct vcpu *v = cv->domain->vcpu[cv->vcpu_id]; >> > > Looks kind of odd, but of course it will certainly work. > >> Of course this shouldn't be used arbitrarily, but to hide an >> implementation detail like that of vmx_vmcs_enter() I think >> this could be justified. Thoughts? >> > > I guess the question is at what level to do this? Probably in the hvm code > rather than in the vmx code. I think it should be in VMX code, as that's where the vmx_vmcs_enter() oddity lives. ----- So you can see Jan asked for other opinions at the time. None were forthcoming. I therefore re-coded as he requested. > > > * Remove the introduced ASSERT(is_hvm_domain(d)) and check the predicate > > directly. While we expect it to be true, the result is potential type > > confusion in release builds based on several subtle aspects of the CPUID > > feature derivation logic with no other safety checks. This also fixes the > > a linker error in the release build of the shim, again for !CONFIG_HVM > > reasons. > > Again, digging back in mail... > > ----- > [From Jan] > > + case MSR_IA32_BNDCFGS: > > + if ( !is_hvm_domain(d) || !cp->feat.mpx || > > + !hvm_set_guest_bndcfgs(v, val) ) > > + goto gp_fault; > > In both cases the is_hvm_*() check looks to be redundant, as > for PV guests cp->feat.mpx can't be set. Personally I'd prefer > this to be an ASSERT() instead, but I'd listen to Andrew (as > the main author of this code) saying otherwise. > ----- > > ...and I do recall asking for your opinion at the time. I guess you changed your mind. > > > * The MSRs in vcpu_msrs are in numeric order. Re-position XSS to match. > > > > That was not at all obvious. If this is the case then there should be comment above the declaration of > struct vcpu_msrs. > > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > However, the changes look ok to me and brings things closer to my earlier code anyway so, with the > comment requested above added... > > Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
On 10/04/2019 09:23, Wei Liu wrote: > On Tue, Apr 09, 2019 at 06:53:47PM +0100, Andrew Cooper wrote: >> The series 832c1803^..f61685a6 was committed without adequate review. >> >> * Fix the shim build by providing a !CONFIG_HVM declaration for >> hvm_get_guest_bndcfgs() >> * Revert the bogus de-const'ing of the vcpu pointer in >> vmx_get_guest_bndcfgs(). vmx_vmcs_enter() really does mutate the vcpu, and >> may cause it to undergo a full de/reschedule, which is in violation of the >> ABI described by hvm_get_guest_bndcfgs(). guest_rdmsr() was always going >> to need to lose its const parameter, and this was the correct time for it >> to happen. > Enlighten me -- why does guest_rdmsr need to lose its const parameter? > > This reads as if the code was buggy from the get-go. It was originally written without a const vcpu *, in full knowledge that when moving onto some of the less trivial MSRs, it would need a mutable parameter. Of course, this wasn't considered a good enough argument during review, and to avoid an argument, it went in with a const parameter. The state of the MSRs may live in various memory structures, or directly in hardware (covers the two cases thus far), or in the VMCS/VMCB, (or worse, the MSR load/save lists). This patch adds the first example of needing to access data straight from the VMCS, which involves negotiating with the scheduler for safe access. ~Andrew
On Wed, Apr 10, 2019 at 10:41:24AM +0100, Andrew Cooper wrote: > On 10/04/2019 09:23, Wei Liu wrote: > > On Tue, Apr 09, 2019 at 06:53:47PM +0100, Andrew Cooper wrote: > >> The series 832c1803^..f61685a6 was committed without adequate review. > >> > >> * Fix the shim build by providing a !CONFIG_HVM declaration for > >> hvm_get_guest_bndcfgs() > >> * Revert the bogus de-const'ing of the vcpu pointer in > >> vmx_get_guest_bndcfgs(). vmx_vmcs_enter() really does mutate the vcpu, and > >> may cause it to undergo a full de/reschedule, which is in violation of the > >> ABI described by hvm_get_guest_bndcfgs(). guest_rdmsr() was always going > >> to need to lose its const parameter, and this was the correct time for it > >> to happen. > > Enlighten me -- why does guest_rdmsr need to lose its const parameter? > > > > This reads as if the code was buggy from the get-go. > > It was originally written without a const vcpu *, in full knowledge that > when moving onto some of the less trivial MSRs, it would need a mutable > parameter. > > Of course, this wasn't considered a good enough argument during review, > and to avoid an argument, it went in with a const parameter. > > The state of the MSRs may live in various memory structures, or directly > in hardware (covers the two cases thus far), or in the VMCS/VMCB, (or > worse, the MSR load/save lists). This patch adds the first example of > needing to access data straight from the VMCS, which involves > negotiating with the scheduler for safe access. Thanks for the explanation. Wei. > > ~Andrew
>>> On 09.04.19 at 19:53, <andrew.cooper3@citrix.com> wrote: > The series 832c1803^..f61685a6 was committed without adequate review. > > * Fix the shim build by providing a !CONFIG_HVM declaration for > hvm_get_guest_bndcfgs() > * Revert the bogus de-const'ing of the vcpu pointer in > vmx_get_guest_bndcfgs(). vmx_vmcs_enter() really does mutate the vcpu, and > may cause it to undergo a full de/reschedule, which is in violation of the > ABI described by hvm_get_guest_bndcfgs(). guest_rdmsr() was always going > to need to lose its const parameter, and this was the correct time for it > to happen. I'd like to ask for a better explanation of the actual issue you see here. By declaring a function parameter pointer to const, nothing tells the compiler that the object may not change (e.g. across function calls made out of this function). All it tells the compiler is that the function promises to not itself alter the pointed to object. Paul has pointed you to the respective earlier discussion, and as you can see when suggesting the alternative I was assuming this might not be liked. But that's then still a matter of taste, not one of correctness, at least until you explicitly call out the correctness issue I'm not seeing. As an aside, I continue to think this de-constification should happen in vmx_vmcs_try_enter(). To the outside world the function is not supposed to alter the struct vcpu it's being handed. It's an implementation detail that in fact it transiently has to. > * Remove the introduced ASSERT(is_hvm_domain(d)) and check the predicate > directly. While we expect it to be true, the result is potential type > confusion in release builds based on several subtle aspects of the CPUID > feature derivation logic with no other safety checks. This also fixes the > a linker error in the release build of the shim, again for !CONFIG_HVM > reasons. I don't understand "no other safety checks": To me the "S" in XEN_CPUFEATURE(MPX, 5*32+14) /*S Memory Protection Extensions */ is clear enough. While perhaps not towards "potential type confusion" as you word it, there are other cases where we make implications from the scope stated in the public header: MSR_FLUSH_CMD, for example, is supposed to be inaccessible to PV guests, but there's no explicit !PV check in its handling code. I would call the current state as inconsistent (seeing e.g. guest_{rd,wr}msr_x2apic() again being behind is_hvm_domain() checks), and hence it's not really possible to derive in which case which approach is to be preferred (or, as in the case here, would be objected to). With this it is really quite important for you to voice an opinion in a timely manner. Or if you want to avoid such a dependency on you, provide a clear "recipe" for when to use what. All of the above said, the code changes themselves all look fine to me; all I'm therefore asking for is better reasoning. In the event you disagree and think it's all obvious anyway, perhaps it would be a good idea to split off the build fix, which can have my A-b right away. Jan
On 10/04/2019 11:24, Jan Beulich wrote: >>>> On 09.04.19 at 19:53, <andrew.cooper3@citrix.com> wrote: >> The series 832c1803^..f61685a6 was committed without adequate review. >> >> * Fix the shim build by providing a !CONFIG_HVM declaration for >> hvm_get_guest_bndcfgs() >> * Revert the bogus de-const'ing of the vcpu pointer in >> vmx_get_guest_bndcfgs(). vmx_vmcs_enter() really does mutate the vcpu, and >> may cause it to undergo a full de/reschedule, which is in violation of the >> ABI described by hvm_get_guest_bndcfgs(). guest_rdmsr() was always going >> to need to lose its const parameter, and this was the correct time for it >> to happen. > I'd like to ask for a better explanation of the actual issue you see > here. By declaring a function parameter pointer to const, nothing > tells the compiler that the object may not change (e.g. across > function calls made out of this function). False. > All it tells the compiler is > that the function promises to not itself alter the pointed to object. Nonsense. The function promising not to alter the pointed-to-object includes the entire child callgraph. The code you insisted Paul to add is: struct vcpu *v = cv->domain->vcpu[cv->vcpu_id]; which is identical to: struct vcpu *v = (struct vcpu *)cv; Which highlights very clearly that this function has undefined behaviour. An optimising compiler which uses an object, and passes a const pointer to that object to a function, is permitted to retain assumptions derived from that state across the function call sequence point, because the ABI of the function states that the content of the object doesn't change. But if you'd prefer a different argument, how about a contradiction. By your interpretation, the const keyword is utterly useless because a compiler must treat all const pointers as non-const, because the pointed-to object can change in any arbitrary way at any point. If this were the intended interpretation, const would never have been added to the C language because it would waste space in the compiler for 0 gain. The fact it was added demonstrates that it had real material gains, which means it isn't a useless keyword, which means the compiler really may depend on the content of a const pointed-to-object not changing at all. > >> * Remove the introduced ASSERT(is_hvm_domain(d)) and check the predicate >> directly. While we expect it to be true, the result is potential type >> confusion in release builds based on several subtle aspects of the CPUID >> feature derivation logic with no other safety checks. This also fixes the >> a linker error in the release build of the shim, again for !CONFIG_HVM >> reasons. > I don't understand "no other safety checks": To me the "S" in > > XEN_CPUFEATURE(MPX, 5*32+14) /*S Memory Protection Extensions */ > > is clear enough. While perhaps not towards "potential type confusion" "type confusion" here is mixing up v->arch.hvm and v->arch.pv, which is what happens when you've actually got a PV vcpu and you call an hvm_* function. "No other safety checks" means that cp->feat.mpx becoming accidentally set results in bad things happening if a PV and HVM vcpu get mixed up. It is same reasoning which causes us to do this: if ( !cond ) { ASSERT_UNREACHABLE(); goto something_safer; } rather than ASSERT(cond) > as you word it, there are other cases where we make implications > from the scope stated in the public header: MSR_FLUSH_CMD, for > example, is supposed to be inaccessible to PV guests, but there's no > explicit !PV check in its handling code. Nothing with the handling of FLUSH_CMD gets into any form of UB whatsoever if cp->feat.l1d_flush becomes accidentally set for a PV guest. > I would call the current state > as inconsistent (seeing e.g. guest_{rd,wr}msr_x2apic() again being > behind is_hvm_domain() checks), and hence it's not really possible > to derive in which case which approach is to be preferred (or, as in > the case here, would be objected to). The very first thing guest_{rd,wr}msr_x2apic() does is operate on v->arch.hvm Anyway, as was included in the bullet point, the is_hvm_domain() check is a critical part of making the shim build work, given that it depends on dead code elimination. Omitting the is_hvm_domain() check really does result in a link error. ~Andrew
On 10/04/2019 09:28, Paul Durrant wrote: > Again, digging back in mail... > > ----- > [From Jan] >> + case MSR_IA32_BNDCFGS: >> + if ( !is_hvm_domain(d) || !cp->feat.mpx || >> + !hvm_set_guest_bndcfgs(v, val) ) >> + goto gp_fault; > In both cases the is_hvm_*() check looks to be redundant, as > for PV guests cp->feat.mpx can't be set. Personally I'd prefer > this to be an ASSERT() instead, but I'd listen to Andrew (as > the main author of this code) saying otherwise. In this case, the compiler/linkers opinion in the shim build does provide a very clear answer. The check is not redundant and must remain. ~Andrew
>>> On 11.04.19 at 20:20, <andrew.cooper3@citrix.com> wrote: > On 10/04/2019 11:24, Jan Beulich wrote: >>>>> On 09.04.19 at 19:53, <andrew.cooper3@citrix.com> wrote: >>> The series 832c1803^..f61685a6 was committed without adequate review. >>> >>> * Fix the shim build by providing a !CONFIG_HVM declaration for >>> hvm_get_guest_bndcfgs() >>> * Revert the bogus de-const'ing of the vcpu pointer in >>> vmx_get_guest_bndcfgs(). vmx_vmcs_enter() really does mutate the vcpu, and >>> may cause it to undergo a full de/reschedule, which is in violation of the >>> ABI described by hvm_get_guest_bndcfgs(). guest_rdmsr() was always going >>> to need to lose its const parameter, and this was the correct time for it >>> to happen. >> I'd like to ask for a better explanation of the actual issue you see >> here. By declaring a function parameter pointer to const, nothing >> tells the compiler that the object may not change (e.g. across >> function calls made out of this function). > > False. > >> All it tells the compiler is >> that the function promises to not itself alter the pointed to object. > > Nonsense. Excuse me? > The function promising not to alter the pointed-to-object > includes the entire child callgraph. > > > The code you insisted Paul to add is: > > struct vcpu *v = cv->domain->vcpu[cv->vcpu_id]; > > which is identical to: > > struct vcpu *v = (struct vcpu *)cv; It is not identical; it is having the same effective behavior when compiled with today's compilers. > Which highlights very clearly that this function has undefined behaviour. It doesn't, no. > An optimising compiler which uses an object, and passes a const pointer > to that object to a function, is permitted to retain assumptions derived > from that state across the function call sequence point, because the ABI > of the function states that the content of the object doesn't change. Very much not so, no. Take this simple (and granted contrived) example: int integer; int test(void) { func(&integer); return integer; } and in a different CU (just to avoid the effect of the compiler inlining the whole thing) void func(const int*pi) { integer = ~*pi; } Various other examples are possible, including ones where there's nothing contrived at all. > But if you'd prefer a different argument, how about a contradiction. > > By your interpretation, the const keyword is utterly useless because a > compiler must treat all const pointers as non-const, because the > pointed-to object can change in any arbitrary way at any point. If this > were the intended interpretation, const would never have been added to > the C language because it would waste space in the compiler for 0 gain. > > The fact it was added demonstrates that it had real material gains, > which means it isn't a useless keyword, which means the compiler really > may depend on the content of a const pointed-to-object not changing at all. I doubt this, and you provide no source where you take from that this was the intention. And despite what you say, "const" has its value nevertheless - it allows the compiler to tell you when a piece of code modifies an object that you didn't mean to alter. Quote from the language spec: "If an attempt is made to modify an object defined with a const- qualified type through use of an lvalue with non-const-qualified type, the behavior is undefined. If an attempt is made to refer to an object defined with a volatile-qualified type through use of an lvalue with non-volatile-qualified type, the behavior is undefined." And the respective footnote: "This applies to those objects that behave as if they were defined with qualified types, even if they are never actually defined as objects in the program (such as an object at a memory-mapped input/output address)." Throughout the verb used is "defined", not "declared". If any struct vcpu instance actually lived in .rodata (for example), then (without casting away constness) it would be impossible to construct a non-const pointer to it. Hence there would be no legitimate means to create a way to modify that instance. But that's specifically not the case here (or in the example given). >>> * Remove the introduced ASSERT(is_hvm_domain(d)) and check the predicate >>> directly. While we expect it to be true, the result is potential type >>> confusion in release builds based on several subtle aspects of the CPUID >>> feature derivation logic with no other safety checks. This also fixes the >>> a linker error in the release build of the shim, again for !CONFIG_HVM >>> reasons. >> I don't understand "no other safety checks": To me the "S" in >> >> XEN_CPUFEATURE(MPX, 5*32+14) /*S Memory Protection Extensions */ >> >> is clear enough. While perhaps not towards "potential type confusion" > > "type confusion" here is mixing up v->arch.hvm and v->arch.pv, which is > what happens when you've actually got a PV vcpu and you call an hvm_* > function. > > "No other safety checks" means that cp->feat.mpx becoming accidentally > set results in bad things happening if a PV and HVM vcpu get mixed up. If whatever bit / field / variable accidentally gains a wrong value, bad things are going to be happening. There's no escape from this. > It is same reasoning which causes us to do this: > > if ( !cond ) > { > ASSERT_UNREACHABLE(); > goto something_safer; > } > > rather than ASSERT(cond) Right, and just to remind you: I'm not objecting to the change itself. I dislike how you call it all broken without demonstrating its brokenness. I'd like to add though that this part of the patch description is perhaps acceptable, if there wasn't the overall initial statement regarding "adequate review". >> as you word it, there are other cases where we make implications >> from the scope stated in the public header: MSR_FLUSH_CMD, for >> example, is supposed to be inaccessible to PV guests, but there's no >> explicit !PV check in its handling code. > > Nothing with the handling of FLUSH_CMD gets into any form of UB > whatsoever if cp->feat.l1d_flush becomes accidentally set for a PV guest. Mind me adjusting this to "Nothing ... currently gets into ..."? >> I would call the current state >> as inconsistent (seeing e.g. guest_{rd,wr}msr_x2apic() again being >> behind is_hvm_domain() checks), and hence it's not really possible >> to derive in which case which approach is to be preferred (or, as in >> the case here, would be objected to). > > The very first thing guest_{rd,wr}msr_x2apic() does is operate on > v->arch.hvm But along the lines of the previous comment: I think the checking done in the caller should not depend on implementation details of the callee. It should be consistent in itself. > Anyway, as was included in the bullet point, the is_hvm_domain() check > is a critical part of making the shim build work, given that it depends > on dead code elimination. Omitting the is_hvm_domain() check really > does result in a link error. This is one way of addressing the build problem, but not the only one. Yet again - I'm okay with the code changes you propose, but please with a more civil (and, in the case of the const aspect, factually correct) description. Jan
> -----Original Message----- > > > An optimising compiler which uses an object, and passes a const pointer > > to that object to a function, is permitted to retain assumptions derived > > from that state across the function call sequence point, because the ABI > > of the function states that the content of the object doesn't change. > > Very much not so, no. Take this simple (and granted contrived) > example: > > int integer; > > int test(void) { > func(&integer); > return integer; > } > > and in a different CU (just to avoid the effect of the compiler > inlining the whole thing) > > void func(const int*pi) { > integer = ~*pi; > } > > Various other examples are possible, including ones where > there's nothing contrived at all. > I'm not sure whether the fact that 'integer' is accessed as a non-const global in the above makes any difference in this example but what you seem to be highlighting is that there's simply no point in ever using const struct vcpu pointers because the compiler can never actually do any useful optimization based on the const qualifier. Is that the case? Paul
>>> On 12.04.19 at 13:00, <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> >> > An optimising compiler which uses an object, and passes a const pointer >> > to that object to a function, is permitted to retain assumptions derived >> > from that state across the function call sequence point, because the ABI >> > of the function states that the content of the object doesn't change. >> >> Very much not so, no. Take this simple (and granted contrived) >> example: >> >> int integer; >> >> int test(void) { >> func(&integer); >> return integer; >> } >> >> and in a different CU (just to avoid the effect of the compiler >> inlining the whole thing) >> >> void func(const int*pi) { >> integer = ~*pi; >> } >> >> Various other examples are possible, including ones where >> there's nothing contrived at all. >> > > I'm not sure whether the fact that 'integer' is accessed as a non-const > global in the above makes any difference in this example but what you seem to > be highlighting is that there's simply no point in ever using const struct > vcpu pointers because the compiler can never actually do any useful > optimization based on the const qualifier. Is that the case? Not exactly: Using const indeed doesn't help code generation. The compiler can't assume objects pointed to by a pointer-to- const won't change (e.g. across function calls). But as said in my earlier reply, there's still value of adding const in that it allows the compiler to spot programming mistakes. Jan
On 12/04/2019 11:46, Jan Beulich wrote: > >> The function promising not to alter the pointed-to-object >> includes the entire child callgraph. >> >> >> The code you insisted Paul to add is: >> >> struct vcpu *v = cv->domain->vcpu[cv->vcpu_id]; >> >> which is identical to: >> >> struct vcpu *v = (struct vcpu *)cv; > It is not identical; it is having the same effective behavior when > compiled with today's compilers. > >> Which highlights very clearly that this function has undefined behaviour. > It doesn't, no. Yes it literally does, and even in the very first sentence you quoted. Reproduced here: > "If an attempt is made to modify an object defined with a const- > qualified type through use of an lvalue with non-const-qualified > type, the behavior is undefined. There is exactly one object for this vcpu. *cv, as defined by the prototype, is const qualified, and is this object. *v is the same object, and mutates it. C doesn't necessarily know that "cv->domain->vcpu[cv->vcpu_id] == cv", but it really is an alias in practice, and therefore is UB under that rule. > >> An optimising compiler which uses an object, and passes a const pointer >> to that object to a function, is permitted to retain assumptions derived >> from that state across the function call sequence point, because the ABI >> of the function states that the content of the object doesn't change. > Very much not so, no. Take this simple (and granted contrived) > example: > > int integer; > > int test(void) { > func(&integer); > return integer; > } > > and in a different CU (just to avoid the effect of the compiler > inlining the whole thing) > > void func(const int*pi) { > integer = ~*pi; > } > > Various other examples are possible, including ones where > there's nothing contrived at all. How about a concrete example which matches the code pattern under argument and demonstrates the issue. void func(const int *pi) { int *i = (int *)pi; *i = 6; } And in a separate translation unit. int test(void) { const int i = 4; func(&i); assert(i == 4); return i; } Funnily enough, the assert never triggers. Even at -O0, it never gets compiled in and test has its return value in the form `mov $4, %eax; ret`, and the only way that occurs is because of the UB. > >> But if you'd prefer a different argument, how about a contradiction. >> >> By your interpretation, the const keyword is utterly useless because a >> compiler must treat all const pointers as non-const, because the >> pointed-to object can change in any arbitrary way at any point. If this >> were the intended interpretation, const would never have been added to >> the C language because it would waste space in the compiler for 0 gain. >> >> The fact it was added demonstrates that it had real material gains, >> which means it isn't a useless keyword, which means the compiler really >> may depend on the content of a const pointed-to-object not changing at all. > I doubt this, and you provide no source where you take from that > this was the intention. And despite what you say, "const" has its > value nevertheless - it allows the compiler to tell you when a piece > of code modifies an object that you didn't mean to alter. > > Quote from the language spec: > > "If an attempt is made to modify an object defined with a const- > qualified type through use of an lvalue with non-const-qualified > type, the behavior is undefined. If an attempt is made to refer > to an object defined with a volatile-qualified type through use > of an lvalue with non-volatile-qualified type, the behavior is > undefined." > > And the respective footnote: > > "This applies to those objects that behave as if they were > defined with qualified types, even if they are never actually > defined as objects in the program (such as an object at a > memory-mapped input/output address)." > > Throughout the verb used is "defined", not "declared". If any > struct vcpu instance actually lived in .rodata (for example), > then (without casting away constness) it would be impossible to > construct a non-const pointer to it. Hence there would be no > legitimate means to create a way to modify that instance. But > that's specifically not the case here (or in the example given). A function which takes a const vcpu* does not know, and has no way of proving, that the object really wasn't const. I will admit that I made a made a mistake with the optimisation claim. The outer function, because it can't see the declaration of the object itself, also can't assume there aren't other aliases. But none of this stops the casting away of const being UB, and it still remains completely dishonest programming to declare vmx_set_guest_bndcfgs() as taking a const vcpu, and then modifying it. > >>>> * Remove the introduced ASSERT(is_hvm_domain(d)) and check the predicate >>>> directly. While we expect it to be true, the result is potential type >>>> confusion in release builds based on several subtle aspects of the CPUID >>>> feature derivation logic with no other safety checks. This also fixes the >>>> a linker error in the release build of the shim, again for !CONFIG_HVM >>>> reasons. >>> I don't understand "no other safety checks": To me the "S" in >>> >>> XEN_CPUFEATURE(MPX, 5*32+14) /*S Memory Protection Extensions */ >>> >>> is clear enough. While perhaps not towards "potential type confusion" >> "type confusion" here is mixing up v->arch.hvm and v->arch.pv, which is >> what happens when you've actually got a PV vcpu and you call an hvm_* >> function. >> >> "No other safety checks" means that cp->feat.mpx becoming accidentally >> set results in bad things happening if a PV and HVM vcpu get mixed up. > If whatever bit / field / variable accidentally gains a wrong value, > bad things are going to be happening. There's no escape from this. Agreed, but we can take active steps to limit the fallout, and this how guest_{cpuid,rdmsr,wrmsr}() have been coded thus far. >>> as you word it, there are other cases where we make implications >>> from the scope stated in the public header: MSR_FLUSH_CMD, for >>> example, is supposed to be inaccessible to PV guests, but there's no >>> explicit !PV check in its handling code. >> Nothing with the handling of FLUSH_CMD gets into any form of UB >> whatsoever if cp->feat.l1d_flush becomes accidentally set for a PV guest. > Mind me adjusting this to "Nothing ... currently gets into ..."? Fair enough, but as this is a write-only MSR, I don't expect it to change moving forwards. > >>> I would call the current state >>> as inconsistent (seeing e.g. guest_{rd,wr}msr_x2apic() again being >>> behind is_hvm_domain() checks), and hence it's not really possible >>> to derive in which case which approach is to be preferred (or, as in >>> the case here, would be objected to). >> The very first thing guest_{rd,wr}msr_x2apic() does is operate on >> v->arch.hvm > But along the lines of the previous comment: I think the checking > done in the caller should not depend on implementation details of > the callee. It should be consistent in itself. I'm starting to regret removing the hvm_ prefix from these functions, which at least makes the caller side of things more obvious. Xen's existing style is to check before calling functions like this, rather than for the callees to check and bail. > >> Anyway, as was included in the bullet point, the is_hvm_domain() check >> is a critical part of making the shim build work, given that it depends >> on dead code elimination. Omitting the is_hvm_domain() check really >> does result in a link error. > This is one way of addressing the build problem, but not the only > one. But it is the one which is consistent with everywhere else in codebase. > Yet again - I'm okay with the code changes you propose, but > please with a more civil (and, in the case of the const aspect, > factually correct) description.probably Its no secret that I wrote this patch while very irritated, and the commit message can almost certainly be phrased better, but I see nothing which factually incorrect. ~Andrew
>>> On 12.04.19 at 15:52, <andrew.cooper3@citrix.com> wrote: > On 12/04/2019 11:46, Jan Beulich wrote: >> >>> The function promising not to alter the pointed-to-object >>> includes the entire child callgraph. >>> >>> >>> The code you insisted Paul to add is: >>> >>> struct vcpu *v = cv->domain->vcpu[cv->vcpu_id]; >>> >>> which is identical to: >>> >>> struct vcpu *v = (struct vcpu *)cv; >> It is not identical; it is having the same effective behavior when >> compiled with today's compilers. >> >>> Which highlights very clearly that this function has undefined behaviour. >> It doesn't, no. > > Yes it literally does, and even in the very first sentence you quoted. > > Reproduced here: > >> "If an attempt is made to modify an object defined with a const- >> qualified type through use of an lvalue with non-const-qualified >> type, the behavior is undefined. > > There is exactly one object for this vcpu. > > *cv, as defined by the prototype, is const qualified, and is this object. > > *v is the same object, and mutates it. But these are _declarations_, not _definitions_. > C doesn't necessarily know that "cv->domain->vcpu[cv->vcpu_id] == cv", > but it really is an alias in practice, and therefore is UB under that rule. As per above, this rule makes it UB only if the object _definition_ was (effectively, as per the footnote) const. >>> An optimising compiler which uses an object, and passes a const pointer >>> to that object to a function, is permitted to retain assumptions derived >>> from that state across the function call sequence point, because the ABI >>> of the function states that the content of the object doesn't change. >> Very much not so, no. Take this simple (and granted contrived) >> example: >> >> int integer; >> >> int test(void) { >> func(&integer); >> return integer; >> } >> >> and in a different CU (just to avoid the effect of the compiler >> inlining the whole thing) >> >> void func(const int*pi) { >> integer = ~*pi; >> } >> >> Various other examples are possible, including ones where >> there's nothing contrived at all. > > How about a concrete example which matches the code pattern under > argument and demonstrates the issue. > > void func(const int *pi) > { > int *i = (int *)pi; To give a valid example, you need to get away without casting away const-ness. This is the real difference between what you correctly name UB and what I had suggested while reviewing Paul's patch. > *i = 6; > } > > And in a separate translation unit. > > int test(void) > { > const int i = 4; > > func(&i); > assert(i == 4); > > return i; > } > > Funnily enough, the assert never triggers. Even at -O0, it never gets > compiled in and test has its return value in the form `mov $4, %eax; > ret`, and the only way that occurs is because of the UB. Sure - even without any optimization the compiler sees the const on the _definition_ of i. >>> But if you'd prefer a different argument, how about a contradiction. >>> >>> By your interpretation, the const keyword is utterly useless because a >>> compiler must treat all const pointers as non-const, because the >>> pointed-to object can change in any arbitrary way at any point. If this >>> were the intended interpretation, const would never have been added to >>> the C language because it would waste space in the compiler for 0 gain. >>> >>> The fact it was added demonstrates that it had real material gains, >>> which means it isn't a useless keyword, which means the compiler really >>> may depend on the content of a const pointed-to-object not changing at all. >> I doubt this, and you provide no source where you take from that >> this was the intention. And despite what you say, "const" has its >> value nevertheless - it allows the compiler to tell you when a piece >> of code modifies an object that you didn't mean to alter. >> >> Quote from the language spec: >> >> "If an attempt is made to modify an object defined with a const- >> qualified type through use of an lvalue with non-const-qualified >> type, the behavior is undefined. If an attempt is made to refer >> to an object defined with a volatile-qualified type through use >> of an lvalue with non-volatile-qualified type, the behavior is >> undefined." >> >> And the respective footnote: >> >> "This applies to those objects that behave as if they were >> defined with qualified types, even if they are never actually >> defined as objects in the program (such as an object at a >> memory-mapped input/output address)." >> >> Throughout the verb used is "defined", not "declared". If any >> struct vcpu instance actually lived in .rodata (for example), >> then (without casting away constness) it would be impossible to >> construct a non-const pointer to it. Hence there would be no >> legitimate means to create a way to modify that instance. But >> that's specifically not the case here (or in the example given). > > A function which takes a const vcpu* does not know, and has no way of > proving, that the object really wasn't const. Exactly. Which is why the compiler can't derive anything for its own code generation purposes. > I will admit that I made a made a mistake with the optimisation claim. > The outer function, because it can't see the declaration of the object > itself, also can't assume there aren't other aliases. And this is true throughout the code, not just for the outer function. > But none of this stops the casting away of const being UB, and it still > remains completely dishonest programming to declare > vmx_set_guest_bndcfgs() as taking a const vcpu, and then modifying it. Well, I was aware that the construct might not be liked, and hence I had specifically called for opinions. Nevertheless it's not at all "dishonest programming" to me. Quite the inverse - I think the VMCS enter/exit logic should have been written this way from the beginning. After all it's not the first time that because of its internal workings something else can get const added. And btw, C++ specifically has the "mutable" keyword to override const in certain (special) cases. I can accept though that you and maybe others don't agree with my view here, and hence (as expressed before) I'm not insisting on it staying the way it is right now. Jan
On Tue, Apr 09, 2019 at 06:53:47PM +0100, Andrew Cooper wrote: > @@ -692,6 +692,7 @@ unsigned long hvm_get_shadow_gs_base(struct vcpu *v); > void hvm_set_info_guest(struct vcpu *v); > void hvm_cpuid_policy_changed(struct vcpu *v); > void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset, uint64_t at_tsc); > +bool hvm_get_guest_bndcfgs(struct vcpu *v, uint64_t *val); Now that osstest is largely back to normal, can we commit this part so that the build is fixed and get some tests run on the x86 side? Wei.
On 15/04/2019 11:03, Wei Liu wrote: > On Tue, Apr 09, 2019 at 06:53:47PM +0100, Andrew Cooper wrote: >> @@ -692,6 +692,7 @@ unsigned long hvm_get_shadow_gs_base(struct vcpu *v); >> void hvm_set_info_guest(struct vcpu *v); >> void hvm_cpuid_policy_changed(struct vcpu *v); >> void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset, uint64_t at_tsc); >> +bool hvm_get_guest_bndcfgs(struct vcpu *v, uint64_t *val); > > Now that osstest is largely back to normal, can we commit this part so > that the build is fixed and get some tests run on the x86 side? Yes, please! This is a needed fix for getting a push. Juergen
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index c46e05b..283eb7b 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1150,11 +1150,8 @@ static bool vmx_set_guest_bndcfgs(struct vcpu *v, u64 val) return true; } -static bool vmx_get_guest_bndcfgs(const struct vcpu *cv, u64 *val) +static bool vmx_get_guest_bndcfgs(struct vcpu *v, u64 *val) { - /* Get a non-const pointer for vmx_vmcs_enter() */ - struct vcpu *v = cv->domain->vcpu[cv->vcpu_id]; - ASSERT(cpu_has_mpx && cpu_has_vmx_mpx); vmx_vmcs_enter(v); diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c index 815d599..0049a73 100644 --- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -115,7 +115,7 @@ int init_vcpu_msr_policy(struct vcpu *v) return 0; } -int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val) +int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) { const struct vcpu *curr = current; const struct domain *d = v->domain; @@ -182,13 +182,9 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val) break; case MSR_IA32_BNDCFGS: - if ( !cp->feat.mpx ) + if ( !cp->feat.mpx || !is_hvm_domain(d) || + !hvm_get_guest_bndcfgs(v, val) ) goto gp_fault; - - ASSERT(is_hvm_domain(d)); - if (!hvm_get_guest_bndcfgs(v, val) ) - goto gp_fault; - break; case MSR_IA32_XSS: @@ -375,13 +371,9 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val) break; case MSR_IA32_BNDCFGS: - if ( !cp->feat.mpx ) + if ( !cp->feat.mpx || !is_hvm_domain(d) || + !hvm_set_guest_bndcfgs(v, val) ) goto gp_fault; - - ASSERT(is_hvm_domain(d)); - if ( !hvm_set_guest_bndcfgs(v, val) ) - goto gp_fault; - break; case MSR_IA32_XSS: diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c index a55a400..af74f50 100644 --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -819,7 +819,7 @@ static inline bool is_cpufreq_controller(const struct domain *d) static int read_msr(unsigned int reg, uint64_t *val, struct x86_emulate_ctxt *ctxt) { - const struct vcpu *curr = current; + struct vcpu *curr = current; const struct domain *currd = curr->domain; bool vpmu_msr = false; int ret; diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index c811fa9..157f0de 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -145,7 +145,7 @@ struct hvm_function_table { int (*get_guest_pat)(struct vcpu *v, u64 *); int (*set_guest_pat)(struct vcpu *v, u64); - bool (*get_guest_bndcfgs)(const struct vcpu *v, u64 *); + bool (*get_guest_bndcfgs)(struct vcpu *v, u64 *); bool (*set_guest_bndcfgs)(struct vcpu *v, u64); void (*set_tsc_offset)(struct vcpu *v, u64 offset, u64 at_tsc); @@ -444,7 +444,7 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v) return hvm_funcs.get_shadow_gs_base(v); } -static inline bool hvm_get_guest_bndcfgs(const struct vcpu *v, u64 *val) +static inline bool hvm_get_guest_bndcfgs(struct vcpu *v, u64 *val) { return hvm_funcs.get_guest_bndcfgs && hvm_funcs.get_guest_bndcfgs(v, val); @@ -692,6 +692,7 @@ unsigned long hvm_get_shadow_gs_base(struct vcpu *v); void hvm_set_info_guest(struct vcpu *v); void hvm_cpuid_policy_changed(struct vcpu *v); void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset, uint64_t at_tsc); +bool hvm_get_guest_bndcfgs(struct vcpu *v, uint64_t *val); /* End of prototype list */ diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h index 0d52c08..3cbbc65 100644 --- a/xen/include/asm-x86/msr.h +++ b/xen/include/asm-x86/msr.h @@ -296,6 +296,11 @@ struct vcpu_msrs }; } misc_features_enables; + /* 0x00000da0 - MSR_IA32_XSS */ + struct { + uint64_t raw; + } xss; + /* * 0xc0000103 - MSR_TSC_AUX * @@ -313,11 +318,6 @@ struct vcpu_msrs * values here may be stale in current context. */ uint32_t dr_mask[4]; - - /* 0x00000da0 - MSR_IA32_XSS */ - struct { - uint64_t raw; - } xss; }; void init_guest_msr_policy(void); @@ -333,7 +333,7 @@ int init_vcpu_msr_policy(struct vcpu *v); * These functions are also used by the migration logic, so need to cope with * being used outside of v's context. */ -int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val); +int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val); int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val); #endif /* !__ASSEMBLY__ */
The series 832c1803^..f61685a6 was committed without adequate review. * Fix the shim build by providing a !CONFIG_HVM declaration for hvm_get_guest_bndcfgs() * Revert the bogus de-const'ing of the vcpu pointer in vmx_get_guest_bndcfgs(). vmx_vmcs_enter() really does mutate the vcpu, and may cause it to undergo a full de/reschedule, which is in violation of the ABI described by hvm_get_guest_bndcfgs(). guest_rdmsr() was always going to need to lose its const parameter, and this was the correct time for it to happen. * Remove the introduced ASSERT(is_hvm_domain(d)) and check the predicate directly. While we expect it to be true, the result is potential type confusion in release builds based on several subtle aspects of the CPUID feature derivation logic with no other safety checks. This also fixes the a linker error in the release build of the shim, again for !CONFIG_HVM reasons. * The MSRs in vcpu_msrs are in numeric order. Re-position XSS to match. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Paul Durrant <paul.durrant@citrix.com> CC: Jun Nakajima <jun.nakajima@intel.com> CC: Kevin Tian <kevin.tian@intel.com> There is a further bug which I haven't got a clean solution for yet. hvm_set_guest_bndcfgs() isn't always safe to use out of current context, and will fail the migration rather than making a remote adjustment to %xcr0. I'm leaning towards dropping this "just in case" logic, but ideally with clarification of the silicon behaviour. --- xen/arch/x86/hvm/vmx/vmx.c | 5 +---- xen/arch/x86/msr.c | 18 +++++------------- xen/arch/x86/pv/emul-priv-op.c | 2 +- xen/include/asm-x86/hvm/hvm.h | 5 +++-- xen/include/asm-x86/msr.h | 12 ++++++------ 5 files changed, 16 insertions(+), 26 deletions(-)