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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.06.2020 20:58, Razvan Cojocaru wrote:
> 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.

Even more so thanks for replying; I did Cc you merely because of
history and because I thought with Alexandru and Petre not
replying you might either know someone else who should, or nudge
them.

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

That's my understanding (and nothing to the contrary looks to
have surfaced from the other communication between you and Tamas),
so with that I guess I'll go and throw in the patch.

Jan