[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] x86/HVM: avoid pointer wraparound in bufioreq handling
> -----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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |