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

[Dan Carpenter <dan.carpenter@linaro.org>]

On Thu, Aug 28, 2025 at 03:10:40PM +0530, Ghimiray, Himal Prasad wrote:
> 
> 
> 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.

This wasn't from static analysis it was just from observation.  At the
start of the function we ensure that if args->num_mem_ranges is > 0 then
args->sizeof_mem_range_attr must be == sizeof(struct drm_xe_mem_range_attr).

> >      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;
>

I guess maybe the thing I didn't explain well enough is that in the
kernel small allocations always succeed.  Otherwise I'm so puzzled by how
my analysis could be wrong.

regards,
dan carpenter