RE: [PATCH-for-4.14] ioreq: handle pending emulation racing with ioreq server destruction

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 08 June 2020 11:58
> To: Paul Durrant <paul@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Paul Durrant <pdurrant@xxxxxxxxxx>; Marek 
> Marczykowski-Górecki
> <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH-for-4.14] ioreq: handle pending emulation racing with 
> ioreq server destruction
> 
> On 08.06.2020 11:46, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@xxxxxxxxxx>
> >
> > When an emulation request is initiated in hvm_send_ioreq() the guest vcpu is
> > blocked on an event channel until that request is completed. If, however,
> > the emulator is killed whilst that emulation is pending then the ioreq
> > server may be destroyed. Thus when the vcpu is awoken the code in
> > handle_hvm_io_completion() will find no pending request to wait for, but 
> > will
> > leave the internal vcpu io_req.state set to IOREQ_READY and the vcpu 
> > shutdown
> > deferall flag in place (because hvm_io_assist() will never be called). The
> > emulation request is then completed anyway. This means that any subsequent 
> > call
> > to hvmemul_do_io() will find an unexpected value in io_req.state and will
> > return X86EMUL_UNHANDLEABLE, which in some cases will result in continuous
> > re-tries.
> >
> > This patch fixes the issue by moving the setting of io_req.state and 
> > clearing
> > of shutdown deferral (as will as MSI-X write completion) out of 
> > hvm_io_assist()
> > and directly into handle_hvm_io_completion().
> >
> > Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with a question:
> 
> > --- a/xen/arch/x86/hvm/ioreq.c
> > +++ b/xen/arch/x86/hvm/ioreq.c
> > @@ -109,15 +109,7 @@ static void hvm_io_assist(struct hvm_ioreq_vcpu *sv, 
> > uint64_t data)
> >      ioreq_t *ioreq = &v->arch.hvm.hvm_io.io_req;
> >
> >      if ( hvm_ioreq_needs_completion(ioreq) )
> > -    {
> > -        ioreq->state = STATE_IORESP_READY;
> >          ioreq->data = data;
> > -    }
> > -    else
> > -        ioreq->state = STATE_IOREQ_NONE;
> > -
> > -    msix_write_completion(v);
> > -    vcpu_end_shutdown_deferral(v);
> >
> >      sv->pending = false;
> >  }
> 
> With this, is the function worth keeping at all?

I did think about that, but it is called in more than one place. So, in the 
interest of trying to make back-porting straightforward, I thought it best to 
keep it simple.

> 
> Also I assume the patch has your implied R-a-b?
> 

Since it has your R-b now, yes :-)

  Paul