Hi,
These patches fix two issues in the drm/drm_crtc driver. Initially I was hitting the BUG_ON() in a scenario as explained in the commit message of what is now the second patch in this series. For the reasons outlines there, the BUG_ON() should just be removed.
After posting, sashiko.dev noticed another issue, that was previously masked by the now-removed BUG_ON(). Since we can't have a loud BUG() be replaced with silent data corruption or worse, I've also added a patch to address this issue highlighted by sashiko.dev. I believe its observation and analysis to be correct.
Cheers, Andre'
Signed-off-by: André Draszik andre.draszik@linaro.org --- Changes in v2: - add new patch 1 to address sashiko observation - original patch 1 becomes patch 2 - patch 2: - don't turn fence_to_crtc() into macro (Jani, Philipp) - update commit message to include reference to deprecated use of BUG - Link to v1: https://lore.kernel.org/r/20260618-linux-drm_crtc_fix2-v1-1-c03e77b36f34@lin...
--- André Draszik (2): drm/drm_crtc: ensure dma_fence_ops remain valid during device unbind drm/drm_crtc: fix race with dma_fence_signal() in ::get_driver_name()
drivers/gpu/drm/drm_crtc.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) --- base-commit: b9810cd75b9fb56a3425d391cba3f608502bd474 change-id: 20260618-linux-drm_crtc_fix2-23a7c354a412
Best regards,
In [1], sashiko reported the following issue:
=== snip === Looking at how these fences are managed, drm_crtc_create_fence() creates a dma_fence without taking a reference to the drm_device or drm_crtc. Because the sync_file framework exposes this fence to userspace, the fence can outlive the CRTC.
The dma_fence contract requires that data accessed by dma_fence_ops (like get_driver_name) must remain valid for an RCU grace period after the fence is signaled. However, drm_crtc_cleanup() and the subsequent freeing of the device do not wait for an RCU grace period via synchronize_rcu().
If userspace calls ioctl(SYNC_IOC_FILE_INFO) concurrently with a device hot-unplug:
CPU1 (Userspace) sync_file_get_name() ops = rcu_dereference(fence->ops); if (!dma_fence_test_signaled_flag()) // Preempted or delayed here
CPU2 (Driver Teardown) Signals the fence (setting fence->ops = NULL) Destroys and frees the CRTC without waiting for an RCU grace period
CPU1 (Resumes) ops->get_driver_name(fence) -> drm_crtc_fence_get_driver_name() crtc = fence_to_crtc(fence); // Casts to the freed CRTC return crtc->dev->driver->name; // Use-after-free
...
Does the CRTC or DRM device need to be kept alive for the RCU grace period, or should the fence hold a proper reference to prevent the use-after-free when get_driver_name() and get_timeline_name() access the freed CRTC structure? === snap ===
I believe this to be a correct observation and this patch implements the suggestion of waiting for an RCU grace period before proceeding with destruction of the drm_crtc, so that get_driver_name() and get_timeline_name() can still work.
Link: https://sashiko.dev/#/patchset/20260618-linux-drm_crtc_fix2-v1-1-c03e77b36f3... Signed-off-by: André Draszik andre.draszik@linaro.org --- drivers/gpu/drm/drm_crtc.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 63ead8ba6756..d55f1377ec36 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -501,6 +501,12 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev;
+ /* Ensure our dma_fence_ops remain valid for an RCU grace period after + * the fence is signaled. This is necessary because our dma_fence_ops + * dereference crtc->dev. + */ + synchronize_rcu(); + /* Note that the crtc_list is considered to be static; should we * remove the drm_crtc at runtime we would have to decrement all * the indices on the drm_crtc after us in the crtc_list.
+Cc Danilo (who is currently concerned with drm_device life times)
On Wed, 2026-07-08 at 16:22 +0100, André Draszik wrote:
[…]
Link: https://sashiko.dev/#/patchset/20260618-linux-drm_crtc_fix2-v1-1-c03e77b36f3... Signed-off-by: André Draszik andre.draszik@linaro.org
I am tempted to think that this also needs a Fixes and needs to be backported into stable kernels, doesn't it? Especially if the BUG_ON disappears in stable kernels.
drivers/gpu/drm/drm_crtc.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 63ead8ba6756..d55f1377ec36 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -501,6 +501,12 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev;
- /* Ensure our dma_fence_ops remain valid for an RCU grace period after
* the fence is signaled. This is necessary because our dma_fence_ops* dereference crtc->dev.*/- synchronize_rcu();
nit: I guess this is the only place where one can reasonably put the synchronize_rcu(). But I would hint at the RCU delay in the function's docu.
/* Note that the crtc_list is considered to be static; should we * remove the drm_crtc at runtime we would have to decrement all * the indices on the drm_crtc after us in the crtc_list.
Hi Philipp,
On Thu, 2026-07-09 at 14:32 +0200, Philipp Stanner wrote:
+Cc Danilo (who is currently concerned with drm_device life times)
On Wed, 2026-07-08 at 16:22 +0100, André Draszik wrote:
[…]
Link: https://sashiko.dev/#/patchset/20260618-linux-drm_crtc_fix2-v1-1-c03e77b36f3... Signed-off-by: André Draszik andre.draszik@linaro.org
I am tempted to think that this also needs a Fixes and needs to be backported into stable kernels, doesn't it? Especially if the BUG_ON disappears in stable kernels.
Good point, thanks. I forgot to add this in and will try to find a reasonable commit to relate to.
drivers/gpu/drm/drm_crtc.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 63ead8ba6756..d55f1377ec36 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -501,6 +501,12 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev;
- /* Ensure our dma_fence_ops remain valid for an RCU grace period after
* the fence is signaled. This is necessary because our dma_fence_ops* dereference crtc->dev.*/- synchronize_rcu();
nit: I guess this is the only place where one can reasonably put the synchronize_rcu(). But I would hint at the RCU delay in the function's docu.
Unfortunately, this still looks like an incomplete fix - https://sashiko.dev/#/patchset/20260618-linux-drm_crtc_fix2-v1-1-c03e77b36f3...
My next version will simply copy the relevant strings into a custom
struct drm_crtc_fence { struct dma_fence base; char driver_name[32]; char timeline_name[32]; };
or similar as part of drm_crtc_create_fence() and just use those as part of the dma_fence_ops. That approach should avoid all race conditions and corner cases with RCU.
I'll also make sure to update relevant documentation.
Cheers, Andre'
On Thu, 2026-07-09 at 15:19 +0100, André Draszik wrote:
Unfortunately, this still looks like an incomplete fix - https://sashiko.dev/#/patchset/20260618-linux-drm_crtc_fix2-v1-1-c03e77b36f3...
Wrong link above, https://sashiko.dev/#/patchset/20260708-linux-drm_crtc_fix2-v2-0-cf72be75d75... is the correct one.
A.
On Thu, 2026-07-09 at 15:19 +0100, André Draszik wrote:
Hi Philipp,
On Thu, 2026-07-09 at 14:32 +0200, Philipp Stanner wrote:
+Cc Danilo (who is currently concerned with drm_device life times)
On Wed, 2026-07-08 at 16:22 +0100, André Draszik wrote:
[…]
Link: https://sashiko.dev/#/patchset/20260618-linux-drm_crtc_fix2-v1-1-c03e77b36f3... Signed-off-by: André Draszik andre.draszik@linaro.org
I am tempted to think that this also needs a Fixes and needs to be backported into stable kernels, doesn't it? Especially if the BUG_ON disappears in stable kernels.
Good point, thanks. I forgot to add this in and will try to find a reasonable commit to relate to.
drivers/gpu/drm/drm_crtc.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 63ead8ba6756..d55f1377ec36 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -501,6 +501,12 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev;
- /* Ensure our dma_fence_ops remain valid for an RCU grace period after
* the fence is signaled. This is necessary because our dma_fence_ops* dereference crtc->dev.*/- synchronize_rcu();
nit: I guess this is the only place where one can reasonably put the synchronize_rcu(). But I would hint at the RCU delay in the function's docu.
Unfortunately, this still looks like an incomplete fix - https://sashiko.dev/#/patchset/20260618-linux-drm_crtc_fix2-v1-1-c03e77b36f3...
My next version will simply copy the relevant strings into a custom
struct drm_crtc_fence { struct dma_fence base; char driver_name[32]; char timeline_name[32]; };
or similar as part of drm_crtc_create_fence() and just use those as part of the dma_fence_ops. That approach should avoid all race conditions and corner cases with RCU.
Now wait a second. I don't see how your struct solves any issue that is not already solved.
static const char *drm_crtc_fence_get_driver_name(struct dma_fence *fence) { struct drm_crtc *crtc = fence_to_crtc(fence);
return crtc->dev->driver->name; }
static const char *drm_crtc_fence_get_timeline_name(struct dma_fence *fence) { struct drm_crtc *crtc = fence_to_crtc(fence);
return crtc->timeline_name; }
The issue here seems to be that a) the crtc is made invalid in drm_crtc_cleanup() (memset(0)) b) the drm_dev can disappear after drm_crtc_cleanup()
Both issues stem from the fact that the fence callbacks can keep running into the driver.
It is true that the fence, being refcounted, can stay alive, but none of the callbacks be invoked anymore, and your grace period wait fullfill.
It is a strict dma_fence requirement that a fence issuer / producer signals all its fences before unload.
The embedded spinlock issue is a separate problem. I think that should not stall your work here and can be addressed in a separate patch.
Note that the embedded spinlock issue is a known one, and it is very much related to the fence-decoupling work related to the ops pointer that Christian has been carrying out. So it can be expected to be a problem in wide parts of DRM.
Regards P.
Since commit 541c8f2468b9 ("dma-buf: detach fence ops on signal v3"), I'm seeing the BUG_ON() triggering in drm_crtc's fence_to_crtc() via drm_crtc_fence_get_driver_name() regularly:
Call trace: panic+0x58/0x5c die+0x160/0x178 bug_brk_handler+0x70/0xa4 call_el1_break_hook+0x3c/0x1a0 do_el1_brk64+0x24/0x74 el1_brk64+0x34/0x54 el1h_64_sync_handler+0x80/0xfc el1h_64_sync+0x84/0x88 drm_crtc_fence_get_driver_name+0x60/0x68 (P) sync_file_get_name+0x184/0x45c sync_file_ioctl+0x404/0xf70 __arm64_sys_ioctl+0x124/0x1dc
This looks to be caused by a code flow similar to the following:
+++ snip +++ thread A thread B
ioctl(SYNC_IOC_FILE_INFO) sync_file_ioctl() sync_file_get_name() dma_fence_signal_timestamp_locked() dma_fence_driver_name() ops = rcu_dereference(fence->ops) if (!dma_fence_test_signaled_flag()) ops->get_driver_name(fence) i.e. drm_crtc_fence_get_driver_name() test_and_set_bit(SIGNALED) RCU_INIT_POINTER(fence->ops, NULL) drm_crtc_fence_get_driver_name() BUG_ON(rcu_access_pointer(fence->ops) != &drm_crtc_fence_ops) +++ snap +++
I see two ways to resolve this: a) simply drop the BUG_ON(). It can not work anymore since above commit, as it is racy now. b) pass the original 'ops' pointer obtained in dma_fence_driver_name() to all callees.
This patch implements option a), as because: * I don't see much benefit in passing the extra pointer just for this BUG_ON() to work. * Requiring the dma_fence_ops in those callbacks is an implementation detail of the drm_crtc driver, and therefore upper layers shouldn't have to care about that. * The existence of the BUG_ON() doesn't appear to be consistent with implementations of ::get_driver_name() or ::get_timeline_name() in the majority of other DRM drivers in the first place. Those that do have a similar BUG_ON() (i915, xe) probably also need an update similar to this patch here but I'm not in a position to test those. * Using BUG() and friends to take down the system is an unacceptable way to handle a failure as evidenced by many threads on LKML and also in the kernel coding style. Here, the check was presumably added for detecting when something passes an invalid pointer, but that does not happen - and if it could, gracefully handling that situation would be more appropriate.
Note that the adjacent drm_crtc_fence_get_timeline_name() has the same problem and is fixed by this patch as well.
Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3") Signed-off-by: André Draszik andre.draszik@linaro.org
--- v2: - don't turn fence_to_crtc() into macro - update commit message to include reference to unacceptable use of BUG --- drivers/gpu/drm/drm_crtc.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d55f1377ec36..36ae50ddf525 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -154,11 +154,8 @@ static void drm_crtc_crc_fini(struct drm_crtc *crtc) #endif }
-static const struct dma_fence_ops drm_crtc_fence_ops; - static struct drm_crtc *fence_to_crtc(struct dma_fence *fence) { - BUG_ON(rcu_access_pointer(fence->ops) != &drm_crtc_fence_ops); return container_of(fence->extern_lock, struct drm_crtc, fence_lock); }
On Wed, 2026-07-08 at 16:22 +0100, André Draszik wrote:
Since commit 541c8f2468b9 ("dma-buf: detach fence ops on signal v3"), I'm seeing the BUG_ON() triggering in drm_crtc's fence_to_crtc() via drm_crtc_fence_get_driver_name() regularly:
Call trace: panic+0x58/0x5c die+0x160/0x178 bug_brk_handler+0x70/0xa4 call_el1_break_hook+0x3c/0x1a0 do_el1_brk64+0x24/0x74 el1_brk64+0x34/0x54 el1h_64_sync_handler+0x80/0xfc el1h_64_sync+0x84/0x88 drm_crtc_fence_get_driver_name+0x60/0x68 (P) sync_file_get_name+0x184/0x45c sync_file_ioctl+0x404/0xf70 __arm64_sys_ioctl+0x124/0x1dc
This looks to be caused by a code flow similar to the following:
+++ snip +++ thread A thread B
ioctl(SYNC_IOC_FILE_INFO) sync_file_ioctl() sync_file_get_name() dma_fence_signal_timestamp_locked() dma_fence_driver_name() ops = rcu_dereference(fence->ops) if (!dma_fence_test_signaled_flag()) ops->get_driver_name(fence) i.e. drm_crtc_fence_get_driver_name() test_and_set_bit(SIGNALED) RCU_INIT_POINTER(fence->ops, NULL) drm_crtc_fence_get_driver_name() BUG_ON(rcu_access_pointer(fence->ops) != &drm_crtc_fence_ops) +++ snap +++
I see two ways to resolve this: a) simply drop the BUG_ON(). It can not work anymore since above commit, as it is racy now. b) pass the original 'ops' pointer obtained in dma_fence_driver_name() to all callees.
This patch implements option a), as because:
- I don't see much benefit in passing the extra pointer just for this
BUG_ON() to work.
- Requiring the dma_fence_ops in those callbacks is an implementation
detail of the drm_crtc driver, and therefore upper layers shouldn't have to care about that.
- The existence of the BUG_ON() doesn't appear to be consistent with
implementations of ::get_driver_name() or ::get_timeline_name() in the majority of other DRM drivers in the first place. Those that do have a similar BUG_ON() (i915, xe) probably also need an update similar to this patch here but I'm not in a position to test those.
- Using BUG() and friends to take down the system is an unacceptable
way to handle a failure as evidenced by many threads on LKML and also in the kernel coding style. Here, the check was presumably added for detecting when something passes an invalid pointer, but that does not happen - and if it could, gracefully handling that situation would be more appropriate.
I think it was there to detect wrong fences.
I think that entire usage is effectively invalid since Christian's rework.
Note that the adjacent drm_crtc_fence_get_timeline_name() has the same problem and is fixed by this patch as well.
nit: It's cool to see folks write detailed commit messages, but I think in this case it is very verbose. The presentation of the solution- options IMO is more something for a comment in the mailing list ;)
The issue at hand is rather simple, AFAICS. Fences now set the ops pointer to NULL on signal, which wasn't the case in the past, which triggers the BUG_ON, which shouldn't be used anymore anyways. That's the core of the issue.
The ops pointer never changed in the past. I suppose it was there to check whether a wrong fence was passed to the container_of().
+Cc stable?
Fixes: 541c8f2468b9 ("dma-buf: detach fence ops on signal v3") Signed-off-by: André Draszik andre.draszik@linaro.org
Reviewed-by: Philipp Stanner phasta@kernel.org
v2:
- don't turn fence_to_crtc() into macro
- update commit message to include reference to unacceptable use of BUG
drivers/gpu/drm/drm_crtc.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d55f1377ec36..36ae50ddf525 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -154,11 +154,8 @@ static void drm_crtc_crc_fini(struct drm_crtc *crtc) #endif } -static const struct dma_fence_ops drm_crtc_fence_ops;
static struct drm_crtc *fence_to_crtc(struct dma_fence *fence) {
- BUG_ON(rcu_access_pointer(fence->ops) != &drm_crtc_fence_ops);
return container_of(fence->extern_lock, struct drm_crtc, fence_lock); }
linaro-mm-sig@lists.linaro.org
-
André Draszik -
Philipp Stanner