On 11/25/25 14:39, Sokolowski, Jan wrote: > > >> -----Original Message----- >> From: Christian König <[email protected]> >> Sent: Tuesday, November 25, 2025 2:23 PM >> To: Sokolowski, Jan <[email protected]>; dri- >> [email protected] >> Cc: David Francis <[email protected]>; Maarten Lankhorst >> <[email protected]>; Maxime Ripard >> <[email protected]>; Thomas Zimmermann <[email protected]>; >> David Airlie <[email protected]>; Simona Vetter <[email protected]>; Felix >> Kuehling <[email protected]>; De Marchi, Lucas >> <[email protected]> >> Subject: Re: [PATCH 1/1] drm: disallow setting 0 as new handle in >> DRM_IOCTL_GEM_CHANGE_HANDLE >> >> On 11/25/25 14:02, Sokolowski, Jan wrote: >>>> -----Original Message----- >>>> From: Christian König <[email protected]> >>>> Sent: Tuesday, November 25, 2025 1:54 PM >>>> To: Sokolowski, Jan <[email protected]>; dri- >>>> [email protected] >>>> Cc: David Francis <[email protected]>; Maarten Lankhorst >>>> <[email protected]>; Maxime Ripard >>>> <[email protected]>; Thomas Zimmermann <[email protected]>; >>>> David Airlie <[email protected]>; Simona Vetter <[email protected]>; Felix >>>> Kuehling <[email protected]>; De Marchi, Lucas >>>> <[email protected]> >>>> Subject: Re: [PATCH 1/1] drm: disallow setting 0 as new handle in >>>> DRM_IOCTL_GEM_CHANGE_HANDLE >>>> >>>> On 11/25/25 11:28, Jan Sokolowski wrote: >>>>> drm_file's object_idr uses 1 as base value, which can cause id >>>>> mismatch when trying to use DRM_IOCTL_GEM_CHANGE_HANDLE >>>>> to change id from 1 to 0. >>>>> >>>>> Disallow 0 as new handle in that ioctl. >>>>> >>>>> Fixes: 53096728b891 ("drm: Add DRM prime interface to reassign GEM >>>> handle") >>>>> Signed-off-by: Jan Sokolowski <[email protected]> >>>>> Cc: David Francis <[email protected]> >>>>> Cc: Maarten Lankhorst <[email protected]> >>>>> Cc: Maxime Ripard <[email protected]> >>>>> Cc: Thomas Zimmermann <[email protected]> >>>>> Cc: David Airlie <[email protected]> >>>>> Cc: Simona Vetter <[email protected]> >>>>> Cc: "Christian König" <[email protected]> >>>>> Cc: Felix Kuehling <[email protected]> >>>>> Cc: Lucas De Marchi <[email protected]> >>>>> --- >>>>> drivers/gpu/drm/drm_gem.c | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >>>>> index 68168d58a7c8..2a49a8e396fa 100644 >>>>> --- a/drivers/gpu/drm/drm_gem.c >>>>> +++ b/drivers/gpu/drm/drm_gem.c >>>>> @@ -975,6 +975,10 @@ int drm_gem_change_handle_ioctl(struct >>>> drm_device *dev, void *data, >>>>> if (args->handle == args->new_handle) >>>>> return 0; >>>>> >>>>> + /* As the idr base is 1, trying to set handle 0 will create id mismatch >>>> */ >>>>> + if (args->new_handle == 0) >>>>> + return 0; >>>> >>>> That would need to return -EINVAl or some other error code. >>> >>> Ok, will change that in next version. >>> >>>> >>>> But I'm wondering why that is necessary at all? Doesn't idr_alloc() return >> an >>>> error when you try to allocate handle 0? >>> >>> It appears that idr_alloc doesn't return any errors in this scenario, >> otherwise we'd goto out_unlock straight away. >> >> Mhm, that is a bit misleading. We intentionally initialized the idr so that >> it >> starts at 1. Maybe idr_alloc() should be fixed instead. > > I've checked what idr_alloc does, and it looks like it'll return 2 in this > case,
Well that would absolute clearly be a bug in idr. The start parameter is documented as the minimum (inclusive) value returned and the end parameter is documented as the maximum (exclusive) value returned. So if you ask for value 0 and get 2 instead that is clearly not the documented behavior. Something fishy is going on here. Please investigate what is actually happening and why. My educated guess is that the base is just ignored by idr_alloc(), that's a bit surprising but at least not otherwise documented. Regards, Christian. > which is the next reserved id (0 is reserved, so it goes to 1) + base (1). > Maybe the solution would be, if idr_alloc returns a value other than > args->new_handle, then > a) bail out > b) new_handle = idr_alloc, and inform user that the assignment was changed to > another handle. > > As idr is used in many other places, I don't think that's feasible or easy to > fix that. > >> >> But that is clearly a much larger change, with the return code fixed feel >> free >> to add Reviewed-by: Christian König <[email protected]> to this >> patch here. >> >> Adding a CC: stable tag sound appropriate as well. >> >> Regards, >> Christian. >> >>> >>>> >>>> Regards, >>>> Christian. >>>> >>>>> + >>>>> mutex_lock(&file_priv->prime.lock); >>>>> >>>>> spin_lock(&file_priv->table_lock); >>> >
