> -----Original Message----- > From: Christian König <[email protected]> > Sent: Tuesday, November 25, 2025 3:11 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: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.
I sent a RFC patch to linux kernel at vger ([RFC PATCH 0/1] IDR fix for potential id mismatch), hopefully that's the proper place for that patch. I'll wait for it to be reviewed before sending any further changes to drm_gem.c (as that patch would fix the warn anyway) Thanks for the review! > 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); > >>> > >
