[PATCH 1/1] drm: check for NULL pointer in drm_gem_object_put
Christian König
christian.koenig at amd.com
Wed May 20 13:03:21 UTC 2020
Am 20.05.20 um 15:00 schrieb Daniel Vetter:
> On Wed, May 20, 2020 at 02:54:55PM +0200, Christian König wrote:
>> 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?
> Hm how did we even get to a place where one of the _put functions had a
> NULL check and the other didn't?
No idea.
> I do expect the compiler to clean up the entire mess, only place where I
> can think of NULL checks is dumb cleanup code when driver load failed
> halfway through. In all other places the compiler should have some
> evidence that the pointer isn't NULL. But would be good to check that's
> the case and we're not doing something stupid here ...
Well Nirmoy is blocked, so we need a solution fast. Auditing all call
sites is not an option.
Revert or this one here I would say, either one is fine with me.
Christian.
> -Daniel
More information about the dri-devel
mailing list