Re: [bug report] drm/xe/uapi: Add UAPI for querying VMA count and memory attributes

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 28-08-2025 14:31, Dan Carpenter wrote:
> Hello Himal Prasad Ghimiray,
> 
> Commit 418807860e94 ("drm/xe/uapi: Add UAPI for querying VMA count
> and memory attributes") from Aug 21, 2025 (linux-next), leads to the
> following Smatch static checker warning:
> 
> 	drivers/gpu/drm/xe/xe_vm.c:2298 xe_vm_query_vmas_attrs_ioctl()
> 	warn: maybe return -EFAULT instead of the bytes remaining?
> 
> drivers/gpu/drm/xe/xe_vm.c
>      2240 int xe_vm_query_vmas_attrs_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>      2241 {
>      2242         struct xe_device *xe = to_xe_device(dev);
>      2243         struct xe_file *xef = to_xe_file(file);
>      2244         struct drm_xe_mem_range_attr *mem_attrs;
>      2245         struct drm_xe_vm_query_mem_range_attr *args = data;
>      2246         u64 __user *attrs_user = u64_to_user_ptr(args->vector_of_mem_attr);
>      2247         struct xe_vm *vm;
>      2248         int err = 0;
>      2249
>      2250         if (XE_IOCTL_DBG(xe,
>      2251                          ((args->num_mem_ranges == 0 &&
>      2252                           (attrs_user || args->sizeof_mem_range_attr != 0)) ||
>      2253                          (args->num_mem_ranges > 0 &&
>      2254                           (!attrs_user ||
>      2255                            args->sizeof_mem_range_attr !=
>      2256                            sizeof(struct drm_xe_mem_range_attr))))))
>      2257                 return -EINVAL;
>      2258
>      2259         vm = xe_vm_lookup(xef, args->vm_id);
>      2260         if (XE_IOCTL_DBG(xe, !vm))
>      2261                 return -EINVAL;
>      2262
>      2263         err = down_read_interruptible(&vm->lock);
>      2264         if (err)
>      2265                 goto put_vm;
>      2266
>      2267         attrs_user = u64_to_user_ptr(args->vector_of_mem_attr);
>      2268
>      2269         if (args->num_mem_ranges == 0 && !attrs_user) {
>      2270                 args->num_mem_ranges = xe_vm_query_vmas(vm, args->start, args->start + args->range);
>      2271                 args->sizeof_mem_range_attr = sizeof(struct drm_xe_mem_range_attr);
>      2272                 goto unlock_vm;
>      2273         }
>      2274
>      2275         mem_attrs = kvmalloc_array(args->num_mem_ranges, args->sizeof_mem_range_attr,
>      2276                                    GFP_KERNEL | __GFP_ACCOUNT |
>      2277                                    __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
>      2278         if (!mem_attrs) {
>      2279                 err = args->num_mem_ranges > 1 ? -ENOBUFS : -ENOMEM;
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^
> 
> This is a weird check.  If args->num_mem_ranges is zero, then kmalloc()
> will succeed with the ZERO_SIZE_PTR.  If it's 1, then
> args->sizeof_mem_range_attr is quite small.  64 bytes.  The allocation will
> succeed as well.  In real life err will never be set to -ENOBUFS.

This is false error.
> 
>      2280                 goto unlock_vm;
>      2281         }
>      2282
>      2283         memset(mem_attrs, 0, args->num_mem_ranges * args->sizeof_mem_range_attr);
>      2284         err = get_mem_attrs(vm, &args->num_mem_ranges, args->start,
>      2285                             args->start + args->range, mem_attrs);
>      2286         if (err)
>      2287                 goto free_mem_attrs;
>      2288
>      2289         err = copy_to_user(attrs_user, mem_attrs,
>      2290                            args->sizeof_mem_range_attr * args->num_mem_ranges);
> 
> copy_to_user() returns the number of bytes it failed to copy.  It should
> be:

will fix it.

Thanks

> 
> 	if (copy_to_user(...))
> 		err = -EFAULT;
> 
>      2291
>      2292 free_mem_attrs:
>      2293         kvfree(mem_attrs);
>      2294 unlock_vm:
>      2295         up_read(&vm->lock);
>      2296 put_vm:
>      2297         xe_vm_put(vm);
> --> 2298         return err;
>      2299 }
> 
> regards,
> dan carpenter