> -----Original Message----- > From: Julien Grall <[email protected]> > Sent: 30 September 2020 18:48 > To: Oleksandr <[email protected]>; [email protected] > Cc: [email protected]; 'Oleksandr Tyshchenko' <[email protected]>; > 'Andrew Cooper' > <[email protected]>; 'George Dunlap' <[email protected]>; 'Ian > Jackson' > <[email protected]>; 'Jan Beulich' <[email protected]>; 'Stefano > Stabellini' > <[email protected]>; 'Wei Liu' <[email protected]>; 'Roger Pau Monné' > <[email protected]>; 'Jun > Nakajima' <[email protected]>; 'Kevin Tian' <[email protected]>; 'Tim > Deegan' <[email protected]>; > 'Julien Grall' <[email protected]> > Subject: Re: [PATCH V1 02/16] xen/ioreq: Make x86's IOREQ feature common > > Hi, > > On 30/09/2020 14:39, Oleksandr wrote: > > > > Hi Julien > > > > On 25.09.20 11:19, Paul Durrant wrote: > >>> -----Original Message----- > >>> From: Julien Grall <[email protected]> > >>> Sent: 24 September 2020 19:01 > >>> To: Oleksandr Tyshchenko <[email protected]>; > >>> [email protected] > >>> Cc: Oleksandr Tyshchenko <[email protected]>; Andrew > >>> Cooper <[email protected]>; > >>> George Dunlap <[email protected]>; Ian Jackson > >>> <[email protected]>; Jan Beulich > >>> <[email protected]>; Stefano Stabellini <[email protected]>; Wei > >>> Liu <[email protected]>; Roger Pau > >>> Monné <[email protected]>; Paul Durrant <[email protected]>; Jun > >>> Nakajima <[email protected]>; > >>> Kevin Tian <[email protected]>; Tim Deegan <[email protected]>; Julien > >>> Grall <[email protected]> > >>> Subject: Re: [PATCH V1 02/16] xen/ioreq: Make x86's IOREQ feature common > >>> > >>> > >>> > >>> On 10/09/2020 21:21, Oleksandr Tyshchenko wrote: > >>>> +static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p) > >>>> +{ > >>>> + unsigned int prev_state = STATE_IOREQ_NONE; > >>>> + unsigned int state = p->state; > >>>> + uint64_t data = ~0; > >>>> + > >>>> + smp_rmb(); > >>>> + > >>>> + /* > >>>> + * The only reason we should see this condition be false is > >>>> when an > >>>> + * emulator dying races with I/O being requested. > >>>> + */ > >>>> + while ( likely(state != STATE_IOREQ_NONE) ) > >>>> + { > >>>> + if ( unlikely(state < prev_state) ) > >>>> + { > >>>> + gdprintk(XENLOG_ERR, "Weird HVM ioreq state transition > >>>> %u -> %u\n", > >>>> + prev_state, state); > >>>> + sv->pending = false; > >>>> + domain_crash(sv->vcpu->domain); > >>>> + return false; /* bail */ > >>>> + } > >>>> + > >>>> + switch ( prev_state = state ) > >>>> + { > >>>> + case STATE_IORESP_READY: /* IORESP_READY -> NONE */ > >>>> + p->state = STATE_IOREQ_NONE; > >>>> + data = p->data; > >>>> + break; > >>>> + > >>>> + case STATE_IOREQ_READY: /* IOREQ_{READY,INPROCESS} -> > >>>> IORESP_READY */ > >>>> + case STATE_IOREQ_INPROCESS: > >>>> + wait_on_xen_event_channel(sv->ioreq_evtchn, > >>>> + ({ state = p->state; > >>>> + smp_rmb(); > >>>> + state != prev_state; })); > >>>> + continue; > >>> As I pointed out previously [1], this helper was implemented with the > >>> expectation that wait_on_xen_event_channel() will not return if the vCPU > >>> got rescheduled. > >>> > >>> However, this assumption doesn't hold on Arm. > >>> > >>> I can see two solution: > >>> 1) Re-execute the caller > >>> 2) Prevent an IOREQ to disappear until the loop finish. > >>> > >>> @Paul any opinions? > >> The ioreq control plane is largely predicated on there being no > >> pending I/O when the state of a server is modified, and it is assumed > >> that domain_pause() is sufficient to achieve this. If that assumption > >> doesn't hold then we need additional synchronization. > > I don't think this assumption even hold on x86 because domain_pause() > will not wait for I/O to finish. > > On x86, the context switch will reset the stack and therefore > wait_on_xen_event_channel() is not going to return. Instead, > handle_hvm_io_completion() will be called from the tail callback in > context_switch(). get_pending_vcpu() would return NULL as the IOREQ > server disappeared. Although, it is not clear whether the vCPU will > continue to run (or not). > > Did I miss anything? > > Regarding the fix itself, I am not sure what sort of synchronization we > can do. Are you suggesting to wait for the I/O to complete? If so, how > do we handle the case the IOREQ server died? >
s/IOREQ server/emulator but that is a good point. If domain_pause() did wait for I/O to complete then this would have always been a problem so, with hindsight, it should have been obvious this was not the case. Digging back, it looks like things would have probably been ok before 125833f5f1f0 "x86: fix ioreq-server event channel vulnerability" because, wait_on_xen_event_channel() and the loop condition above it did not dereference anything that would disappear with IOREQ server destruction (they used the shared page, which at this point was always a magic page and hence part of the target domain's memory). So things have probably been broken since 2014. To fix the problem I think it is sufficient that we go back to a wait loop that can tolerate the IOREQ server disappearing between iterations and deals with that as a completed emulation (albeit returning f's for reads and sinking writes). Paul > > May I please clarify whether a concern still stands (with what was said > > above) and we need an additional synchronization on Arm? > > Yes the concern is still there (See above). > > Cheers, > > -- > Julien Grall
