Re: [patch v3] infiniband: uverbs: handle large number of entries

[Dan Carpenter <error27@gmail.com>]

On Tue, Oct 12, 2010 at 03:01:18PM -0600, Jason Gunthorpe wrote:
> On Tue, Oct 12, 2010 at 01:31:17PM +0200, Dan Carpenter wrote:
> > In the original code there was a potential integer overflow if you
> > passed in a large cmd.ne.  The calls to kmalloc() would allocate smaller
> > buffers than intended, leading to memory corruption.
> 
> Keep in mind these are probably performance sensitive APIs, I was
> imagining batching a small number and they copy_to_user ? No idea what
> the various performance trades offs are..
> 
> > Please, please, check this.  I've think I've done it right, but I don't
> > have the hardware and can not test it.
> 
> Nor, do I.. I actually don't know what hardware uses this path? The
> Mellanox cards use a user-space only version.
>  
> Maybe an iwarp card? I kinda recall some recent messages concerning
> memory allocations in these paths for iwarp. I wonder if removing the
> allocation is such a big win the larger number of copy_to_user calls
> does not matter?
> 

Who knows?

The reason I'm writing this is to fix a potential security issue, but I
think that viewed from a holistic perspective this patch is also a
performance improvement over the original code because it avoids the big
kmalloc()s.  Doing the copy_to_user() in batches of PAGE_SIZE might be
better but it's more complicated and I'm very lazy... :/  If someone
steps up to do the benchmarks then I might take a look at it.

> > It's strange to me that we return "in_len" on success.
> 
> Agree..
> 
> > +static int copy_header_to_user(void __user *dest, u32 count)
> > +{
> > +	u32 header[2];  /* the second u32 is reserved */
> > +
> > +	memset(header, 0, sizeof(header));
> 
> Don't you need header[0] = count ?
> 

Yes.  Thank you for catching that.

> Maybe:
>   u32 header[2] = {count};
> 
> And let the compiler 0 the other word optimally. Also, I'm not matters
> here, since you are zeroing user memory that isn't currently used..

It does matter, because we don't want to leak information to the user.

> 
> > +static int copy_wc_to_user(void __user *dest, struct ib_wc *wc)
> > +{
> > +	struct ib_uverbs_wc tmp;
> > +
> > +	memset(&tmp, 0, sizeof(tmp));
> 
> I'd really like to see that memset go away for performance. Again
> maybe use named initializers and let the compiler zero the
> uninitialized (does it zero padding, I wonder?). Or pre-zero this
> memory outside the loop..
> 

Good idea.  Yes, it does do zero padding.

regards,
dan carpenter