* [bug report] drm/xe/uapi: Add UAPI for querying VMA count and memory attributes
@ 2025-08-28 9:01 Dan Carpenter
2025-08-28 9:40 ` Ghimiray, Himal Prasad
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-08-28 9:01 UTC (permalink / raw)
To: Himal Prasad Ghimiray; +Cc: intel-xe
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.
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:
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] drm/xe/uapi: Add UAPI for querying VMA count and memory attributes
2025-08-28 9:01 [bug report] drm/xe/uapi: Add UAPI for querying VMA count and memory attributes Dan Carpenter
@ 2025-08-28 9:40 ` Ghimiray, Himal Prasad
2025-08-29 9:05 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Ghimiray, Himal Prasad @ 2025-08-28 9:40 UTC (permalink / raw)
To: Dan Carpenter; +Cc: intel-xe
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] drm/xe/uapi: Add UAPI for querying VMA count and memory attributes
2025-08-28 9:40 ` Ghimiray, Himal Prasad
@ 2025-08-29 9:05 ` Dan Carpenter
2025-09-01 5:29 ` Ghimiray, Himal Prasad
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-08-29 9:05 UTC (permalink / raw)
To: Ghimiray, Himal Prasad; +Cc: intel-xe
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] drm/xe/uapi: Add UAPI for querying VMA count and memory attributes
2025-08-29 9:05 ` Dan Carpenter
@ 2025-09-01 5:29 ` Ghimiray, Himal Prasad
0 siblings, 0 replies; 4+ messages in thread
From: Ghimiray, Himal Prasad @ 2025-09-01 5:29 UTC (permalink / raw)
To: Dan Carpenter; +Cc: intel-xe
On 29-08-2025 14:35, Dan Carpenter wrote:
> 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.
if kvmalloc_array fails for num_mem_ranges > 1 error can be -ENOBUFS, I
am not sure why -ENOBUFS will never be set if mem allio >>
>> 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.
Thanks for taking out time on responding to this.
We do ensure that args->sizeof_mem_range_attr == sizeof(struct
drm_xe_mem_range_attr), which guarantees that the structure size is
correct. However, I’m struggling to understand how this check leads
to the conclusion that -ENOBUFS can never be returned.
UAPI behaviour:
If memory allocation fails for a single range, we return -ENOMEM.
If allocation fails for multiple ranges, we return -ENOBUFS.
While I agree that allocation for a single range is typically small and
unlikely to fail, I believe the error handling logic is still valuable
and does no harm. In fact, it seems prudent to retain it for robustness.
If there’s a kernel guideline or kernel-doc reference that explicitly
recommends not adding error handling for small allocations, I’d be happy
to remove the check. Otherwise, I’d prefer to keep it for completeness
and safety.
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-09-01 5:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-28 9:01 [bug report] drm/xe/uapi: Add UAPI for querying VMA count and memory attributes Dan Carpenter
2025-08-28 9:40 ` Ghimiray, Himal Prasad
2025-08-29 9:05 ` Dan Carpenter
2025-09-01 5:29 ` Ghimiray, Himal Prasad
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox