[PATCH] drm/drm_crtc: fix race with dma_fence_signal() in ::get_driver_name()

[PATCH] drm/drm_crtc: fix race with dma_fence_signal() in ::get_driver_name()

[André Draszik]

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.

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>
---
 drivers/gpu/drm/drm_crtc.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 63ead8ba6756..31c8636e7467 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -73,6 +73,9 @@
  * &drm_mode_config_funcs.atomic_check.
  */
 
+#define fence_to_crtc(f) container_of((f)->extern_lock, \
+				      struct drm_crtc, fence_lock)
+
 /**
  * drm_crtc_from_index - find the registered CRTC at an index
  * @dev: DRM device
@@ -154,14 +157,6 @@ 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);
-}
-
 static const char *drm_crtc_fence_get_driver_name(struct dma_fence *fence)
 {
 	struct drm_crtc *crtc = fence_to_crtc(fence);

---
base-commit: e2cae00c05d196491c318196792297f2dfbaa02c
change-id: 20260618-linux-drm_crtc_fix2-23a7c354a412

Best regards,
-- 
André Draszik <andre.draszik@linaro.org>

Re: [PATCH] drm/drm_crtc: fix race with dma_fence_signal() in ::get_driver_name()

[Jani Nikula]

On Thu, 18 Jun 2026, André Draszik <andre.draszik@linaro.org> 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.
>
> 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>
> ---
>  drivers/gpu/drm/drm_crtc.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 63ead8ba6756..31c8636e7467 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -73,6 +73,9 @@
>   * &drm_mode_config_funcs.atomic_check.
>   */
>  
> +#define fence_to_crtc(f) container_of((f)->extern_lock, \
> +				      struct drm_crtc, fence_lock)
> +
>  /**
>   * drm_crtc_from_index - find the registered CRTC at an index
>   * @dev: DRM device
> @@ -154,14 +157,6 @@ 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);

Whether removing the BUG_ON() turns out to be the right choice or not, I
couldn't say, but please don't turn this function into a macro, at least
not without rationale. (I can't think of any.)

BR,
Jani.

> -	return container_of(fence->extern_lock, struct drm_crtc, fence_lock);
> -}
> -
>  static const char *drm_crtc_fence_get_driver_name(struct dma_fence *fence)
>  {
>  	struct drm_crtc *crtc = fence_to_crtc(fence);
>
> ---
> base-commit: e2cae00c05d196491c318196792297f2dfbaa02c
> change-id: 20260618-linux-drm_crtc_fix2-23a7c354a412
>
> Best regards,

-- 
Jani Nikula, Intel

Re: [PATCH] drm/drm_crtc: fix race with dma_fence_signal() in ::get_driver_name()

[Philipp Stanner]

+Cc Danilo

On Thu, 2026-06-18 at 15:03 +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)

Now this looks like a very similar problem that I have recently been
concerned with:

https://lore.kernel.org/dri-devel/20260612104251.2264707-2-phasta@kernel.org/

https://lore.kernel.org/dri-devel/fa0dc9757bf8343516c4b156a2b70ec91b64ef8f.camel@mailbox.org/


I continue to believe because of bugs like this and the ones I have
quoted in the threads above the robustness of the kernel could be
greatly improved if we could get dma_fence fully synchronized with its
lock.


That said:

> +++ 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.
> 
> 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>
> ---
>  drivers/gpu/drm/drm_crtc.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 63ead8ba6756..31c8636e7467 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -73,6 +73,9 @@
>   * &drm_mode_config_funcs.atomic_check.
>   */
>  
> +#define fence_to_crtc(f) container_of((f)->extern_lock, \
> +				      struct drm_crtc, fence_lock)

I agree that macros should be avoided if possible.

> +
>  /**
>   * drm_crtc_from_index - find the registered CRTC at an index
>   * @dev: DRM device
> @@ -154,14 +157,6 @@ 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);

+1

BUG_ON is more or less deprecated and should not be used anymore. There
needs to be bombastic justification for shooting down the entire
kernel.


P.

> -	return container_of(fence->extern_lock, struct drm_crtc, fence_lock);
> -}
> -
>  static const char *drm_crtc_fence_get_driver_name(struct dma_fence *fence)
>  {
>  	struct drm_crtc *crtc = fence_to_crtc(fence);
> 
> ---
> base-commit: e2cae00c05d196491c318196792297f2dfbaa02c
> change-id: 20260618-linux-drm_crtc_fix2-23a7c354a412
> 
> Best regards,

Re: [PATCH] drm/drm_crtc: fix race with dma_fence_signal() in ::get_driver_name()

[André Draszik]

Hi,

On Thu, 2026-06-18 at 17:56 +0200, Philipp Stanner wrote:
> +Cc Danilo
> 
> On Thu, 2026-06-18 at 15:03 +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)
> 
> Now this looks like a very similar problem that I have recently been
> concerned with:
> 
> https://lore.kernel.org/dri-devel/20260612104251.2264707-2-phasta@kernel.org/
> 
> https://lore.kernel.org/dri-devel/fa0dc9757bf8343516c4b156a2b70ec91b64ef8f.camel@mailbox.org/
> 
> 
> I continue to believe because of bugs like this and the ones I have
> quoted in the threads above the robustness of the kernel could be
> greatly improved if we could get dma_fence fully synchronized with its
> lock.

On top of that, sashiko highlighted  (via my other patch) that the existing
code is missing some memory barriers:

https://sashiko.dev/#/patchset/20260618-linux-drm_crtc_fix-v1-1-801f29c9853d@linaro.org?part=1

I believe Lock synchronization would resolve that (as would adding explicit
memory barriers).

[...]

> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 63ead8ba6756..31c8636e7467 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -73,6 +73,9 @@
> >   * &drm_mode_config_funcs.atomic_check.
> >   */
> >  
> > +#define fence_to_crtc(f) container_of((f)->extern_lock, \
> > +				      struct drm_crtc, fence_lock)
> 
> I agree that macros should be avoided if possible.

No problem, I'll change that.

> 
> > +
> >  /**
> >   * drm_crtc_from_index - find the registered CRTC at an index
> >   * @dev: DRM device
> > @@ -154,14 +157,6 @@ 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);
> 
> +1
> 
> BUG_ON is more or less deprecated and should not be used anymore. There
> needs to be bombastic justification for shooting down the entire
> kernel.

Yes, I meant to mention that in my commit message as well.

Now, you might have seen that sashiko highlighted that the existing
BUG_ON() is masking a potential use-after-free during driver removal which
I believe to be a correct observation. It suggests two alternative ways to
resolve it:

https://sashiko.dev/#/patchset/20260618-linux-drm_crtc_fix2-v1-1-c03e77b36f34@linaro.org?part=1

> 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?

Do you guys have any preference on that? It appears the use-after-free
should be resolved before merging the removal of the BUG_ON(), and I'd like
to progress on this.

Cheers,
Andre'

Re: [PATCH] drm/drm_crtc: fix race with dma_fence_signal() in ::get_driver_name()

[Philipp Stanner]

On Tue, 2026-06-23 at 12:37 +0100, André Draszik wrote:
> Hi,
> 
> On Thu, 2026-06-18 at 17:56 +0200, Philipp Stanner wrote:
> > +Cc Danilo
> > 
> > On Thu, 2026-06-18 at 15:03 +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)
> > 
> > Now this looks like a very similar problem that I have recently been
> > concerned with:
> > 
> > https://lore.kernel.org/dri-devel/20260612104251.2264707-2-phasta@kernel.org/
> > 
> > https://lore.kernel.org/dri-devel/fa0dc9757bf8343516c4b156a2b70ec91b64ef8f.camel@mailbox.org/
> > 
> > 
> > I continue to believe because of bugs like this and the ones I have
> > quoted in the threads above the robustness of the kernel could be
> > greatly improved if we could get dma_fence fully synchronized with its
> > lock.
> 
> On top of that, sashiko highlighted  (via my other patch) that the existing
> code is missing some memory barriers:
> 
> https://sashiko.dev/#/patchset/20260618-linux-drm_crtc_fix-v1-1-801f29c9853d@linaro.org?part=1
> 
> I believe Lock synchronization would resolve that (as would adding explicit
> memory barriers).

That is being discussed in the thread I linked, where Gary lists which
barriers you would need for (presumably correct) lockless magic.

However, if my issue were to be solved with barriers, the
test_and_set_bit() in dma_fence_signal_timestamp_locked() would have to
be replaced with the more weakly ordered test_bit() and set_bit(),
maybe creating other pitfalls.

The ordering issue in the get_*_name() functions plays into that.
Setting the bit would then be done after setting the ops-pointer to
NULL. So one would have to try to move the NULL set, too.

Long story short, this is painful and subtle.

But I think what we are realizing over and over again is that dma_fence
has many subtleties to its API contract, and the implementation's
sparring use of spinlocks leads to workarounds where people take locks
manually or have to do an RCU dance.

Note that Christian is strongly opposed to guarding everything with
locks, in part for supposedly occuring deadlocks in the fence callbacks
when the driver needs to take its own locks.

The community discussion regarding that problem is currently in some
sort of dead end, where none of us seems to know what the correct path
forward is.

drm_sched users (and future users in Rust) use intermediate fences
which decouple e.g. userspace from the actual hardware fences. So one
path forward might be to question the callbacks in general and think
about some sort of replacement for them.

> 
> [...]
> > > 
> 
> > 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?
> 
> Do you guys have any preference on that? It appears the use-after-free
> should be resolved before merging the removal of the BUG_ON(), and I'd like
> to progress on this.

My understanding of the current situation is that as an issuer of
dma_fence's you, in general, should wait for a grace period until you
perform operations like driver unload, or, more generally, have fence-
related resources and such being accessed through callbacks go away.

Danilo has recently mentioned some life-time inconsistencies between
wider kernel device model and DRM device model that might be related to
that discussion, and which made him object against some RCU
requirements.

Maybe he's got the time to share some details with you that are
relevant to your work.


P.

> 
> Cheers,
> Andre'

Re: [PATCH] drm/drm_crtc: fix race with dma_fence_signal() in ::get_driver_name()

[André Draszik]

Hi Philipp,

On Tue, 2026-06-23 at 13:58 +0200, Philipp Stanner wrote:
> On Tue, 2026-06-23 at 12:37 +0100, André Draszik wrote:
> > On Thu, 2026-06-18 at 17:56 +0200, Philipp Stanner wrote:
> > > 
> > > I continue to believe because of bugs like this and the ones I have
> > > quoted in the threads above the robustness of the kernel could be
> > > greatly improved if we could get dma_fence fully synchronized with its
> > > lock.
> > 
> > On top of that, sashiko highlighted  (via my other patch) that the existing
> > code is missing some memory barriers:
> > 
> > https://sashiko.dev/#/patchset/20260618-linux-drm_crtc_fix-v1-1-801f29c9853d@linaro.org?part=1
> > 
> > I believe Lock synchronization would resolve that (as would adding explicit
> > memory barriers).
> 
> That is being discussed in the thread I linked, where Gary lists which
> barriers you would need for (presumably correct) lockless magic.

Having read Gary's suggestion, that aligns with what I had in mind.

> However, if my issue were to be solved with barriers, the
> test_and_set_bit() in dma_fence_signal_timestamp_locked() would have to
> be replaced with the more weakly ordered test_bit() and set_bit(),
> maybe creating other pitfalls.

For the avoidance of doubts, I'm not saying that all the issues you raised
can be solved by barriers instead of appropriate locks (I don't know enough
about the code and issues in general here).

I do think however that appropriate locks will fix the ordering issue
highlighted by sashiko (i.e. +1 for your argument). Barriers would fix this
specific issue, too, but that is not a statement about any wider issues. 

> The ordering issue in the get_*_name() functions plays into that.
> Setting the bit would then be done after setting the ops-pointer to
> NULL. So one would have to try to move the NULL set, too.
> 
> Long story short, this is painful and subtle.
> 
> But I think what we are realizing over and over again is that dma_fence
> has many subtleties to its API contract, and the implementation's
> sparring use of spinlocks leads to workarounds where people take locks
> manually or have to do an RCU dance.
> 
> Note that Christian is strongly opposed to guarding everything with
> locks, in part for supposedly occuring deadlocks in the fence callbacks
> when the driver needs to take its own locks.

ww_mutex could help against deadlocks, but might affect performance, in case
these are all critical code paths (IDK),

> The community discussion regarding that problem is currently in some
> sort of dead end, where none of us seems to know what the correct path
> forward is.

Please ignore if the following doesn't make sense, I'm just a bystander :-)
How about at least adding the required barriers and related changes, and
taking it from there? This would solve some immediate and easy to hit
issues on Arm64? If they turn out to be insufficient, code can still
be changed.



> > > 
> [...]
> My understanding of the current situation is that as an issuer of
> dma_fence's you, in general, should wait for a grace period until you
> perform operations like driver unload, or, more generally, have fence-
> related resources and such being accessed through callbacks go away.

If I understand correctly, simply waiting for a grace period in the
driver's unbind should be the way to go.


> Danilo ... Maybe he's got the time to share some details with you that are
> relevant to your work.

Will wait a little :-)



BTW, thanks Philipp for all these details, much appreciated.

Cheers,
A.

Re: [PATCH] drm/drm_crtc: fix race with dma_fence_signal() in ::get_driver_name()

[Philipp Stanner]

On Tue, 2026-06-23 at 15:33 +0100, André Draszik wrote:
> > However, if my issue were to be solved with barriers, the
> > test_and_set_bit() in dma_fence_signal_timestamp_locked() would have to
> > be replaced with the more weakly ordered test_bit() and set_bit(),
> > maybe creating other pitfalls.
> 
> For the avoidance of doubts, I'm not saying that all the issues you raised
> can be solved by barriers instead of appropriate locks (I don't know enough
> about the code and issues in general here).

I'm not saying that you're saying that. I'm just cautioning you that
this change could be tricky.

> 
> I do think however that appropriate locks will fix the ordering issue
> highlighted by sashiko (i.e. +1 for your argument). Barriers would fix this
> specific issue, too, but that is not a statement about any wider issues. 
> 
> > The ordering issue in the get_*_name() functions plays into that.
> > Setting the bit would then be done after setting the ops-pointer to
> > NULL. So one would have to try to move the NULL set, too.
> > 
> > Long story short, this is painful and subtle.
> > 
> > But I think what we are realizing over and over again is that dma_fence
> > has many subtleties to its API contract, and the implementation's
> > sparring use of spinlocks leads to workarounds where people take locks
> > manually or have to do an RCU dance.
> > 
> > Note that Christian is strongly opposed to guarding everything with
> > locks, in part for supposedly occuring deadlocks in the fence callbacks
> > when the driver needs to take its own locks.
> 
> ww_mutex could help against deadlocks, but might affect performance, in case
> these are all critical code paths (IDK),

You can't use sleepable locks in fences. They fire in interrupt context
left and right ;)

Despite, that wouldn't even solve the reported problem.

The tl;dr is:

there is fence_ops->enable_signaling(), which is currently being called
with the fence lock held. So the driver, in that callback, cannot take
a driver-specific lock IF there is another driver party (like an IRQ)
taking first the driver lock and then the fence lock.

Which is why Christian König wants to remove the fence lock being held
in enable_signaling().

One reason why that, supposedly, is currently not a problem is that
without fence->inline_lock, you can protect the fctx with the same lock
and do fctx list manipulations in enable_signaling() with lock
protection.


If you have a big bowl of popcorn available, you could checkout this
thread:

https://lore.kernel.org/dri-devel/20260608142436.265820-2-phasta@kernel.org/

;p

My own thinking is:
If everyone used inline_lock, and if we could rely on everyone being
able to do the necessary work in enable_signaling() without said lock-
inversion, then we could perfectly synchronize all actions related to
dma_fence, including driver and, thus, fence_ops unload.

The only thing blocking really might be enable_signaling (the other
callbacks already take the lock). The more difficult question would be
how to implement that in a backwards compatible manner, i.e., for those
who don't have inline_lock.

Another idea for the distant future might be to question the existence
of those callbacks. Userspace often is sort of decoupled from the
hardware fences through intermediate fences already.

> 
> > The community discussion regarding that problem is currently in some
> > sort of dead end, where none of us seems to know what the correct path
> > forward is.
> 
> Please ignore if the following doesn't make sense, I'm just a bystander :-)
> How about at least adding the required barriers and related changes, and
> taking it from there? This would solve some immediate and easy to hit
> issues on Arm64? If they turn out to be insufficient, code can still
> be changed.
> 

I am in support of that, which is why I posted that RFC for feedback
about the appropriate memory barriers.

> 
> BTW, thanks Philipp for all these details, much appreciated.

You're welcome. If you'd find a clever solution, probably everyone
would be happy.


P.

> 
> Cheers,
> A.