* [bug report] habanalabs: Timestamps buffers registration
@ 2023-01-06 8:32 Dan Carpenter
2023-01-09 15:07 ` Farah Kassabri
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2023-01-06 8:32 UTC (permalink / raw)
To: fkassabri; +Cc: dri-devel
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.
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.
2177
--> 2178 ts_buff = kzalloc(sizeof(*ts_buff), GFP_KERNEL);
^^^^^^^^^^
Smatch is correct that it should be used here.
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?
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
^ permalink raw reply [flat|nested] 3+ messages in thread* RE: [bug report] habanalabs: Timestamps buffers registration
2023-01-06 8:32 [bug report] habanalabs: Timestamps buffers registration Dan Carpenter
@ 2023-01-09 15:07 ` Farah Kassabri
2023-01-10 9:54 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Farah Kassabri @ 2023-01-09 15:07 UTC (permalink / raw)
To: Dan Carpenter; +Cc: dri-devel@lists.freedesktop.org
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] habanalabs: Timestamps buffers registration
2023-01-09 15:07 ` Farah Kassabri
@ 2023-01-10 9:54 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2023-01-10 9:54 UTC (permalink / raw)
To: Farah Kassabri; +Cc: dri-devel@lists.freedesktop.org
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-01-10 9:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-06 8:32 [bug report] habanalabs: Timestamps buffers registration Dan Carpenter
2023-01-09 15:07 ` Farah Kassabri
2023-01-10 9:54 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.