From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Deegan Subject: Re: [PATCH 1/5] xen: add ssize_t Date: Thu, 9 Aug 2012 11:59:06 +0100 Message-ID: <20120809105906.GE16986@ocelot.phlegethon.org> References: <1344023454-31425-1-git-send-email-jean.guyader@citrix.com> <1344023454-31425-2-git-send-email-jean.guyader@citrix.com> <501F97890200007800092CA9@nat28.tlf.novell.com> <20120809095136.GB16986@ocelot.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jean Guyader Cc: Jan Beulich , Jean Guyader , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org At 11:19 +0100 on 09 Aug (1344511142), Jean Guyader wrote: > On 9 August 2012 10:51, Tim Deegan wrote: > > At 15:47 +0100 on 06 Aug (1344268059), Jean Guyader wrote: > >> On 6 August 2012 09:08, Jan Beulich wrote: > >> >>>> On 03.08.12 at 21:50, Jean Guyader 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.