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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

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

  Paul

> 
> >      Do we also need a way for default servers to flag atomicity?
> 
> Not doing so leaves them open to the issue which you are fixing here, I
> think?
> 
> Andy indicated that qemu-trad was the only default-server these days, so
> perhaps we can just decree that it is so while fixing qemu-trad as
> necessary?
> 
> In fact, are ioreq servers new enough and with few enough users that can
> we decree that even they are always atomic, perhaps after having audited
> the current users to ensure they behave correctly?
> 
> > > For the hypercall interface level, I wonder if handle_bufioreq is still
> > > an appropriate name given its no longer treated as a boolean flag?
> > > bufioreq_type or something perhaps?
> >
> > I think the name is fine as is (it doesn't really imply boolean-ness
> > in my reading), but if others agree with you that it should be
> > renamed I also wouldn't object.
> 
> I read it as "should handle buf ioreqs", with should being a boolean
> type predicate thing, but that might just be me, I suppose it could also
> be read as "kind of buf ioreqs to handle".

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