Re: [PATCH v4 for-4.14] x86/monitor: revert default behavior when monitoring register write events

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 6/8/20 6:55 PM, Jan Beulich wrote:
> On 03.06.2020 17:07, Roger Pau Monné wrote:
>> On Wed, Jun 03, 2020 at 06:52:37AM -0600, Tamas K Lengyel wrote:
>>> For the last couple years we have received numerous reports from users of
>>> monitor vm_events of spurious guest crashes when using events. In 
>>> particular,
>>> it has observed that the problem occurs when vm_events are being disabled. 
>>> The
>>> nature of the guest crash varied widely and has only occured occasionally. 
>>> This
>>> made debugging the issue particularly hard. We had discussions about this 
>>> issue
>>> even here on the xen-devel mailinglist with no luck figuring it out.
>>>
>>> The bug has now been identified as a race-condition between register event
>>> handling and disabling the monitor vm_event interface. The default behavior
>>> regarding emulation of register write events is changed so that they get
>>> postponed until the corresponding vm_event handler decides whether to allow 
>>> such
>>> write to take place. Unfortunately this can only be implemented by 
>>> performing the
>>> deny/allow step when the vCPU gets scheduled.
>>>
>>> Due to that postponed emulation of the event if the user decides to pause 
>>> the
>>> VM in the vm_event handler and then disable events, the entire emulation 
>>> step
>>> is skipped the next time the vCPU is resumed. Even if the user doesn't pause
>>> during the vm_event handling but exits immediately and disables vm_event, 
>>> the
>>> situation becomes racey as disabling vm_event may succeed before the guest's
>>> vCPUs get scheduled with the pending emulation task. This has been 
>>> particularly
>>> the case with VMS that have several vCPUs as after the VM is unpaused it may
>>> actually take a long time before all vCPUs get scheduled.
>>>
>>> In this patch we are reverting the default behavior to always perform 
>>> emulation
>>> of register write events when the event occurs. To postpone them can be 
>>> turned
>>> on as an option. In that case the user of the interface still has to take 
>>> care
>>> of only disabling the interface when its safe as it remains buggy.
>>>
>>> Fixes: 96760e2fba10 ('vm_event: deny register writes if refused by vm_event
>>> reply').
>>>
>>> Signed-off-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
>>
>> Thanks!
>>
>> Reviewed-by: Roger Pau Monné <rogerpau@xxxxxxxxxx>
>>
>> I would like to get some input from Bitdefender really, and whether
>> they are fine with this approach.

Hello,

Not really my call to make anymore, but I do have a few notes.

First, IIRC the problem stems from the initial choice to have the
vm_event data allocated on-demand when first subscribing to events. The
proper solution (since this patch doesn't actually fix the problem),
IMHO, would be for the vm_event data to _always_ exist, and instead of
relying on the value of its pointer to check if there are event
subscribers, we could just check the emulation flags individually and
never miss a pending emulated something again. I did try to go that way
in the beginning, but it has reasonably been objected that we should cut
back on using hypervisor memory unnecessarily, hence we got at this point.

Secondly, I see no reason why we couldn't adapt to the new default
behaviour provided that the old behaviour continues to work _exactly_ as
before.

And last but not least, the proper sequence is: 1. unsubscribe from
register write events, 2. process all events "still in the chamber"
(keep checking the ring buffer for a while), 3. detach from the guest
(disable the vm_event subsystem). Not ideal perhaps (in that it's not
guaranteed that a VCPU won't resume after a longer period than our
timeout), but if the sequence is followed there should be no guest hangs
or crashes (at least none that we or our clients have observed so far).

So in short, I think there's a better fix for this by simply not
allocating the vm_event memory on-demand anymore and never having to
deal with lost pending emulations again. It should also decrease code
complexity by a tiny bit. Then again, as stated at the beginning of this
message, that's just a recommendation.


HTH,
Razvan