Re: Ping²: [PATCH] x86/PV: conditionally avoid raising #GP for early guest MSR accesses

[Jürgen Groß <jgross@xxxxxxxx>]

On 05.02.21 11:14, Jan Beulich wrote:
> (simply re-sending what was sent over 2 months ago)
>
> On 04.11.2020 11:50, Jan Beulich wrote:
> > On 03.11.2020 18:31, Andrew Cooper wrote:
> > > On 03/11/2020 17:06, Jan Beulich wrote:
> > > > Prior to 4.15 Linux, when running in PV mode, did not install a #GP
> > > > handler early enough to cover for example the rdmsrl_safe() of
> > > > MSR_K8_TSEG_ADDR in bsp_init_amd() (not to speak of the unguarded read
> > > > of MSR_K7_HWCR later in the same function). The respective change
> > > > (42b3a4cb5609 "x86/xen: Support early interrupts in xen pv guests") was
> > > > backported to 4.14, but no further - presumably since it wasn't really
> > > > easy because of other dependencies.
> > > >
> > > > Therefore, to prevent our change in the handling of guest MSR accesses
> > > > to render PV Linux 4.13 and older unusable on at least AMD systems, make
> > > > the raising of #GP on these paths conditional upon the guest having
> > > > installed a handler. Producing zero for reads and discarding writes
> > > > isn't necessarily correct and may trip code trying to detect presence of
> > > > MSRs early, but since such detection logic won't work without a #GP
> > > > handler anyway, this ought to be a fair workaround.
> > > >
> > > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > I appreciate that we probably have to do something, but I don't think
> > > this is a wise move.
> > I wouldn't call it wise either, but I'm afraid something along
> > these lines is necessary.
> >
> > > Linux is fundamentally buggy.  It is deliberately looking for a
> > > potential #GP fault given its use of rdmsrl_safe().  The reason this bug
> > > stayed hidden for so long was as a consequence of Xen's inappropriate
> > > MSR handling for guests, and the reasons for changing Xen's behaviour
> > > still stand.
> > I agree.
> >
> > > This change, in particular, does not apply to any explicitly handled
> > > MSRs, and therefore is not a comprehensive fix.
> > But it's intentional that this deals with the situation in a
> > generic way, not on a per-MSR basis. If we did as you suggest
> > further down, we'd have to audit all Linux versions up to 4.14
> > for similar issues with other MSRs. I don't think this would
> > be a practical thing to do, and I also don't think that leaving
> > things as they are until we have concrete reports of problems
> > is a viable option either.
> >
> > Adding explicit handling for the two offending MSRs (and any
> > possible further ones we discover) would imo only be to avoid
> > issuing the respective log messages.
> >
> > >    Nor is it robust to
> > > someone adding code to explicitly handling the impacted MSRs at a later
> > > date (which are are likely to need to do for HWCR), and which would
> > > reintroduce this failure to boot.
> > I'm afraid I don't understand. Looking at the two functions
> > the patch alters, only X86EMUL_OKAY is used in return statements
> > other than the final one. If this model is to be followed by
> > future additions (which I think it ought to be; perhaps we
> > should add comments to this effect), the code introduced here
> > will take care of the situation nevertheless.
> >
> > > We should have the impacted MSRs handled explicitly, with a note stating
> > > this was a bug in Linux 4.14 and older.  We already have workaround for
> > > similar bugs in Windows, and it also gives us a timeline to eventually
> > > removing support for obsolete workarounds, rather than having a "now and
> > > in the future, we'll explicitly tolerate broken PV behaviour for one bug
> > > back in ancient linux".
> > Comparing with Windows isn't very helpful; the patch here is
> > specifically about PV, and would help other OSes as well in
> > case they would have missed setting up exceptions early in
> > just the PV-on-Xen case. For the HVM case I'd indeed rather
> > see us go the route we've gone for Windows, if need be.
> As can be seen from this reply, we're not in agreement what to
> do here. But we need to do something. I'm not sure how to get
> unstuck discussions like this one ...
>
> Besides this suggestion of yours I also continue to have
> trouble seeing what good it will do to record an exception to
> inject into a guest when we know it didn't install a handler
> yet.
As we need to consider backports of processor bug mitigations
in old guests, too, I think we need to have a "catch-all"
fallback.

Not being able to run an old updated guest until we add handling
for a new MSR isn't a viable option IMO.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature