Re: [PATCH] drm/syncobj: Fix syncobj leak in drm_syncobj_eventfd_ioctl

[Christian König]

Am 10.09.24 um 16:47 schrieb T.J. Mercier:
> On Tue, Sep 10, 2024 at 12:30 AM Christian König
> <christian.koenig@xxxxxxx> wrote:
> > Am 10.09.24 um 09:26 schrieb Tvrtko Ursulin:
> > > On 09/09/2024 21:53, T.J. Mercier wrote:
> > > > A syncobj reference is taken in drm_syncobj_find, but not released if
> > > > eventfd_ctx_fdget or kzalloc fails. Put the reference in these error
> > > > paths.
> > > >
> > > > Reported-by: Xingyu Jin <xingyuj@xxxxxxxxxx>
> > > > Fixes: c7a472297169 ("drm/syncobj: add IOCTL to register an eventfd")
> > > > Signed-off-by: T.J. Mercier <tjmercier@xxxxxxxxxx>
> > > > ---
> > > >    drivers/gpu/drm/drm_syncobj.c | 17 +++++++++++++----
> > > >    1 file changed, 13 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_syncobj.c
> > > > b/drivers/gpu/drm/drm_syncobj.c
> > > > index a0e94217b511..4fcfc0b9b386 100644
> > > > --- a/drivers/gpu/drm/drm_syncobj.c
> > > > +++ b/drivers/gpu/drm/drm_syncobj.c
> > > > @@ -1464,6 +1464,7 @@ drm_syncobj_eventfd_ioctl(struct drm_device
> > > > *dev, void *data,
> > > >        struct drm_syncobj *syncobj;
> > > >        struct eventfd_ctx *ev_fd_ctx;
> > > >        struct syncobj_eventfd_entry *entry;
> > > > +    int ret;
> > > >          if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
> > > >            return -EOPNOTSUPP;
> > > > @@ -1479,13 +1480,15 @@ drm_syncobj_eventfd_ioctl(struct drm_device
> > > > *dev, void *data,
> > > >            return -ENOENT;
> > > >          ev_fd_ctx = eventfd_ctx_fdget(args->fd);
> > > > -    if (IS_ERR(ev_fd_ctx))
> > > > -        return PTR_ERR(ev_fd_ctx);
> > > > +    if (IS_ERR(ev_fd_ctx)) {
> > > > +        ret = PTR_ERR(ev_fd_ctx);
> > > > +        goto err_fdget;
> > > > +    }
> > > >          entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > > >        if (!entry) {
> > > > -        eventfd_ctx_put(ev_fd_ctx);
> > > > -        return -ENOMEM;
> > > > +        ret = -ENOMEM;
> > > > +        goto err_kzalloc;
> > > >        }
> > > >        entry->syncobj = syncobj;
> > > >        entry->ev_fd_ctx = ev_fd_ctx;
> > > > @@ -1496,6 +1499,12 @@ drm_syncobj_eventfd_ioctl(struct drm_device
> > > > *dev, void *data,
> > > >        drm_syncobj_put(syncobj);
> > > >          return 0;
> > > > +
> > > > +err_kzalloc:
> > > > +    eventfd_ctx_put(ev_fd_ctx);
> > > > +err_fdget:
> > > > +    drm_syncobj_put(syncobj);
> > > > +    return ret;
> > > >    }
> > > >      int
> > > Easy enough to review while browsing the list:
> > >
> > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
> > Looks reasonable to me as well.
> >
> > Reviewed-by. Christian König <christian.koenig@xxxxxxx>
> Thanks!
>
> > CC: stable?
> Yes, I think we should. 6.6 and 6.10
>
> > Let me know when you need someone to push it to drm-misc-fixes.
> Anytime is good, no rush for this one.

Done.

Christian.

> > Regards,
> > Christian.
> >
> > > Regards,
> > >
> > > Tvrtko