Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object

From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object
Date: Fri, 26 Oct 2018 10:01:20 +0200	[thread overview]
Message-ID: <403d08b8-5b1b-5b08-5cae-0a98520dcc9a@gmail.com> (raw)
In-Reply-To: <154050251352.8648.3496879509165225854@skylake-alporthouse-com>

Am 25.10.18 um 23:21 schrieb Chris Wilson:
> Quoting Chris Wilson (2018-10-25 21:20:21)
>> Quoting Chris Wilson (2018-10-25 21:15:17)
>>> Quoting Christian König (2018-10-04 14:12:43)
>>>> No need for that any more. Just replace the list when there isn't enough
>>>> room any more for the additional fence.
>>> Just a heads up. After this series landed, we started hitting a
>>> use-after-free when iterating the shared list.
>>>
>>> <4> [109.613162] general protection fault: 0000 [#1] PREEMPT SMP PTI
>>> <4> [109.613177] CPU: 1 PID: 1357 Comm: gem_busy Tainted: G     U            4.19.0-rc8-CI-CI_DRM_5035+ #1
>>> <4> [109.613189] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
>>> <4> [109.613252] RIP: 0010:i915_gem_busy_ioctl+0x146/0x380 [i915]
>>> <4> [109.613261] Code: 0b 43 04 49 83 c6 08 4d 39 e6 89 43 04 74 6d 4d 8b 3e e8 5d 54 f4 e0 85 c0 74 0d 80 3d 08 71 1d 00 00 0f 84 bb 00 00 00 31 c0 <49> 81 7f 08 20 3a 2c a0 75 cc 41 8b 97 50 02 00 00 49 8b 8f a8 00
>>> <4> [109.613283] RSP: 0018:ffffc9000044bcf8 EFLAGS: 00010246
>>> <4> [109.613292] RAX: 0000000000000000 RBX: ffffc9000044bdc0 RCX: 0000000000000001
>>> <4> [109.613302] RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffffffff822474a0
>>> <4> [109.613311] RBP: ffffc9000044bd28 R08: ffff88021e158680 R09: 0000000000000001
>>> <4> [109.613321] R10: 0000000000000040 R11: 0000000000000000 R12: ffff88021e1641b8
>>> <4> [109.613331] R13: 0000000000000003 R14: ffff88021e1641b0 R15: 6b6b6b6b6b6b6b6b
>>> <4> [109.613341] FS:  00007f9c9fc84980(0000) GS:ffff880227a40000(0000) knlGS:0000000000000000
>>> <4> [109.613352] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> <4> [109.613360] CR2: 00007f9c9fcb8000 CR3: 00000002247d4005 CR4: 00000000000606e0
>> First guess would be
>>
>> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
>> index 5fb4fd461908..833698a0d548 100644
>> --- a/drivers/dma-buf/reservation.c
>> +++ b/drivers/dma-buf/reservation.c
>> @@ -169,7 +169,6 @@ void reservation_object_add_shared_fence(struct reservation_object *obj,
>>          }
>>
>>          BUG_ON(fobj->shared_count >= fobj->shared_max);
>> -       fobj->shared_count++;
>>
>>   replace:
>>          /*
>> @@ -177,6 +176,8 @@ void reservation_object_add_shared_fence(struct reservation_object *obj,
>>           * fobj->shared_count is protected by this lock too
>>           */
>>          RCU_INIT_POINTER(fobj->shared[i], fence);
>> +       if (i == fobj->shared_count)
>> +               fobj->shared_count++;
>>          write_seqcount_end(&obj->seq);
>>          preempt_enable();
>>   }
>>
>> Strictly, perhaps WRITE_ONCE(fobj->shared_count, i + 1); ?
> Updating shared_count after setting the fence does the trick.

Ah, crap. I can stare at the code for ages and don't see that problem. 
Neither did any internal testing showed any issues.

Anyway your change is Reviewed-by: Christian König 
<christian.koenig@amd.com>

Thanks for the quick fix,
Christian.

> -Chris

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel