[PATCH] drm/amd/scheduler: fix one used-after-free case for job->s_entity

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



In this case we remove the entity from the rq and start/stop the main 
thread to be sure that the entity isn't in use any more.

The start/stop is totally a hack as well and I hope to remove that soon, 
but for now it should be sufficient.

Christian.

Am 24.10.2017 um 13:42 schrieb Liu, Monk:
> What about the case that user interrupt the entity_fini() ?
>
> -----Original Message-----
> From: Koenig, Christian
> Sent: 2017å¹´10æ??24æ?¥ 19:08
> To: Liu, Monk <Monk.Liu at amd.com>; Zhou, David(ChunMing) <David1.Zhou at amd.com>; amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH] drm/amd/scheduler: fix one used-after-free case for job->s_entity
>
> I suggest to move the guilty check into _pop_job() before we actually remove the job from the entity. If the entity is guilty we can abort early there.
>
> Only for recovering the job I didn't had a good idea yet. Probably best to just find the entity using the fence context number once more.
>
> Regards,
> Christian.
>
> Am 24.10.2017 um 13:05 schrieb Liu, Monk:
>> Oh yeah, right ,
>>
>> So the only remaining wild pointer is we still allow entity access in
>> job_run() in sched_job_recovery(), under the gpu reset patch,
>>
>> Need find a way to avoid this accessing
>>
>>
>> BR Monk
>>
>> -----Original Message-----
>> From: Koenig, Christian
>> Sent: 2017å¹´10æ??24æ?¥ 19:00
>> To: Liu, Monk <Monk.Liu at amd.com>; Zhou, David(ChunMing)
>> <David1.Zhou at amd.com>; amd-gfx at lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amd/scheduler: fix one used-after-free case
>> for job->s_entity
>>
>> Am 24.10.2017 um 12:54 schrieb Liu, Monk:
>>>> No, that is unproblematic. The entity won't be freed until all its jobs are freed.
>>> The entity could be freed as long as all jobs in its KFIFO are
>>> scheduled to ring, and after that in scheduler's main routine any
>>> access to entity without holding the RQ's lock Is dangerous
>> Correct, but take a look at the code again. We don't access the entity any more after removing the job from it.
>>
>> So the entity can safely be destroyed while we are processing its last job.
>>
>> Regards,
>> Christian.
>>
>>> Br Monk
>>>
>>> -----Original Message-----
>>> From: Koenig, Christian
>>> Sent: 2017å¹´10æ??24æ?¥ 18:38
>>> To: Liu, Monk <Monk.Liu at amd.com>; Zhou, David(ChunMing)
>>> <David1.Zhou at amd.com>; amd-gfx at lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amd/scheduler: fix one used-after-free case
>>> for job->s_entity
>>>
>>>> But immediately you call "sched_job =
>>>> amd_sched_entity_peek_job(entity)", keep in mind that this point the
>>>> "entity" may Already be a wild pointer, since above
>>>> "sched_select_entity' unlock the RQ's spinlock
>>> No, that is unproblematic. The entity won't be freed until all its jobs are freed.
>>>
>>>> For gpu reset feature, it's more complicated, because in run_job()
>>>> routine we need check entity's guilty pointer, but that time The
>>>> entity from sched_job->s_entity may also already a wild pointer
>>> Crap, that is indeed a problem. We should probably rather move that check into amd_sched_entity_pop_job().
>>>
>>> Problem is that we can't check the entity then again during job recovery. Need to take a second look at this.
>>>
>>> Christian.
>>>
>>> Am 24.10.2017 um 12:17 schrieb Liu, Monk:
>>>> You get entity from "amd_sched_select_entity", and after that the
>>>> spinlock of RQ backing this entity is also unlocked,
>>>>
>>>> But immediately you call "sched_job =
>>>> amd_sched_entity_peek_job(entity)", keep in mind that this point the
>>>> "entity" may Already be a wild pointer, since above
>>>> "sched_select_entity' unlock the RQ's spinlock
>>>>
>>>> For gpu reset feature, it's more complicated, because in run_job()
>>>> routine we need check entity's guilty pointer, but that time The
>>>> entity from sched_job->s_entity may also already a wild pointer
>>>>
>>>>
>>>> Hah, headache
>>>>
>>>> BR Monk
>>>>
>>>> -----Original Message-----
>>>> From: Koenig, Christian
>>>> Sent: 2017å¹´10æ??24æ?¥ 18:13
>>>> To: Liu, Monk <Monk.Liu at amd.com>; Zhou, David(ChunMing)
>>>> <David1.Zhou at amd.com>; amd-gfx at lists.freedesktop.org
>>>> Subject: Re: [PATCH] drm/amd/scheduler: fix one used-after-free case
>>>> for job->s_entity
>>>>
>>>> I thought we had fixed that one as well.
>>>>
>>>> The entity isn't accessed any more after amd_sched_entity_pop_job().
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> Am 24.10.2017 um 12:06 schrieb Liu, Monk:
>>>>> Christian
>>>>>
>>>>> Actually there are more wild pointer issue for entity in scheduler's main routine ....
>>>>>
>>>>>
>>>>> See the message I replied to David
>>>>>
>>>>> BR Monk
>>>>>
>>>>> -----Original Message-----
>>>>> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On
>>>>> Behalf Of Christian K?nig
>>>>> Sent: 2017å¹´10æ??24æ?¥ 18:01
>>>>> To: Zhou, David(ChunMing) <David1.Zhou at amd.com>;
>>>>> amd-gfx at lists.freedesktop.org
>>>>> Subject: Re: [PATCH] drm/amd/scheduler: fix one used-after-free
>>>>> case for job->s_entity
>>>>>
>>>>> Andrey already submitted a fix for this a few days ago.
>>>>>
>>>>> Christian.
>>>>>
>>>>> Am 24.10.2017 um 11:55 schrieb Chunming Zhou:
>>>>>> The s_entity presented process could already be closed when calling amdgpu_job_free_cb.
>>>>>> the s_entity will be buggy pointer after it's freed. See below calltrace:
>>>>>>
>>>>>> [  355.616964]
>>>>>> ==================================================================
>>>>>> [  355.617191] BUG: KASAN: use-after-free in
>>>>>> amdgpu_job_free_cb+0x2f/0xc0 [amdgpu] [  355.617197] Read of size
>>>>>> 8 at addr ffff88039d593c40 by task kworker/9:1/100
>>>>>>
>>>>>> [  355.617206] CPU: 9 PID: 100 Comm: kworker/9:1 Not tainted
>>>>>> 4.13.0-custom #1 [  355.617208] Hardware name: Gigabyte Technology
>>>>>> Co., Ltd. Default string/X99P-SLI-CF, BIOS F23 07/22/2016 [
>>>>>> 355.617342] Workqueue: events amd_sched_job_finish [amdgpu] [  355.617344] Call Trace:
>>>>>> [  355.617351]  dump_stack+0x63/0x8d [  355.617356]
>>>>>> print_address_description+0x70/0x290
>>>>>> [  355.617474]  ? amdgpu_job_free_cb+0x2f/0xc0 [amdgpu] [
>>>>>> 355.617477]
>>>>>> kasan_report+0x265/0x350 [  355.617479]  __asan_load8+0x54/0x90 [
>>>>>> 355.617603]  amdgpu_job_free_cb+0x2f/0xc0 [amdgpu] [  355.617721]
>>>>>> amd_sched_job_finish+0x161/0x180 [amdgpu] [  355.617725]
>>>>>> process_one_work+0x2ab/0x700 [  355.617727]
>>>>>> worker_thread+0x90/0x720 [  355.617731]  kthread+0x18c/0x1e0 [  355.617732]  ?
>>>>>> process_one_work+0x700/0x700 [  355.617735]  ?
>>>>>> kthread_create_on_node+0xb0/0xb0 [  355.617738]
>>>>>> ret_from_fork+0x25/0x30
>>>>>>
>>>>>> [  355.617742] Allocated by task 1347:
>>>>>> [  355.617747]  save_stack_trace+0x1b/0x20 [  355.617749]
>>>>>> save_stack+0x46/0xd0 [  355.617751]  kasan_kmalloc+0xad/0xe0 [
>>>>>> 355.617753]  kmem_cache_alloc_trace+0xef/0x200 [  355.617853]
>>>>>> amdgpu_driver_open_kms+0x98/0x290 [amdgpu] [  355.617883]
>>>>>> drm_open+0x38c/0x6e0 [drm] [  355.617908]
>>>>>> drm_stub_open+0x144/0x1b0 [drm] [  355.617911]
>>>>>> chrdev_open+0x180/0x320 [  355.617913]
>>>>>> do_dentry_open+0x3a2/0x570 [  355.617915]  vfs_open+0x86/0xe0 [
>>>>>> 355.617918]  path_openat+0x49e/0x1db0 [  355.617919]
>>>>>> do_filp_open+0x11c/0x1a0 [  355.617921]  do_sys_open+0x16f/0x2a0 [
>>>>>> 355.617923]  SyS_open+0x1e/0x20 [  355.617926]
>>>>>> do_syscall_64+0xea/0x210 [  355.617928]
>>>>>> return_from_SYSCALL_64+0x0/0x6a
>>>>>>
>>>>>> [  355.617931] Freed by task 1347:
>>>>>> [  355.617934]  save_stack_trace+0x1b/0x20 [  355.617936]
>>>>>> save_stack+0x46/0xd0 [  355.617937]  kasan_slab_free+0x70/0xc0 [
>>>>>> 355.617939]  kfree+0x9d/0x1c0 [  355.618038]
>>>>>> amdgpu_driver_postclose_kms+0x1bc/0x3e0 [amdgpu] [  355.618063]
>>>>>> drm_release+0x454/0x610 [drm] [  355.618065]  __fput+0x177/0x350 [
>>>>>> 355.618066]  ____fput+0xe/0x10 [  355.618068]
>>>>>> task_work_run+0xa0/0xc0 [  355.618070]  do_exit+0x456/0x1320 [
>>>>>> 355.618072]
>>>>>> do_group_exit+0x86/0x130 [  355.618074]  SyS_exit_group+0x1d/0x20
>>>>>> [ 355.618076]  do_syscall_64+0xea/0x210 [  355.618078]
>>>>>> return_from_SYSCALL_64+0x0/0x6a
>>>>>>
>>>>>> [  355.618081] The buggy address belongs to the object at ffff88039d593b80
>>>>>>                       which belongs to the cache kmalloc-2048 of
>>>>>> size
>>>>>> 2048 [  355.618085] The buggy address is located 192 bytes inside of
>>>>>>                       2048-byte region [ffff88039d593b80,
>>>>>> ffff88039d594380) [  355.618087] The buggy address belongs to the page:
>>>>>> [  355.618091] page:ffffea000e756400 count:1 mapcount:0 mapping:          (null) index:0x0 compound_mapcount: 0
>>>>>> [  355.618095] flags: 0x2ffff0000008100(slab|head) [  355.618099] raw:
>>>>>> 02ffff0000008100 0000000000000000 0000000000000000
>>>>>> 00000001000f000f [ 355.618103] raw: ffffea000edb0600
>>>>>> 0000000200000002
>>>>>> ffff8803bfc0ea00
>>>>>> 0000000000000000 [  355.618105] page dumped because: kasan: bad
>>>>>> access detected
>>>>>>
>>>>>> [  355.618108] Memory state around the buggy address:
>>>>>> [  355.618110]  ffff88039d593b00: fc fc fc fc fc fc fc fc fc fc fc
>>>>>> fc fc fc fc fc [  355.618113]  ffff88039d593b80: fb fb fb fb fb fb
>>>>>> fb fb fb fb fb fb fb fb fb fb [  355.618116] >ffff88039d593c00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>>>>> [  355.618117]                                            ^
>>>>>> [  355.618120]  ffff88039d593c80: fb fb fb fb fb fb fb fb fb fb fb
>>>>>> fb fb fb fb fb [  355.618122]  ffff88039d593d00: fb fb fb fb fb fb
>>>>>> fb fb fb fb fb fb fb fb fb fb [  355.618124]
>>>>>> ==================================================================
>>>>>> [  355.618126] Disabling lock debugging due to kernel taint
>>>>>>
>>>>>> Change-Id: I8ff7122796b8cd16fc26e9c40e8d4c8153d67e0c
>>>>>> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
>>>>>> ---
>>>>>>        drivers/gpu/drm/amd/scheduler/gpu_scheduler.c |  1 +
>>>>>>        drivers/gpu/drm/amd/scheduler/gpu_scheduler.h | 27 ++++++++++++++-------------
>>>>>>        2 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>>>>> b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>>>>> index 007fdbd..8101ed7 100644
>>>>>> --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>>>>> +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>>>>> @@ -535,6 +535,7 @@ int amd_sched_job_init(struct amd_sched_job *job,
>>>>>>        	if (!job->s_fence)
>>>>>>        		return -ENOMEM;
>>>>>>        	job->id = atomic64_inc_return(&sched->job_id_count);
>>>>>> +	job->priority = job->s_entity->rq - job->sched->sched_rq;
>>>>>>        
>>>>>>        	INIT_WORK(&job->finish_work, amd_sched_job_finish);
>>>>>>        	INIT_LIST_HEAD(&job->node); diff --git
>>>>>> a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>>>>>> b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>>>>>> index e21299c..8808eb1 100644
>>>>>> --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>>>>>> +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>>>>>> @@ -77,6 +77,18 @@ struct amd_sched_fence {
>>>>>>        	void                            *owner;
>>>>>>        };
>>>>>>        
>>>>>> +enum amd_sched_priority {
>>>>>> +	AMD_SCHED_PRIORITY_MIN,
>>>>>> +	AMD_SCHED_PRIORITY_LOW = AMD_SCHED_PRIORITY_MIN,
>>>>>> +	AMD_SCHED_PRIORITY_NORMAL,
>>>>>> +	AMD_SCHED_PRIORITY_HIGH_SW,
>>>>>> +	AMD_SCHED_PRIORITY_HIGH_HW,
>>>>>> +	AMD_SCHED_PRIORITY_KERNEL,
>>>>>> +	AMD_SCHED_PRIORITY_MAX,
>>>>>> +	AMD_SCHED_PRIORITY_INVALID = -1,
>>>>>> +	AMD_SCHED_PRIORITY_UNSET = -2
>>>>>> +};
>>>>>> +
>>>>>>        struct amd_sched_job {
>>>>>>        	struct amd_gpu_scheduler        *sched;
>>>>>>        	struct amd_sched_entity         *s_entity;
>>>>>> @@ -87,6 +99,7 @@ struct amd_sched_job {
>>>>>>        	struct delayed_work		work_tdr;
>>>>>>        	uint64_t			id;
>>>>>>        	atomic_t karma;
>>>>>> +	enum amd_sched_priority		priority;
>>>>>>        };
>>>>>>        
>>>>>>        extern const struct dma_fence_ops
>>>>>> amd_sched_fence_ops_scheduled; @@
>>>>>> -118,18 +131,6 @@ struct amd_sched_backend_ops {
>>>>>>        	void (*free_job)(struct amd_sched_job *sched_job);
>>>>>>        };
>>>>>>        
>>>>>> -enum amd_sched_priority {
>>>>>> -	AMD_SCHED_PRIORITY_MIN,
>>>>>> -	AMD_SCHED_PRIORITY_LOW = AMD_SCHED_PRIORITY_MIN,
>>>>>> -	AMD_SCHED_PRIORITY_NORMAL,
>>>>>> -	AMD_SCHED_PRIORITY_HIGH_SW,
>>>>>> -	AMD_SCHED_PRIORITY_HIGH_HW,
>>>>>> -	AMD_SCHED_PRIORITY_KERNEL,
>>>>>> -	AMD_SCHED_PRIORITY_MAX,
>>>>>> -	AMD_SCHED_PRIORITY_INVALID = -1,
>>>>>> -	AMD_SCHED_PRIORITY_UNSET = -2
>>>>>> -};
>>>>>> -
>>>>>>        /**
>>>>>>         * One scheduler is implemented for each hardware ring
>>>>>>        */
>>>>>> @@ -183,7 +184,7 @@ void amd_sched_job_kickout(struct amd_sched_job *s_job);
>>>>>>        static inline enum amd_sched_priority
>>>>>>        amd_sched_get_job_priority(struct amd_sched_job *job)
>>>>>>        {
>>>>>> -	return (job->s_entity->rq - job->sched->sched_rq);
>>>>>> +	return job->priority;
>>>>>>        }
>>>>>>        
>>>>>>        #endif
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux