On Mon, Jan 09, 2023 at 03:07:33PM +0000, Farah Kassabri wrote:
> > 2171 {
> > 2172 struct hl_ts_buff *ts_buff = NULL;
> > 2173 u32 size, num_elements;
> > 2174 void *p;
> > 2175
> > 2176 num_elements = *(u32 *)args;
> >
> > This business of passing void pointers and pretending that
> > hl_cb_mmap_mem_alloc() and hl_ts_alloc_buf() are the same function is a
> > nightmare.
> >
> > Create two ->alloc functions. Split hl_mmap_mem_buf_alloc() into one
> > function that allocates idr stuff. Create a function to free/remove the idr
> > stuff. Create two new helper function that call the idr function and then the
> > appropriate alloc() function.
> >
> > It will be much cleaner than using a void pointer.
>
> I'm not sure I understood your intention.
> What void pointers are you referring to ? the args in this line rc = buf->behavior->alloc(buf, gfp, args); ?
> If yes what's so bad about it, it much simpler to have one common function and call specific implementation through pointers.
> BTW same goes to the map function also, not just the alloc (each behavior has alloc and map method)
>
Yeah, you're right. I didn't look at this carefully. I'm sorry.
> >
> > 2177
> > --> 2178 ts_buff = kzalloc(sizeof(*ts_buff), GFP_KERNEL);
> > ^^^^^^^^^^ Smatch is correct that it should be
> > used here.
>
> Sure will be fixed.
>
> >
> > 2179 if (!ts_buff)
> > 2180 return -ENOMEM;
> > 2181
> > 2182 /* Allocate the user buffer */
> > 2183 size = num_elements * sizeof(u64);
> >
> > Can this have an integer overflow on 32bit systems?
>
> I'll define "size" as size_t instead of u32.
>
This can't actually overflow because it's checked in the caller.
Perhaps the careful way to write this is to change size to size_t as you
suggest which fixes the issue for 64bit systems and use size_mul() so it
doesn't overflow on 32bit systems either.
size = size_mul(num_elements, sizeof(u64));
But it doesn't really matter either way because num_elements is capped
in the caller.
regards,
dan carpenter
