[bug report] habanalabs: Timestamps buffers registration

[Farah Kassabri]

On Mon, Jan 09, 2023 at 2:00:48PM +0000, Dan Carpenter wrote:

> 
> Hello farah kassabri,
> 
> The patch 9158bf69e74f: "habanalabs: Timestamps buffers registration"
> from Dec 23, 2021, leads to the following Smatch static checker
> warning:
> 
>         drivers/accel/habanalabs/common/memory.c:2178 hl_ts_alloc_buf()
>         warn: use 'gfp' here instead of GFP_XXX?
> 
> drivers/accel/habanalabs/common/memory.c
>     2170 static int hl_ts_alloc_buf(struct hl_mmap_mem_buf *buf, gfp_t gfp,
> void *args)
>                                                                        ^^^ "gfp" is never used.

Correct, I'll fix it.

> 
>     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)

> 
>     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.

> 
>     2184         p = vmalloc_user(size);
>     2185         if (!p)
>     2186                 goto free_mem;
>     2187
>     2188         ts_buff->user_buff_address = p;
>     2189         buf->mappable_size = size;
>     2190
>     2191         /* Allocate the internal kernel buffer */
>     2192         size = num_elements * sizeof(struct hl_user_pending_interrupt);
>     2193         p = vzalloc(size);
>     2194         if (!p)
>     2195                 goto free_user_buff;
>     2196
>     2197         ts_buff->kernel_buff_address = p;
>     2198         ts_buff->kernel_buff_size = size;
>     2199
>     2200         buf->private = ts_buff;
>     2201
>     2202         return 0;
>     2203
>     2204 free_user_buff:
>     2205         vfree(ts_buff->user_buff_address);
>     2206 free_mem:
>     2207         kfree(ts_buff);
>     2208         return -ENOMEM;
>     2209 }
> 
> regards,
> dan carpenter