[bug report] habanalabs: convert ts to use unified memory manager

[bug report] habanalabs: convert ts to use unified memory manager

[Dan Carpenter]

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

Re: [bug report] habanalabs: convert ts to use unified memory manager

[Yuri Nudelman]

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

Re: [bug report] habanalabs: convert ts to use unified memory manager

[Dan Carpenter]

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