[PATCH 21/22] dma-buf/reservation: shouldn't kfree staged when slot available

[Monk.Liu@xxxxxxx (Liu, Monk)]

Hi Christian

we call reservation_object_reserve_shared() at very first time, and later we call
reservation_object_add_shared_fence(), which will call reservation_object_add_shared_replace() since
fobj is not NULL by this time.

in reservation_object_get_list(), @old is NULL, @fobj is not NULL, so @fence will init the @fobj->shared[]
and @fobj->shared_count set to 1, and later the @obj->fence will be set to @fobj  (through RCU_INIT_POINTER)
and this is the part obj->fence related with obj->staged...

At next time calls on reservation_object_reserve_shared(), since current logic will kfree @obj->staged, which
let above @obj->fence points to a wild pointer, and lead to weird bugs,

in fact we pass stress test on vulkan CTS and vk_example after this patch applied ... 

/Monk


-----Original Message-----
From: Christian König [mailto:ckoenig.leichtzumerken@xxxxxxxxx] 
Sent: 2018å¹´2æ??26æ?¥ 17:42
To: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org; Chris Wilson <chris at chris-wilson.co.uk>
Subject: Re: [PATCH 21/22] dma-buf/reservation: shouldn't kfree staged when slot available

Well first of all you need to send that to dri-devel and even lkml, so that Chris and others can take a look as well.

Second that patch doesn't looks correct to me, obj->staged should never be related to obj->fence.

Regards,
Christian.

Am 26.02.2018 um 06:34 schrieb Monk Liu:
> issue:
> kernel oops or vmc page fault occured during vk_example/vk_cts test.
>
> root cause:
> previously reservation object would kfree the staged when slot checked 
> available during reserve_shared(), which is incorrect becasue this way 
> reservation_object->fence will be a wild pointer referenced by 
> following reservation_object_add_shared_fence,
> and lead to lot of abnormal cases depends on luck.
>
> fix:
> don't call kfree on staged in reserve_shared and there won't be 
> memleak introduced because reservation's finish routine would kfree 
> both staged and fence
>
> Change-Id: If7c01f1b4be3d3d8a81efa90216841f79ab1fc1c
> Signed-off-by: Monk Liu <Monk.Liu at amd.com>
> ---
>   drivers/dma-buf/reservation.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma-buf/reservation.c 
> b/drivers/dma-buf/reservation.c index 314eb10..bc01e0d 100644
> --- a/drivers/dma-buf/reservation.c
> +++ b/drivers/dma-buf/reservation.c
> @@ -74,12 +74,9 @@ int reservation_object_reserve_shared(struct reservation_object *obj)
>   	old = reservation_object_get_list(obj);
>   
>   	if (old && old->shared_max) {
> -		if (old->shared_count < old->shared_max) {
> -			/* perform an in-place update */
> -			kfree(obj->staged);
> -			obj->staged = NULL;
> +		if (old->shared_count < old->shared_max)
>   			return 0;
> -		} else
> +		else
>   			max = old->shared_max * 2;
>   	} else
>   		max = 4;