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
