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