From fedf54132d2410c3949036e3f611ab8dd9dbe89e Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Mon, 29 Aug 2016 08:08:30 +0100 Subject: dma-buf: Restart reservation_object_get_fences_rcu() after writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In order to be completely generic, we have to double check the read seqlock after acquiring a reference to the fence. If the driver is allocating fences from a SLAB_DESTROY_BY_RCU, or similar freelist, then within an RCU grace period a fence may be freed and reallocated. The RCU read side critical section does not prevent this reallocation, instead we have to inspect the reservation's seqlock to double check if the fences have been reassigned as we were acquiring our reference. Signed-off-by: Chris Wilson Cc: Daniel Vetter Cc: Maarten Lankhorst Cc: Christian König Cc: Alex Deucher Cc: Sumit Semwal Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org Reviewed-by: Daniel Vetter Signed-off-by: Sumit Semwal Link: http://patchwork.freedesktop.org/patch/msgid/20160829070834.22296-7-chris@chris-wilson.co.uk --- drivers/dma-buf/reservation.c | 71 +++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 40 deletions(-) (limited to 'drivers/dma-buf') diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 723d8af988e5..ba3e25dab95b 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -280,18 +280,24 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj, unsigned *pshared_count, struct fence ***pshared) { - unsigned shared_count = 0; - unsigned retry = 1; - struct fence **shared = NULL, *fence_excl = NULL; - int ret = 0; + struct fence **shared = NULL; + struct fence *fence_excl; + unsigned int shared_count; + int ret = 1; - while (retry) { + do { struct reservation_object_list *fobj; unsigned seq; + unsigned int i; - seq = read_seqcount_begin(&obj->seq); + shared_count = i = 0; rcu_read_lock(); + seq = read_seqcount_begin(&obj->seq); + + fence_excl = rcu_dereference(obj->fence_excl); + if (fence_excl && !fence_get_rcu(fence_excl)) + goto unlock; fobj = rcu_dereference(obj->fence); if (fobj) { @@ -309,52 +315,37 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj, } ret = -ENOMEM; - shared_count = 0; break; } shared = nshared; - memcpy(shared, fobj->shared, sz); shared_count = fobj->shared_count; - } else - shared_count = 0; - fence_excl = rcu_dereference(obj->fence_excl); - - retry = read_seqcount_retry(&obj->seq, seq); - if (retry) - goto unlock; - - if (!fence_excl || fence_get_rcu(fence_excl)) { - unsigned i; for (i = 0; i < shared_count; ++i) { - if (fence_get_rcu(shared[i])) - continue; - - /* uh oh, refcount failed, abort and retry */ - while (i--) - fence_put(shared[i]); - - if (fence_excl) { - fence_put(fence_excl); - fence_excl = NULL; - } - - retry = 1; - break; + shared[i] = rcu_dereference(fobj->shared[i]); + if (!fence_get_rcu(shared[i])) + break; } - } else - retry = 1; + } + + if (i != shared_count || read_seqcount_retry(&obj->seq, seq)) { + while (i--) + fence_put(shared[i]); + fence_put(fence_excl); + goto unlock; + } + ret = 0; unlock: rcu_read_unlock(); - } - *pshared_count = shared_count; - if (shared_count) - *pshared = shared; - else { - *pshared = NULL; + } while (ret); + + if (!shared_count) { kfree(shared); + shared = NULL; } + + *pshared_count = shared_count; + *pshared = shared; *pfence_excl = fence_excl; return ret; -- cgit v1.2.3 From 1cec20f0ea0e3bc617aed47e0936f17386c131f9 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Mon, 29 Aug 2016 08:08:31 +0100 Subject: dma-buf: Restart reservation_object_wait_timeout_rcu() after writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In order to be completely generic, we have to double check the read seqlock after acquiring a reference to the fence. If the driver is allocating fences from a SLAB_DESTROY_BY_RCU, or similar freelist, then within an RCU grace period a fence may be freed and reallocated. The RCU read side critical section does not prevent this reallocation, instead we have to inspect the reservation's seqlock to double check if the fences have been reassigned as we were acquiring our reference. Signed-off-by: Chris Wilson Cc: Daniel Vetter Cc: Maarten Lankhorst Cc: Christian König Cc: Alex Deucher Cc: Sumit Semwal Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org Reviewed-by: Daniel Vetter Signed-off-by: Sumit Semwal Link: http://patchwork.freedesktop.org/patch/msgid/20160829070834.22296-8-chris@chris-wilson.co.uk --- drivers/dma-buf/reservation.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'drivers/dma-buf') diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index ba3e25dab95b..648e5d0325f9 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -388,9 +388,6 @@ retry: if (fobj) shared_count = fobj->shared_count; - if (read_seqcount_retry(&obj->seq, seq)) - goto unlock_retry; - for (i = 0; i < shared_count; ++i) { struct fence *lfence = rcu_dereference(fobj->shared[i]); @@ -413,9 +410,6 @@ retry: if (!shared_count) { struct fence *fence_excl = rcu_dereference(obj->fence_excl); - if (read_seqcount_retry(&obj->seq, seq)) - goto unlock_retry; - if (fence_excl && !test_bit(FENCE_FLAG_SIGNALED_BIT, &fence_excl->flags)) { if (!fence_get_rcu(fence_excl)) @@ -430,6 +424,11 @@ retry: rcu_read_unlock(); if (fence) { + if (read_seqcount_retry(&obj->seq, seq)) { + fence_put(fence); + goto retry; + } + ret = fence_wait_timeout(fence, intr, ret); fence_put(fence); if (ret > 0 && wait_all && (i + 1 < shared_count)) -- cgit v1.2.3 From b68d8379c28ddfe74b78be9414082f281332d86c Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Mon, 29 Aug 2016 08:08:32 +0100 Subject: dma-buf: Restart reservation_object_test_signaled_rcu() after writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In order to be completely generic, we have to double check the read seqlock after acquiring a reference to the fence. If the driver is allocating fences from a SLAB_DESTROY_BY_RCU, or similar freelist, then within an RCU grace period a fence may be freed and reallocated. The RCU read side critical section does not prevent this reallocation, instead we have to inspect the reservation's seqlock to double check if the fences have been reassigned as we were acquiring our reference. Signed-off-by: Chris Wilson Cc: Daniel Vetter Cc: Maarten Lankhorst Cc: Christian König Cc: Alex Deucher Cc: Sumit Semwal Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org Reviewed-by: Daniel Vetter Signed-off-by: Sumit Semwal Link: http://patchwork.freedesktop.org/patch/msgid/20160829070834.22296-9-chris@chris-wilson.co.uk --- drivers/dma-buf/reservation.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) (limited to 'drivers/dma-buf') diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 648e5d0325f9..82de59f7cbbd 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -474,12 +474,13 @@ bool reservation_object_test_signaled_rcu(struct reservation_object *obj, bool test_all) { unsigned seq, shared_count; - int ret = true; + int ret; + rcu_read_lock(); retry: + ret = true; shared_count = 0; seq = read_seqcount_begin(&obj->seq); - rcu_read_lock(); if (test_all) { unsigned i; @@ -490,46 +491,35 @@ retry: if (fobj) shared_count = fobj->shared_count; - if (read_seqcount_retry(&obj->seq, seq)) - goto unlock_retry; - for (i = 0; i < shared_count; ++i) { struct fence *fence = rcu_dereference(fobj->shared[i]); ret = reservation_object_test_signaled_single(fence); if (ret < 0) - goto unlock_retry; + goto retry; else if (!ret) break; } - /* - * There could be a read_seqcount_retry here, but nothing cares - * about whether it's the old or newer fence pointers that are - * signaled. That race could still have happened after checking - * read_seqcount_retry. If you care, use ww_mutex_lock. - */ + if (read_seqcount_retry(&obj->seq, seq)) + goto retry; } if (!shared_count) { struct fence *fence_excl = rcu_dereference(obj->fence_excl); - if (read_seqcount_retry(&obj->seq, seq)) - goto unlock_retry; - if (fence_excl) { ret = reservation_object_test_signaled_single( fence_excl); if (ret < 0) - goto unlock_retry; + goto retry; + + if (read_seqcount_retry(&obj->seq, seq)) + goto retry; } } rcu_read_unlock(); return ret; - -unlock_retry: - rcu_read_unlock(); - goto retry; } EXPORT_SYMBOL_GPL(reservation_object_test_signaled_rcu); -- cgit v1.2.3 From 30cd85dd6edc86ea8d8589efb813f1fad41ef233 Mon Sep 17 00:00:00 2001 From: Gustavo Padovan Date: Wed, 19 Oct 2016 15:48:32 -0200 Subject: dma-buf/sync_file: hold reference to fence when creating sync_file fence referencing was out of balance. It was not taking any ref to the fence at creating time, but it was putting a reference when freeing the sync file. This patch fixes the balancing issue by getting a reference for the fence when creating the sync_file. Signed-off-by: Gustavo Padovan Signed-off-by: Sean Paul Link: http://patchwork.freedesktop.org/patch/msgid/1476899313-22241-1-git-send-email-gustavo@padovan.org --- drivers/dma-buf/sync_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/dma-buf') diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index b29a9e817320..235f8ac113cc 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -79,7 +79,7 @@ struct sync_file *sync_file_create(struct fence *fence) if (!sync_file) return NULL; - sync_file->fence = fence; + sync_file->fence = fence_get(fence); snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d", fence->ops->get_driver_name(fence), -- cgit v1.2.3