diff mbox series

[v2] dma-buf/sw_sync: Avoid recursive lock during fence signal

Message ID 20230818145939.39697-1-robdclark@gmail.com (mailing list archive)
State Not Applicable
Headers
Series [v2] dma-buf/sw_sync: Avoid recursive lock during fence signal |

Commit Message

Rob Clark Aug. 18, 2023, 2:59 p.m. UTC 
  From: Rob Clark <robdclark@chromium.org>

If a signal callback releases the sw_sync fence, that will trigger a
deadlock as the timeline_fence_release recurses onto the fence->lock
(used both for signaling and the the timeline tree).

To avoid that, temporarily hold an extra reference to the signalled
fences until after we drop the lock.

(This is an alternative implementation of https://patchwork.kernel.org/patch/11664717/
which avoids some potential UAF issues with the original patch.)

v2: Remove now obsolete comment, use list_move_tail() and
    list_del_init()

Reported-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/dma-buf/sw_sync.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Christian König Aug. 22, 2023, 1:01 p.m. UTC | #1 
Am 18.08.23 um 16:59 schrieb Rob Clark:
> From: Rob Clark <robdclark@chromium.org>
>
> If a signal callback releases the sw_sync fence, that will trigger a
> deadlock as the timeline_fence_release recurses onto the fence->lock
> (used both for signaling and the the timeline tree).
>
> To avoid that, temporarily hold an extra reference to the signalled
> fences until after we drop the lock.
>
> (This is an alternative implementation of https://patchwork.kernel.org/patch/11664717/
> which avoids some potential UAF issues with the original patch.)
>
> v2: Remove now obsolete comment, use list_move_tail() and
>      list_del_init()
>
> Reported-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/dma-buf/sw_sync.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 63f0aeb66db6..f0a35277fd84 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -191,6 +191,7 @@ static const struct dma_fence_ops timeline_fence_ops = {
>    */
>   static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
>   {
> +	LIST_HEAD(signalled);
>   	struct sync_pt *pt, *next;
>   
>   	trace_sync_timeline(obj);
> @@ -203,21 +204,20 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
>   		if (!timeline_fence_signaled(&pt->base))
>   			break;
>   
> -		list_del_init(&pt->link);
> +		dma_fence_get(&pt->base);
> +
> +		list_move_tail(&pt->link, &signalled);
>   		rb_erase(&pt->node, &obj->pt_tree);
>   
> -		/*
> -		 * A signal callback may release the last reference to this
> -		 * fence, causing it to be freed. That operation has to be
> -		 * last to avoid a use after free inside this loop, and must
> -		 * be after we remove the fence from the timeline in order to
> -		 * prevent deadlocking on timeline->lock inside
> -		 * timeline_fence_release().
> -		 */
>   		dma_fence_signal_locked(&pt->base);
>   	}
>   
>   	spin_unlock_irq(&obj->lock);
> +
> +	list_for_each_entry_safe(pt, next, &signalled, link) {
> +		list_del_init(&pt->link);
> +		dma_fence_put(&pt->base);
> +	}
>   }
>   
>   /**
Rob Clark Aug. 22, 2023, 5:15 p.m. UTC | #2 
On Tue, Aug 22, 2023 at 6:01 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 18.08.23 um 16:59 schrieb Rob Clark:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > If a signal callback releases the sw_sync fence, that will trigger a
> > deadlock as the timeline_fence_release recurses onto the fence->lock
> > (used both for signaling and the the timeline tree).
> >
> > To avoid that, temporarily hold an extra reference to the signalled
> > fences until after we drop the lock.
> >
> > (This is an alternative implementation of https://patchwork.kernel.org/patch/11664717/
> > which avoids some potential UAF issues with the original patch.)
> >
> > v2: Remove now obsolete comment, use list_move_tail() and
> >      list_del_init()
> >
> > Reported-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
>
> Reviewed-by: Christian König <christian.koenig@amd.com>

Thanks, any chance you could take this via drm-misc?

BR,
-R

>
> > ---
> >   drivers/dma-buf/sw_sync.c | 18 +++++++++---------
> >   1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> > index 63f0aeb66db6..f0a35277fd84 100644
> > --- a/drivers/dma-buf/sw_sync.c
> > +++ b/drivers/dma-buf/sw_sync.c
> > @@ -191,6 +191,7 @@ static const struct dma_fence_ops timeline_fence_ops = {
> >    */
> >   static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
> >   {
> > +     LIST_HEAD(signalled);
> >       struct sync_pt *pt, *next;
> >
> >       trace_sync_timeline(obj);
> > @@ -203,21 +204,20 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
> >               if (!timeline_fence_signaled(&pt->base))
> >                       break;
> >
> > -             list_del_init(&pt->link);
> > +             dma_fence_get(&pt->base);
> > +
> > +             list_move_tail(&pt->link, &signalled);
> >               rb_erase(&pt->node, &obj->pt_tree);
> >
> > -             /*
> > -              * A signal callback may release the last reference to this
> > -              * fence, causing it to be freed. That operation has to be
> > -              * last to avoid a use after free inside this loop, and must
> > -              * be after we remove the fence from the timeline in order to
> > -              * prevent deadlocking on timeline->lock inside
> > -              * timeline_fence_release().
> > -              */
> >               dma_fence_signal_locked(&pt->base);
> >       }
> >
> >       spin_unlock_irq(&obj->lock);
> > +
> > +     list_for_each_entry_safe(pt, next, &signalled, link) {
> > +             list_del_init(&pt->link);
> > +             dma_fence_put(&pt->base);
> > +     }
> >   }
> >
> >   /**
>
Christian König Aug. 28, 2023, 5:56 a.m. UTC | #3 
Am 22.08.23 um 19:15 schrieb Rob Clark:
> On Tue, Aug 22, 2023 at 6:01 AM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> Am 18.08.23 um 16:59 schrieb Rob Clark:
>>> From: Rob Clark <robdclark@chromium.org>
>>>
>>> If a signal callback releases the sw_sync fence, that will trigger a
>>> deadlock as the timeline_fence_release recurses onto the fence->lock
>>> (used both for signaling and the the timeline tree).
>>>
>>> To avoid that, temporarily hold an extra reference to the signalled
>>> fences until after we drop the lock.
>>>
>>> (This is an alternative implementation of https://patchwork.kernel.org/patch/11664717/
>>> which avoids some potential UAF issues with the original patch.)
>>>
>>> v2: Remove now obsolete comment, use list_move_tail() and
>>>       list_del_init()
>>>
>>> Reported-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
>>> Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free")
>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
> Thanks, any chance you could take this via drm-misc?

I've already pushed this quite a while ago.

At the moment I have problem answering because AMD has a new security 
policy which makes it impossible to push patches and access mails at the 
same time.

We are working with our IT to get this fixed, but at the moment its 
eating my time.

Sorry for the delay,
Christian.

>
> BR,
> -R
>
>>> ---
>>>    drivers/dma-buf/sw_sync.c | 18 +++++++++---------
>>>    1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
>>> index 63f0aeb66db6..f0a35277fd84 100644
>>> --- a/drivers/dma-buf/sw_sync.c
>>> +++ b/drivers/dma-buf/sw_sync.c
>>> @@ -191,6 +191,7 @@ static const struct dma_fence_ops timeline_fence_ops = {
>>>     */
>>>    static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
>>>    {
>>> +     LIST_HEAD(signalled);
>>>        struct sync_pt *pt, *next;
>>>
>>>        trace_sync_timeline(obj);
>>> @@ -203,21 +204,20 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
>>>                if (!timeline_fence_signaled(&pt->base))
>>>                        break;
>>>
>>> -             list_del_init(&pt->link);
>>> +             dma_fence_get(&pt->base);
>>> +
>>> +             list_move_tail(&pt->link, &signalled);
>>>                rb_erase(&pt->node, &obj->pt_tree);
>>>
>>> -             /*
>>> -              * A signal callback may release the last reference to this
>>> -              * fence, causing it to be freed. That operation has to be
>>> -              * last to avoid a use after free inside this loop, and must
>>> -              * be after we remove the fence from the timeline in order to
>>> -              * prevent deadlocking on timeline->lock inside
>>> -              * timeline_fence_release().
>>> -              */
>>>                dma_fence_signal_locked(&pt->base);
>>>        }
>>>
>>>        spin_unlock_irq(&obj->lock);
>>> +
>>> +     list_for_each_entry_safe(pt, next, &signalled, link) {
>>> +             list_del_init(&pt->link);
>>> +             dma_fence_put(&pt->base);
>>> +     }
>>>    }
>>>
>>>    /**
diff mbox series

Patch

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 63f0aeb66db6..f0a35277fd84 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -191,6 +191,7 @@  static const struct dma_fence_ops timeline_fence_ops = {
  */
 static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 {
+	LIST_HEAD(signalled);
 	struct sync_pt *pt, *next;
 
 	trace_sync_timeline(obj);
@@ -203,21 +204,20 @@  static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 		if (!timeline_fence_signaled(&pt->base))
 			break;
 
-		list_del_init(&pt->link);
+		dma_fence_get(&pt->base);
+
+		list_move_tail(&pt->link, &signalled);
 		rb_erase(&pt->node, &obj->pt_tree);
 
-		/*
-		 * A signal callback may release the last reference to this
-		 * fence, causing it to be freed. That operation has to be
-		 * last to avoid a use after free inside this loop, and must
-		 * be after we remove the fence from the timeline in order to
-		 * prevent deadlocking on timeline->lock inside
-		 * timeline_fence_release().
-		 */
 		dma_fence_signal_locked(&pt->base);
 	}
 
 	spin_unlock_irq(&obj->lock);
+
+	list_for_each_entry_safe(pt, next, &signalled, link) {
+		list_del_init(&pt->link);
+		dma_fence_put(&pt->base);
+	}
 }
 
 /**