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? > 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 ? 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.. > +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.. Jason -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html