mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 0/2] drm/syncobj: fix sync_file import into timeline points
@ 2026-10-10 18:07 Julian Orth
  2026-10-10 18:07 ` [PATCH 1/2] drm/syncobj: fix fence and syncobj leak in drm_syncobj_import_sync_file_fence Julian Orth
  2026-10-10 18:07 ` [PATCH 2/2] drm/syncobj: flatten chains when importing sync_file into timeline point Julian Orth
  0 siblings, 2 replies; 9+ messages in thread
From: Julian Orth @ 2026-10-10 18:07 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Sumit Semwal, Christian König,
	Dmitry Osipenko, Rob Clark
  Cc: dri-devel, linux-kernel, linux-media, linaro-mm-sig, Julian Orth

Importing a sync_file into a timeline point
(DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE |
DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE) has two bugs since
c2d3a7300695:

1. The fence and syncobj references leak if dma_fence_chain_alloc()
   fails.
2. A sync_file that contains a dma_fence_chain (e.g. one exported from a
   timeline point) is wrapped in another chain and triggers the
   WARN_ON in dma_fence_chain_init(). I reported this on 2026-05-20.

These patches were developed completely autonomously by AI. I have
reviewed them, but they have not been tested at runtime. They are only
compile-tested.

Julian Orth (2):
  drm/syncobj: fix fence and syncobj leak in
    drm_syncobj_import_sync_file_fence
  drm/syncobj: flatten chains when importing sync_file into timeline
    point

 drivers/gpu/drm/drm_syncobj.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)


base-commit: 6ccf996a0dec1852dab94ad865f27b4904750e12
-- 
2.56.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] drm/syncobj: fix fence and syncobj leak in drm_syncobj_import_sync_file_fence
  2026-10-10 18:07 [PATCH 0/2] drm/syncobj: fix sync_file import into timeline points Julian Orth
@ 2026-10-10 18:07 ` Julian Orth
  2026-10-12  7:45   ` Christian König
  2026-10-10 18:07 ` [PATCH 2/2] drm/syncobj: flatten chains when importing sync_file into timeline point Julian Orth
  1 sibling, 1 reply; 9+ messages in thread
From: Julian Orth @ 2026-10-10 18:07 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Sumit Semwal, Christian König,
	Dmitry Osipenko, Rob Clark
  Cc: dri-devel, linux-kernel, linux-media, linaro-mm-sig, Julian Orth

If dma_fence_chain_alloc() fails, the function returns -ENOMEM without
dropping the references to the imported fence and the syncobj.

Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Julian Orth <ju.orth@gmail.com>
---
 drivers/gpu/drm/drm_syncobj.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index c23a5de27..5ca5163d0 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -730,6 +730,7 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
 {
 	struct dma_fence *fence = sync_file_get_fence(fd);
 	struct drm_syncobj *syncobj;
+	int ret = 0;
 
 	if (!fence)
 		return -EINVAL;
@@ -743,17 +744,20 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
 	if (point) {
 		struct dma_fence_chain *chain = dma_fence_chain_alloc();
 
-		if (!chain)
-			return -ENOMEM;
+		if (!chain) {
+			ret = -ENOMEM;
+			goto out;
+		}
 
 		drm_syncobj_add_point(syncobj, chain, fence, point);
 	} else {
 		drm_syncobj_replace_fence(syncobj, fence);
 	}
 
+out:
 	dma_fence_put(fence);
 	drm_syncobj_put(syncobj);
-	return 0;
+	return ret;
 }
 
 static int drm_syncobj_export_sync_file(struct drm_file *file_private,
-- 
2.56.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] drm/syncobj: flatten chains when importing sync_file into timeline point
  2026-10-10 18:07 [PATCH 0/2] drm/syncobj: fix sync_file import into timeline points Julian Orth
  2026-10-10 18:07 ` [PATCH 1/2] drm/syncobj: fix fence and syncobj leak in drm_syncobj_import_sync_file_fence Julian Orth
@ 2026-10-10 18:07 ` Julian Orth
  2026-10-12  7:51   ` Christian König
  1 sibling, 1 reply; 9+ messages in thread
From: Julian Orth @ 2026-10-10 18:07 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Sumit Semwal, Christian König,
	Dmitry Osipenko, Rob Clark
  Cc: dri-devel, linux-kernel, linux-media, linaro-mm-sig, Julian Orth

A sync_file exported from a timeline point can contain a dma_fence_chain.
Importing such a sync_file into a timeline point with
DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE |
DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE passes the chain directly to
dma_fence_chain_init() and triggers

  WARNING: drivers/dma-buf/dma-fence-chain.c:286 at dma_fence_chain_init

because chain fences must not be wrapped in other chain fences.

Flatten the fence with dma_fence_unwrap_merge() before adding it to the
timeline, as drm_syncobj_transfer_to_timeline() already does.

Reproducer:

1. Export a sync_file from a point > 0 of a timeline syncobj with
   DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD.
2. Import it into a point > 0 of another timeline syncobj with
   DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE.

Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Julian Orth <ju.orth@gmail.com>
---
 drivers/gpu/drm/drm_syncobj.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 5ca5163d0..c545af77e 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -742,8 +742,18 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
 	}
 
 	if (point) {
-		struct dma_fence_chain *chain = dma_fence_chain_alloc();
+		struct dma_fence_chain *chain;
+		struct dma_fence *tmp;
+
+		tmp = dma_fence_unwrap_merge(fence);
+		dma_fence_put(fence);
+		fence = tmp;
+		if (!fence) {
+			drm_syncobj_put(syncobj);
+			return -ENOMEM;
+		}
 
+		chain = dma_fence_chain_alloc();
 		if (!chain) {
 			ret = -ENOMEM;
 			goto out;
-- 
2.56.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm/syncobj: fix fence and syncobj leak in drm_syncobj_import_sync_file_fence
  2026-10-10 18:07 ` [PATCH 1/2] drm/syncobj: fix fence and syncobj leak in drm_syncobj_import_sync_file_fence Julian Orth
@ 2026-10-12  7:45   ` Christian König
  2026-10-12 11:38     ` Julian Orth
  0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2026-10-12  7:45 UTC (permalink / raw)
  To: Julian Orth, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Sumit Semwal, Dmitry Osipenko,
	Rob Clark
  Cc: dri-devel, linux-kernel, linux-media, linaro-mm-sig



On 10/10/26 20:07, Julian Orth wrote:
> If dma_fence_chain_alloc() fails, the function returns -ENOMEM without
> dropping the references to the imported fence and the syncobj.
> 
> Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs")
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Julian Orth <ju.orth@gmail.com>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index c23a5de27..5ca5163d0 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -730,6 +730,7 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>  {
>  	struct dma_fence *fence = sync_file_get_fence(fd);
>  	struct drm_syncobj *syncobj;
> +	int ret = 0;

Please don't initialize return values on declaration that is often considered bad coding style.

>  
>  	if (!fence)
>  		return -EINVAL;
> @@ -743,17 +744,20 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>  	if (point) {

>  		struct dma_fence_chain *chain = dma_fence_chain_alloc();
>  
> -		if (!chain)
> -			return -ENOMEM;
> +		if (!chain) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
>  
>  		drm_syncobj_add_point(syncobj, chain, fence, point);

Allocation and adding the point is intentionally separated so that dma_fence_chain_alloc() can be called before looking up the syncobj or previous fence.

I suggest to make use of that.

Regards,
Christian.

>  	} else {
>  		drm_syncobj_replace_fence(syncobj, fence);
>  	}
>  
> +out:
>  	dma_fence_put(fence);
>  	drm_syncobj_put(syncobj);
> -	return 0;
> +	return ret;
>  }
>  
>  static int drm_syncobj_export_sync_file(struct drm_file *file_private,


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] drm/syncobj: flatten chains when importing sync_file into timeline point
  2026-10-10 18:07 ` [PATCH 2/2] drm/syncobj: flatten chains when importing sync_file into timeline point Julian Orth
@ 2026-10-12  7:51   ` Christian König
  2026-10-12 11:42     ` Julian Orth
  0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2026-10-12  7:51 UTC (permalink / raw)
  To: Julian Orth, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Sumit Semwal, Dmitry Osipenko,
	Rob Clark
  Cc: dri-devel, linux-kernel, linux-media, linaro-mm-sig

On 10/10/26 20:07, Julian Orth wrote:
> A sync_file exported from a timeline point can contain a dma_fence_chain.

No it can't, IIRC sync files are only supposed to contain raw fences or dma_fence_array containers.

I think we never documented that and that's why this issue here leaked in.

I'm not sure if that doesn't break anywhere else as well, so I think we should fix the exporting side instead.

Alternatively we need to audit everywhere where fences from syncfiles can be imported somewhere else.

Regards,
Christian.

> Importing such a sync_file into a timeline point with
> DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE |
> DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE passes the chain directly to
> dma_fence_chain_init() and triggers
> 
>   WARNING: drivers/dma-buf/dma-fence-chain.c:286 at dma_fence_chain_init
> 
> because chain fences must not be wrapped in other chain fences.
> 
> Flatten the fence with dma_fence_unwrap_merge() before adding it to the
> timeline, as drm_syncobj_transfer_to_timeline() already does.
> 
> Reproducer:
> 
> 1. Export a sync_file from a point > 0 of a timeline syncobj with
>    DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD.
> 2. Import it into a point > 0 of another timeline syncobj with
>    DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE.
> 
> Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs")
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Julian Orth <ju.orth@gmail.com>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 5ca5163d0..c545af77e 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -742,8 +742,18 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>  	}
>  
>  	if (point) {
> -		struct dma_fence_chain *chain = dma_fence_chain_alloc();
> +		struct dma_fence_chain *chain;
> +		struct dma_fence *tmp;
> +
> +		tmp = dma_fence_unwrap_merge(fence);
> +		dma_fence_put(fence);
> +		fence = tmp;
> +		if (!fence) {
> +			drm_syncobj_put(syncobj);
> +			return -ENOMEM;
> +		}
>  
> +		chain = dma_fence_chain_alloc();
>  		if (!chain) {
>  			ret = -ENOMEM;
>  			goto out;


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm/syncobj: fix fence and syncobj leak in drm_syncobj_import_sync_file_fence
  2026-10-12  7:45   ` Christian König
@ 2026-10-12 11:38     ` Julian Orth
  2026-10-12 12:56       ` Christian König
  0 siblings, 1 reply; 9+ messages in thread
From: Julian Orth @ 2026-10-12 11:38 UTC (permalink / raw)
  To: Christian König
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Sumit Semwal, Dmitry Osipenko,
	Rob Clark, dri-devel, linux-kernel, linux-media, linaro-mm-sig

On Mon, Oct 12, 2026 at 9:45 AM Christian König
<christian.koenig@amd.com> wrote:
>
>
>
> On 10/10/26 20:07, Julian Orth wrote:
> > If dma_fence_chain_alloc() fails, the function returns -ENOMEM without
> > dropping the references to the imported fence and the syncobj.
> >
> > Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs")
> > Assisted-by: Claude:claude-opus-5-5
> > Signed-off-by: Julian Orth <ju.orth@gmail.com>
> > ---
> >  drivers/gpu/drm/drm_syncobj.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> > index c23a5de27..5ca5163d0 100644
> > --- a/drivers/gpu/drm/drm_syncobj.c
> > +++ b/drivers/gpu/drm/drm_syncobj.c
> > @@ -730,6 +730,7 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
> >  {
> >       struct dma_fence *fence = sync_file_get_fence(fd);
> >       struct drm_syncobj *syncobj;
> > +     int ret = 0;
>
> Please don't initialize return values on declaration that is often considered bad coding style.
>
> >
> >       if (!fence)
> >               return -EINVAL;
> > @@ -743,17 +744,20 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
> >       if (point) {
>
> >               struct dma_fence_chain *chain = dma_fence_chain_alloc();
> >
> > -             if (!chain)
> > -                     return -ENOMEM;
> > +             if (!chain) {
> > +                     ret = -ENOMEM;
> > +                     goto out;
> > +             }
> >
> >               drm_syncobj_add_point(syncobj, chain, fence, point);
>
> Allocation and adding the point is intentionally separated so that dma_fence_chain_alloc() can be called before looking up the syncobj or previous fence.
>
> I suggest to make use of that.

This is the fourth time that someone has tried to fix this trivial bug
for over a year. The previous 3 cases were:

https://lore.kernel.org/all/c715ef30-9d91-46f3-8a0f-1039f3d25e7d@stanley.mountain/
https://lore.kernel.org/all/20260516-jorth-syncobj-v1-9-88ede9d98a81@gmail.com/
https://lore.kernel.org/all/20260525025455.3887523-1-vulab@iscas.ac.cn/

The last of which didn't even receive a reply. And there have been
more cases in the past where I encountered a dri-devel-adjacent issue,
googled it, and found that patches had already been sent in years ago
to fix it, but were ignored.

Which is all to explain why I had all of the work for this series done by AI.

I heard some chatter about dri-devel adopting a gitlab-based workflow,
and I hope that works out and I didn't misunderstand. Because the
current system is clearly broken. Nobody keeps track of which bugs
have been reported, nobody triages existing bugs, nobody keeps track
of attempts to fix bugs. Which is how 3 people ended up doing repeat
work of discovering, analyzing, and fixing the same bug.

The same pattern applies to my patch two. I clearly don't know enough
about the (probably) undocumented semantics of where and when which
structure of fences can be used. Which is why I sent in a bug report
about this issue in May instead of trying to fix it on my own:
https://lore.kernel.org/all/CAHijbEUKuXNeSnT82P-ojOhzU5siNHjxUW+qEDOqx07Ys9m=xw@mail.gmail.com/

I would have hoped that a bug report amounting to "this new feature
causes the kernel to spam dmesg at 60 fps" would get some attention.
But no. The only way to get this issue fixed seems to be me acting as
a meat proxy to fix an issue that I know very little about.

Thanks for reading my blog post.


>
> Regards,
> Christian.
>
> >       } else {
> >               drm_syncobj_replace_fence(syncobj, fence);
> >       }
> >
> > +out:
> >       dma_fence_put(fence);
> >       drm_syncobj_put(syncobj);
> > -     return 0;
> > +     return ret;
> >  }
> >
> >  static int drm_syncobj_export_sync_file(struct drm_file *file_private,
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] drm/syncobj: flatten chains when importing sync_file into timeline point
  2026-10-12  7:51   ` Christian König
@ 2026-10-12 11:42     ` Julian Orth
  2026-10-12 13:06       ` Christian König
  0 siblings, 1 reply; 9+ messages in thread
From: Julian Orth @ 2026-10-12 11:42 UTC (permalink / raw)
  To: Christian König
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Sumit Semwal, Dmitry Osipenko,
	Rob Clark, dri-devel, linux-kernel, linux-media, linaro-mm-sig

On Mon, Oct 12, 2026 at 9:51 AM Christian König
<christian.koenig@amd.com> wrote:
>
> On 10/10/26 20:07, Julian Orth wrote:
> > A sync_file exported from a timeline point can contain a dma_fence_chain.
>
> No it can't, IIRC sync files are only supposed to contain raw fences or dma_fence_array containers.
>
> I think we never documented that and that's why this issue here leaked in.

When I reviewed this patch before sending it in, I asked the AI if it
would not be better to do the merging at export time. It was adamant
that it had to be at import time because of commit 519f490d by
yourself:

    dma-buf/sync-file: fix warning about fence containers

    The dma_fence_chain containers can show up in sync_files as well

>
> I'm not sure if that doesn't break anywhere else as well, so I think we should fix the exporting side instead.
>
> Alternatively we need to audit everywhere where fences from syncfiles can be imported somewhere else.
>
> Regards,
> Christian.
>
> > Importing such a sync_file into a timeline point with
> > DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE |
> > DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE passes the chain directly to
> > dma_fence_chain_init() and triggers
> >
> >   WARNING: drivers/dma-buf/dma-fence-chain.c:286 at dma_fence_chain_init
> >
> > because chain fences must not be wrapped in other chain fences.
> >
> > Flatten the fence with dma_fence_unwrap_merge() before adding it to the
> > timeline, as drm_syncobj_transfer_to_timeline() already does.
> >
> > Reproducer:
> >
> > 1. Export a sync_file from a point > 0 of a timeline syncobj with
> >    DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD.
> > 2. Import it into a point > 0 of another timeline syncobj with
> >    DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE.
> >
> > Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs")
> > Assisted-by: Claude:claude-opus-5-5
> > Signed-off-by: Julian Orth <ju.orth@gmail.com>
> > ---
> >  drivers/gpu/drm/drm_syncobj.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> > index 5ca5163d0..c545af77e 100644
> > --- a/drivers/gpu/drm/drm_syncobj.c
> > +++ b/drivers/gpu/drm/drm_syncobj.c
> > @@ -742,8 +742,18 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
> >       }
> >
> >       if (point) {
> > -             struct dma_fence_chain *chain = dma_fence_chain_alloc();
> > +             struct dma_fence_chain *chain;
> > +             struct dma_fence *tmp;
> > +
> > +             tmp = dma_fence_unwrap_merge(fence);
> > +             dma_fence_put(fence);
> > +             fence = tmp;
> > +             if (!fence) {
> > +                     drm_syncobj_put(syncobj);
> > +                     return -ENOMEM;
> > +             }
> >
> > +             chain = dma_fence_chain_alloc();
> >               if (!chain) {
> >                       ret = -ENOMEM;
> >                       goto out;
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm/syncobj: fix fence and syncobj leak in drm_syncobj_import_sync_file_fence
  2026-10-12 11:38     ` Julian Orth
@ 2026-10-12 12:56       ` Christian König
  0 siblings, 0 replies; 9+ messages in thread
From: Christian König @ 2026-10-12 12:56 UTC (permalink / raw)
  To: Julian Orth
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Sumit Semwal, Dmitry Osipenko,
	Rob Clark, dri-devel, linux-kernel, linux-media, linaro-mm-sig

On 10/12/26 13:38, Julian Orth wrote:
> On Mon, Oct 12, 2026 at 9:45 AM Christian König
> <christian.koenig@amd.com> wrote:
>>
>>
>>
>> On 10/10/26 20:07, Julian Orth wrote:
>>> If dma_fence_chain_alloc() fails, the function returns -ENOMEM without
>>> dropping the references to the imported fence and the syncobj.
>>>
>>> Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs")
>>> Assisted-by: Claude:claude-opus-5-5
>>> Signed-off-by: Julian Orth <ju.orth@gmail.com>
>>> ---
>>>  drivers/gpu/drm/drm_syncobj.c | 10 +++++++---
>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>>> index c23a5de27..5ca5163d0 100644
>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>> @@ -730,6 +730,7 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>>>  {
>>>       struct dma_fence *fence = sync_file_get_fence(fd);
>>>       struct drm_syncobj *syncobj;
>>> +     int ret = 0;
>>
>> Please don't initialize return values on declaration that is often considered bad coding style.
>>
>>>
>>>       if (!fence)
>>>               return -EINVAL;
>>> @@ -743,17 +744,20 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>>>       if (point) {
>>
>>>               struct dma_fence_chain *chain = dma_fence_chain_alloc();
>>>
>>> -             if (!chain)
>>> -                     return -ENOMEM;
>>> +             if (!chain) {
>>> +                     ret = -ENOMEM;
>>> +                     goto out;
>>> +             }
>>>
>>>               drm_syncobj_add_point(syncobj, chain, fence, point);
>>
>> Allocation and adding the point is intentionally separated so that dma_fence_chain_alloc() can be called before looking up the syncobj or previous fence.
>>
>> I suggest to make use of that.
> 
> This is the fourth time that someone has tried to fix this trivial bug
> for over a year.

I was already wondering as well if we haven't fixed that before.

> The previous 3 cases were:
> 
> https://lore.kernel.org/all/c715ef30-9d91-46f3-8a0f-1039f3d25e7d@stanley.mountain/
> https://lore.kernel.org/all/20260516-jorth-syncobj-v1-9-88ede9d98a81@gmail.com/
> https://lore.kernel.org/all/20260525025455.3887523-1-vulab@iscas.ac.cn/
> 
> The last of which didn't even receive a reply. And there have been
> more cases in the past where I encountered a dri-devel-adjacent issue,
> googled it, and found that patches had already been sent in years ago
> to fix it, but were ignored.

The problem here is clearly that nobody feels responsible for pushing that fix.

Regards,
Christian.

> 
> Which is all to explain why I had all of the work for this series done by AI.
> 
> I heard some chatter about dri-devel adopting a gitlab-based workflow,
> and I hope that works out and I didn't misunderstand. Because the
> current system is clearly broken. Nobody keeps track of which bugs
> have been reported, nobody triages existing bugs, nobody keeps track
> of attempts to fix bugs. Which is how 3 people ended up doing repeat
> work of discovering, analyzing, and fixing the same bug.
> 
> The same pattern applies to my patch two. I clearly don't know enough
> about the (probably) undocumented semantics of where and when which
> structure of fences can be used. Which is why I sent in a bug report
> about this issue in May instead of trying to fix it on my own:
> https://lore.kernel.org/all/CAHijbEUKuXNeSnT82P-ojOhzU5siNHjxUW+qEDOqx07Ys9m=xw@mail.gmail.com/
> 
> I would have hoped that a bug report amounting to "this new feature
> causes the kernel to spam dmesg at 60 fps" would get some attention.
> But no. The only way to get this issue fixed seems to be me acting as
> a meat proxy to fix an issue that I know very little about.
> 
> Thanks for reading my blog post.
> 
> 
>>
>> Regards,
>> Christian.
>>
>>>       } else {
>>>               drm_syncobj_replace_fence(syncobj, fence);
>>>       }
>>>
>>> +out:
>>>       dma_fence_put(fence);
>>>       drm_syncobj_put(syncobj);
>>> -     return 0;
>>> +     return ret;
>>>  }
>>>
>>>  static int drm_syncobj_export_sync_file(struct drm_file *file_private,
>>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] drm/syncobj: flatten chains when importing sync_file into timeline point
  2026-10-12 11:42     ` Julian Orth
@ 2026-10-12 13:06       ` Christian König
  0 siblings, 0 replies; 9+ messages in thread
From: Christian König @ 2026-10-12 13:06 UTC (permalink / raw)
  To: Julian Orth
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Sumit Semwal, Dmitry Osipenko,
	Rob Clark, dri-devel, linux-kernel, linux-media, linaro-mm-sig

On 10/12/26 13:42, Julian Orth wrote:
> On Mon, Oct 12, 2026 at 9:51 AM Christian König
> <christian.koenig@amd.com> wrote:
>>
>> On 10/10/26 20:07, Julian Orth wrote:
>>> A sync_file exported from a timeline point can contain a dma_fence_chain.
>>
>> No it can't, IIRC sync files are only supposed to contain raw fences or dma_fence_array containers.
>>
>> I think we never documented that and that's why this issue here leaked in.
> 
> When I reviewed this patch before sending it in, I asked the AI if it
> would not be better to do the merging at export time. It was adamant
> that it had to be at import time because of commit 519f490d by
> yourself:
> 
>     dma-buf/sync-file: fix warning about fence containers
> 
>     The dma_fence_chain containers can show up in sync_files as well

Yeah that fix was driven by the warning but it was never a complete audit.

IIRC we later found multiple other issues as well and because of that the approach of exporting dma_fence_chain in syncfiles was dropped for at least one other occasion.

I don't mind allowing a dma_fence_chain in a syncfile, but we really need to audit the code because I can't think of a way how to do this automatically.

Mixing dma_fence_chain and dma_fence_array containers into larger data structures can trivially exhaust the kernel stack on release/cleanup because of recursion.

Regards,
Christian.

> 
>>
>> I'm not sure if that doesn't break anywhere else as well, so I think we should fix the exporting side instead.
>>
>> Alternatively we need to audit everywhere where fences from syncfiles can be imported somewhere else.
>>
>> Regards,
>> Christian.
>>
>>> Importing such a sync_file into a timeline point with
>>> DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE |
>>> DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE passes the chain directly to
>>> dma_fence_chain_init() and triggers
>>>
>>>   WARNING: drivers/dma-buf/dma-fence-chain.c:286 at dma_fence_chain_init
>>>
>>> because chain fences must not be wrapped in other chain fences.
>>>
>>> Flatten the fence with dma_fence_unwrap_merge() before adding it to the
>>> timeline, as drm_syncobj_transfer_to_timeline() already does.
>>>
>>> Reproducer:
>>>
>>> 1. Export a sync_file from a point > 0 of a timeline syncobj with
>>>    DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD.
>>> 2. Import it into a point > 0 of another timeline syncobj with
>>>    DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE.
>>>
>>> Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs")
>>> Assisted-by: Claude:claude-opus-5-5
>>> Signed-off-by: Julian Orth <ju.orth@gmail.com>
>>> ---
>>>  drivers/gpu/drm/drm_syncobj.c | 12 +++++++++++-
>>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>>> index 5ca5163d0..c545af77e 100644
>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>> @@ -742,8 +742,18 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>>>       }
>>>
>>>       if (point) {
>>> -             struct dma_fence_chain *chain = dma_fence_chain_alloc();
>>> +             struct dma_fence_chain *chain;
>>> +             struct dma_fence *tmp;
>>> +
>>> +             tmp = dma_fence_unwrap_merge(fence);
>>> +             dma_fence_put(fence);
>>> +             fence = tmp;
>>> +             if (!fence) {
>>> +                     drm_syncobj_put(syncobj);
>>> +                     return -ENOMEM;
>>> +             }
>>>
>>> +             chain = dma_fence_chain_alloc();
>>>               if (!chain) {
>>>                       ret = -ENOMEM;
>>>                       goto out;
>>


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-10-12 13:06 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-10 18:07 [PATCH 0/2] drm/syncobj: fix sync_file import into timeline points Julian Orth
2026-10-10 18:07 ` [PATCH 1/2] drm/syncobj: fix fence and syncobj leak in drm_syncobj_import_sync_file_fence Julian Orth
2026-10-12  7:45   ` Christian König
2026-10-12 11:38     ` Julian Orth
2026-10-12 12:56       ` Christian König
2026-10-10 18:07 ` [PATCH 2/2] drm/syncobj: flatten chains when importing sync_file into timeline point Julian Orth
2026-10-12  7:51   ` Christian König
2026-10-12 11:42     ` Julian Orth
2026-10-12 13:06       ` Christian König

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®