[PATCH 1/1] drm: check for NULL pointer in drm_gem_object_put

[Emil Velikov]

On Wed, 20 May 2020 at 14:30, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
> On Wed, 20 May 2020 at 14:17, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >
> > Quoting Christian König (2020-05-20 13:54:55)
> > > Am 20.05.20 um 14:49 schrieb Chris Wilson:
> > > > Quoting Christian König (2020-05-20 13:19:42)
> > > >> Am 20.05.20 um 14:14 schrieb Nirmoy Das:
> > > >>> drm_gem_fb_destroy() calls drm_gem_object_put() with NULL obj causing:
> > > >>> [   11.584209] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > >>> [   11.584213] #PF: supervisor write access in kernel mode
> > > >>> [   11.584215] #PF: error_code(0x0002) - not-present page
> > > >>> [   11.584216] PGD 0 P4D 0
> > > >>> [   11.584220] Oops: 0002 [#1] SMP NOPTI
> > > >>> [   11.584223] CPU: 7 PID: 1571 Comm: gnome-shell Tainted: G            E     5.7.0-rc1-1-default+ #27
> > > >>> [   11.584225] Hardware name: Micro-Star International Co., Ltd. MS-7A31/X370 XPOWER GAMING TITANIUM (MS-7A31), BIOS 1.MR 12/03/2019
> > > >>> [   11.584237] RIP: 0010:drm_gem_fb_destroy+0x28/0x70 [drm_kms_helper]
> > > >>> <snip>
> > > >>> [   11.584256] Call Trace:
> > > >>> [   11.584279]  drm_mode_rmfb+0x189/0x1c0 [drm]
> > > >>> [   11.584299]  ? drm_mode_rmfb+0x1c0/0x1c0 [drm]
> > > >>> [   11.584314]  drm_ioctl_kernel+0xaa/0xf0 [drm]
> > > >>> [   11.584329]  drm_ioctl+0x1ff/0x3b0 [drm]
> > > >>> [   11.584347]  ? drm_mode_rmfb+0x1c0/0x1c0 [drm]
> > > >>> [   11.584421]  amdgpu_drm_ioctl+0x49/0x80 [amdgpu]
> > > >>> [   11.584427]  ksys_ioctl+0x87/0xc0
> > > >>> [   11.584430]  __x64_sys_ioctl+0x16/0x20
> > > >>> [   11.584434]  do_syscall_64+0x5f/0x240
> > > >>> [   11.584438]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > >>> [   11.584440] RIP: 0033:0x7f0ef80f7227
> > > >>>
> > > >>> Signed-off-by: Nirmoy Das <nirmoy.das at amd.com>
> > > >> Fixes: .... missing here. Apart from that Reviewed-by: Christian König
> > > >> <christian.koenig at amd.com>.
> > > >>
> > > >> Please resend with the tag added, then I'm going to push this to
> > > >> drm-misc-next asap.
> > > >>
> > > >> Christian.
> > > >>
> > > >>> ---
> > > >>>    include/drm/drm_gem.h | 3 +++
> > > >>>    1 file changed, 3 insertions(+)
> > > >>>
> > > >>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > > >>> index 52173abdf500..a13510346a9b 100644
> > > >>> --- a/include/drm/drm_gem.h
> > > >>> +++ b/include/drm/drm_gem.h
> > > >>> @@ -372,6 +372,9 @@ static inline void drm_gem_object_get(struct drm_gem_object *obj)
> > > >>>    static inline void
> > > >>>    drm_gem_object_put(struct drm_gem_object *obj)
> > > >>>    {
> > > >>> +     if (!obj)
> > > >>> +             return;
> > > >>> +
> > > > This adds several thousand NULL checks where there were previously none.
> > > > I doubt the compiler eliminates them all.
> > > >
> > > > I'd suggest reverting
> > > > b5d250744ccc ("drm/gem: fold drm_gem_object_put_unlocked and __drm_gem_object_put()")
> > >
> > > I didn't looked into this patch in detail, but allowing to call *_put()
> > > functions with NULL and nothing bad happens is rather common practice.
> > >
> > > On the other hand I agree that this might cause a performance problem.
> > > How many sites do we have which could call drm_gem_object_put() with NULL?
> >
> > Just to put this in a tiny bit of perspective, for i915.ko
> >
> > add/remove: 0/0 grow/shrink: 141/20 up/down: 2211/-525 (1686)
> >
> > We've had flame wars for less. (Because it's easy to argue over little
> > changes.) Now this is just from this patch and not the revert...
> > The revert has no effect on code size.
>
> If we play the revert game thing will never end with having it fixed :-(
> I'd suggest sticking with the NULL check, maybe even a WARN to aid
> debug the 240 usecases.
>
> For the patch:
> Fixes: b5d250744ccc ("drm/gem: fold drm_gem_object_put_unlocked and
> __drm_gem_object_put()")
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
>
Alternatively, if you give me 30 minutes I should be able to finish
auditing the users ;-)

-Emil