dma_fence's lock already guards the signaled-state, and most fence
backend_ops callbacks are already called with the lock taken.
The contract already relies on the fact that a producer must signal all
fences before unloading. The callbacks then must never run on a fence
that is already signaled.
Currently, driver unload is supported by RCU-checking the fence->ops
pointer. However, the design ended up with two partially redundant
mechanisms, the other being the signaled-state:
/*
* Make load ordering irrelevant by checking both signaled state and ops
* pointer and ops pointer is only set to NULL on newer implementations.
*/
if (!dma_fence_test_signaled_flag(fence) && ops)
We have seen multiple bugs, such as ordering issues, because of that,
for instance in:
035219a760ed ("dma-buf: dma-fence: Fix potential NULL pointer dereference")
3ed11c671ff7 ("dma-buf/dma-fence: fix checking signaling bit for timeline and
driver name v3")
Additionally, our RCU requirements can by definition not be controled by
dma_fence, resulting in a burden on users and an unnecessary grace
period delay. Most recently, this came to light in CRTC:
https://lore.kernel.org/dri-devel/[email protected]/
It appears to be the natural solution to guard the fence's signaled
state *and* accesses to the driver and its data through the lock.
However, in the past it has been argued that one cannot hold the
fence-lock while running a callback, because this might lead to
deadlocks, since drivers might want to access data protected by a driver
lock.
De-facto, it is already the case that ops callbacks are called with the
lock taken, and don't run into the driver if the fence is signaled
already. Thus, we merely need to make that rule official.
This commit proposes making it standard to guard against UAF and module
unload by holding the lock while running fence->ops.
The current situation of the ops is like this:
- release() and wait() are deprecated and can never work with module
unload in any case, thus neither RCU nor lock help them anyways.
- enable_signaling() and is_signaled() are already being invoked with the
lock, thus continue doing so is fine.
- get_driver_name() and get_timeline_name() can elegantly be solved by
copying the name-strings. See subsequent patch.
- set_deadline() is the only callback for which a locked call would be
new. It has very few implementors, so correctness is easy to verify.
For dma_fence_is_signaled(), dma_fence_is_signaled_locked()'s existence
proofs that we can lock the former's ops->signaled() call, too.
The fact that most callbacks are already invoked with the lock taken
proofs that the drivers support it, and doing it consistently will make
the entire API far more robust and consistent than RCU-guarding.
Current users keep working, and future users can be well-informed about
the locking considerations.
Let's go for full, consistent locking. Add lock guards around the two
missing paths running into non-deprecated backend_ops.
Signed-off-by: Philipp Stanner <[email protected]>
---
drivers/dma-buf/dma-fence.c | 7 ++++++-
include/linux/dma-fence.h | 21 +++++++++++++++++----
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 5dab647e3f3f..e4350bce9cf9 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -997,9 +997,14 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
{
const struct dma_fence_ops *ops = fence->ops;
+ unsigned long flags;
- if (ops->set_deadline && !dma_fence_is_signaled(fence))
+ dma_fence_lock_irqsave(fence, flags);
+
+ if (ops->set_deadline && !dma_fence_is_signaled_locked(fence))
ops->set_deadline(fence, deadline);
+
+ dma_fence_unlock_irqrestore(fence, flags);
}
EXPORT_SYMBOL(dma_fence_set_deadline);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 874848a4aec4..2223d75c5a49 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -549,16 +549,29 @@ static inline bool
dma_fence_is_signaled(struct dma_fence *fence)
{
const struct dma_fence_ops *ops = fence->ops;
+ unsigned long flags;
+ bool ret = false;
+ /* Fast-path optimization. */
if (dma_fence_test_signaled_flag(fence))
return true;
- if (ops && ops->signaled && ops->signaled(fence)) {
- dma_fence_signal(fence);
- return true;
+ dma_fence_lock_irqsave(fence, flags);
+
+ if (dma_fence_test_signaled_flag(fence)) {
+ ret = true;
+ goto out;
}
- return false;
+ if (ops->signaled && ops->signaled(fence)) {
+ ret = true;
+ dma_fence_signal_locked(fence);
+ }
+
+out:
+ dma_fence_unlock_irqrestore(fence, flags);
+
+ return ret;
}
/**
--
2.56.0
