Re: [Xen-devel] [PATCH 1/5] xen: add ssize_t

[Tim Deegan <tim@xxxxxxx>]

At 11:19 +0100 on 09 Aug (1344511142), Jean Guyader wrote:
> On 9 August 2012 10:51, Tim Deegan <tim@xxxxxxx> wrote:
> > At 15:47 +0100 on 06 Aug (1344268059), Jean Guyader wrote:
> >> On 6 August 2012 09:08, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >> >>>> On 03.08.12 at 21:50, Jean Guyader <jean.guyader@xxxxxxxxxx> wrote:
> >> >
> >> > Without finally explaining why you need this type in the first place,
> >> > I'll continue to NAK this patch. (This is made even worse by the fact
> >> > taht the two inline functions in patch 5 that make use of the type
> >> > appear to be unused.)
> >> >
> >>
> >> Understood. I'll switch to use long instead of ssize_t in my
> >> forthcoming patch serie.
> >
> > Please use an explicitly 64-bit type - AFAICS you're holding the sum of
> > some 64-bit length fields.
> >
> 
> Ok but ssize_t is kind of a funny one. It should accept everything
> that size_t can accept + negative values.

Given that you start with user-supplied 64-bit iov lengths, there is no
such type. :)

And now that I look at it, your handling of sizes is pretty confused.
v4v_ringbuf_insertv() returns a ssize_t, but calculates it internally as
an int32_t, which is _smaller_ than the size_t it starts with (which is
the sum of some guest-supplied uint64_ts).  And then v4v_sendv() puts
that ssize_t into an int again before returning it as a size_t, which
d_v4v_op() casts as a long to give to the guest.  Whee!

Can you please make that lot consistent, and then audit the whole path
from user-supplied iovec lists to actual copies and make sure that there
are no overflows?

I think that explicitly limiting your sum-of-iovec-lengths to 31 bits
would be perfectly reasonable (1 hypercall per 2GB copy), and would
avoid a lot of pain here.  As long as it was documented in the
interface, of course!

Cheers,

Tim.

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