* [bug report] habanalabs: convert ts to use unified memory manager
@ 2022-05-24 13:56 Dan Carpenter
2022-05-24 15:07 ` Yuri Nudelman
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2022-05-24 13:56 UTC (permalink / raw)
To: ynudelman, Oded Gabbay; +Cc: kernel-janitors
Hello Yuri Nudelman,
The patch 4d530e7d121a: "habanalabs: convert ts to use unified memory
manager" from Mar 20, 2022, leads to the following Smatch static
checker warning:
drivers/misc/habanalabs/common/memory.c:2117 hl_ts_alloc_buf()
warn: use 'gfp' here instead of GFP_XXX?
drivers/misc/habanalabs/common/memory.c
2107 static int hl_ts_alloc_buf(struct hl_mmap_mem_buf *buf, gfp_t gfp, void *args)
2108 {
2109 struct hl_ts_buff *ts_buff = NULL;
2110 u32 size, num_elements;
2111 void *p;
2112
2113 num_elements = *(u32 *)args;
2114
2115 ts_buff = kzalloc(sizeof(*ts_buff), GFP_KERNEL);
^^^^^^^^^^
The "gfp" is sometimes GFP_ATOMIC so that suggests that this is a bug.
However, I can't immediately see why it nees to be atomic. As far as
I can see the callers are holding mutexes not spinlocks. But I have
not looked hard.
2116 if (!ts_buff)
2117 return -ENOMEM;
2118
2119 /* Allocate the user buffer */
2120 size = num_elements * sizeof(u64);
2121 p = vmalloc_user(size);
It's concerning that there are no integer overflow checks. I feel like
someone should make a vmalloc_array_user() or vmalloc_user_array()
function with built in integer overflow checking.
Of course vmalloc() sleeps so GFP_ATOMIC doesn't really help. There
should be a Smatch warning for GFP_ATOMIC followed by a sleeping
function with no preempt enable in between... #Idea #EasyToImplement
The way to do it would be to look for GFP_ATOMIC. Hook into the
preempt_enable() hook in check_preempt_info.c. Add a hook into the
check_sleep_info.c.
1) If we see an ATOMIC, set the state to &atomic.
2) If we see a preempt_enable() then set then set the state to &undefined
3) If we see a sleep() then check for if the state can be &atomic and
if so then print a warning.
2122 if (!p)
2123 goto free_mem;
2124
2125 ts_buff->user_buff_address = p;
2126 buf->mappable_size = size;
2127
2128 /* Allocate the internal kernel buffer */
2129 size = num_elements * sizeof(struct hl_user_pending_interrupt);
2130 p = vmalloc(size);
Use vmalloc_array() to prevent integer overflows.
2131 if (!p)
2132 goto free_user_buff;
2133
2134 ts_buff->kernel_buff_address = p;
2135 ts_buff->kernel_buff_size = size;
2136
2137 buf->private = ts_buff;
2138
2139 return 0;
2140
2141 free_user_buff:
2142 vfree(ts_buff->user_buff_address);
2143 free_mem:
2144 kfree(ts_buff);
2145 return -ENOMEM;
2146 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] habanalabs: convert ts to use unified memory manager
2022-05-24 13:56 [bug report] habanalabs: convert ts to use unified memory manager Dan Carpenter
@ 2022-05-24 15:07 ` Yuri Nudelman
2022-05-25 6:55 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Yuri Nudelman @ 2022-05-24 15:07 UTC (permalink / raw)
To: Dan Carpenter, Oded Gabbay; +Cc: kernel-janitors
On 5/24/2022 4:56 PM, Dan Carpenter wrote:
> [You don't often get email from dan.carpenter@oracle.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification.]
>
> Hello Yuri Nudelman,
>
> The patch 4d530e7d121a: "habanalabs: convert ts to use unified memory
> manager" from Mar 20, 2022, leads to the following Smatch static
> checker warning:
>
> drivers/misc/habanalabs/common/memory.c:2117 hl_ts_alloc_buf()
> warn: use 'gfp' here instead of GFP_XXX?
>
> drivers/misc/habanalabs/common/memory.c
> 2107 static int hl_ts_alloc_buf(struct hl_mmap_mem_buf *buf, gfp_t gfp, void *args)
> 2108 {
> 2109 struct hl_ts_buff *ts_buff = NULL;
> 2110 u32 size, num_elements;
> 2111 void *p;
> 2112
> 2113 num_elements = *(u32 *)args;
> 2114
> 2115 ts_buff = kzalloc(sizeof(*ts_buff), GFP_KERNEL);
> ^^^^^^^^^^
> The "gfp" is sometimes GFP_ATOMIC so that suggests that this is a bug.
> However, I can't immediately see why it nees to be atomic. As far as
> I can see the callers are holding mutexes not spinlocks. But I have
> not looked hard.
>
> 2116 if (!ts_buff)
> 2117 return -ENOMEM;
> 2118
> 2119 /* Allocate the user buffer */
> 2120 size = num_elements * sizeof(u64);
> 2121 p = vmalloc_user(size);
>
> It's concerning that there are no integer overflow checks. I feel like
> someone should make a vmalloc_array_user() or vmalloc_user_array()
> function with built in integer overflow checking.
>
> Of course vmalloc() sleeps so GFP_ATOMIC doesn't really help. There
> should be a Smatch warning for GFP_ATOMIC followed by a sleeping
> function with no preempt enable in between... #Idea #EasyToImplement
>
> The way to do it would be to look for GFP_ATOMIC. Hook into the
> preempt_enable() hook in check_preempt_info.c. Add a hook into the
> check_sleep_info.c.
> 1) If we see an ATOMIC, set the state to &atomic.
> 2) If we see a preempt_enable() then set then set the state to &undefined
> 3) If we see a sleep() then check for if the state can be &atomic and
> if so then print a warning.
>
> 2122 if (!p)
> 2123 goto free_mem;
> 2124
> 2125 ts_buff->user_buff_address = p;
> 2126 buf->mappable_size = size;
> 2127
> 2128 /* Allocate the internal kernel buffer */
> 2129 size = num_elements * sizeof(struct hl_user_pending_interrupt);
> 2130 p = vmalloc(size);
>
> Use vmalloc_array() to prevent integer overflows.
>
> 2131 if (!p)
> 2132 goto free_user_buff;
> 2133
> 2134 ts_buff->kernel_buff_address = p;
> 2135 ts_buff->kernel_buff_size = size;
> 2136
> 2137 buf->private = ts_buff;
> 2138
> 2139 return 0;
> 2140
> 2141 free_user_buff:
> 2142 vfree(ts_buff->user_buff_address);
> 2143 free_mem:
> 2144 kfree(ts_buff);
> 2145 return -ENOMEM;
> 2146 }
>
> regards,
> dan carpenter
Hi,
In this flow gfp cannot be atomic, so there is no severe problem with
that. The fact I left explicit GFP_KERNEL flag in this code is a typo.
This function serves as a callback, and is invoked via a function pointer
through the hl_mmap_mem_buf_alloc, which can receive a GFP_ATOMIC
argument, but not in the flow when the callback is hl_ts_alloc_buf.
I will change it from GFP_KERNEL to gfp to avoid the warning, plus use
vmalloc_array as you suggested.
Thanks,
Yuri Nudelman
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] habanalabs: convert ts to use unified memory manager
2022-05-24 15:07 ` Yuri Nudelman
@ 2022-05-25 6:55 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2022-05-25 6:55 UTC (permalink / raw)
To: Yuri Nudelman; +Cc: Oded Gabbay, kernel-janitors
On Tue, May 24, 2022 at 03:07:18PM +0000, Yuri Nudelman wrote:
>
> Hi,
>
> In this flow gfp cannot be atomic, so there is no severe problem with
> that. The fact I left explicit GFP_KERNEL flag in this code is a typo.
>
> This function serves as a callback, and is invoked via a function pointer
> through the hl_mmap_mem_buf_alloc, which can receive a GFP_ATOMIC
> argument, but not in the flow when the callback is hl_ts_alloc_buf.
That's okay then.
>
> I will change it from GFP_KERNEL to gfp to avoid the warning,
No don't do that. Just ignore false positives. Plus eventually we will
implement the other check that I mentioned.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-25 6:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-24 13:56 [bug report] habanalabs: convert ts to use unified memory manager Dan Carpenter
2022-05-24 15:07 ` Yuri Nudelman
2022-05-25 6:55 ` 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.