On 5/24/2022 4:56 PM, Dan Carpenter wrote: > [You don't often get email from dan.carpenter@xxxxxxxxxx. 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
