Re: [bug report] habanalabs: Timestamps buffers registration

[Dan Carpenter <error27@gmail.com>]

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