Re: [Xen-devel] [PATCH] x86/HVM: avoid pointer wraparound in bufioreq handling

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 16 June 2015 10:30
> To: Paul Durrant
> Cc: Andrew Cooper; Ian Campbell; Ian Jackson; Stefano Stabellini; Wei Liu;
> xen-devel; Keir (Xen.org)
> Subject: RE: [Xen-devel] [PATCH] x86/HVM: avoid pointer wraparound in
> bufioreq handling
> 
> >>> On 16.06.15 at 11:15, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Ian Campbell [mailto:ian.campbell@xxxxxxxxxx]
> >> Sent: 16 June 2015 10:00
> >> To: Jan Beulich
> >> Cc: Andrew Cooper; Wei Liu; Ian Jackson; Stefano Stabellini; xen-devel;
> Keir
> >> (Xen.org); Paul Durrant
> >> Subject: Re: [Xen-devel] [PATCH] x86/HVM: avoid pointer wraparound in
> >> bufioreq handling
> >>
> >> On Tue, 2015-06-16 at 09:37 +0100, Jan Beulich wrote:
> >> > >>> On 16.06.15 at 10:20, <ian.campbell@xxxxxxxxxx> wrote:
> >> > > On Tue, 2015-06-16 at 07:44 +0100, Jan Beulich wrote:
> >> > >> >>> On 15.06.15 at 16:30, <JBeulich@xxxxxxxx> wrote:
> >> > >> > The number of slots per page being 511 (i.e. not a power of two)
> >> means
> >> > >> > that the (32-bit) read and write indexes going beyond 2^32 will
> likely
> >> > >> > disturb operation. Extend I/O req server creation so the caller can
> >> > >> > indicate that it is using suitable atomic accesses where needed (not
> >> > >> > all accesses to the two pointers really need to be atomic), allowing
> >> > >> > the hypervisor to atomically canonicalize both pointers when both
> >> have
> >> > >> > gone through at least one cycle.
> >> > >> >
> >> > >> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> > >>
> >> > >> No matter that it's just a single line change, I realized that I
> >> > >> forgot to Cc the tools maintainers. While a v2 will be needed (see
> >> > >> the reply just sent to Andrew) I'd still appreciate input (if any) to
> >> > >> limit the number of revisions needed.
> >> > >
> >> > > For such a simple toolstack side change which just reflects the
> >> > > underlying hcall interface I have no real opinion so far as the tools
> >> > > side goes, but it would be good to update the comments in xenctrl.h
> too.
> >> > > With that done for the tools change:
> >> > >         Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >> >
> >> > Thanks. The request for feedback went beyond the request for
> >> > an ack though, namely
> >> >
> >> > TBD: Do we need to be worried about non-libxc users of the changed
> >> >      (tools only) interface?
> >>
> >> It's (currently at least) a declared non-stable API, so in principal no.
> >> It would be polite to give a heads up to the expected potential users
> >> though, which you've done by CCing the QEMU maintainers I think.
> Adding
> >> Paul D for completeness though.
> >
> > From my reading, both QEMU upstream and trad are safe. They use a loop
> of
> > the form:
> >
> > while (read_ptr != write_ptr)
> > {
> >    do stuff
> >
> >   read_ptr += (handled a qword) ? 2 : 1;
> > }
> >
> > So, since the only test is for equality I think overflow should be handled
> > correctly. So, does anything actually need to be fixed?
> 
> Of course this needs to be fixed: When either pointer crosses the
> 2^32 boundary, the slot referenced goes from 0x1f to 0 (due to the
> "modulo 511" operation determining the slot to be used), introducing
> a discontinuity and potentially corrupting data in slots not consumed
> yet.
> 

Ah yes. I thought you were worried about inequality checks going wrong.

The way that QEMU processes buffered requests means that a synchronous ioreq is 
a barrier to buffered ring processing. So I guess it should be possible to send 
a synchronous request and then zero the buffered ring counters before they 
reach overflow.

  Paul
 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel