dma-buf/sw_sync: Synchronize signal vs syncpt free

Submitted by Chris Wilson on Aug. 12, 2019, 3:42 p.m.

Details

Message ID 20190812154247.20508-1-chris@chris-wilson.co.uk
State New
Headers show
Series "dma-buf/sw_sync: Synchronize signal vs syncpt free" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Chris Wilson Aug. 12, 2019, 3:42 p.m.
During release of the syncpt, we remove it from the list of syncpt and
the tree, but only if it is not already been removed. However, during
signaling, we first remove the syncpt from the list. So, if we
concurrently free and signal the syncpt, the free may decide that it is
not part of the tree and immediately free itself -- meanwhile the
signaler goes onto to use the now freed datastructure.

In particular, we get struct by commit 0e2f733addbf ("dma-buf: make
dma_fence structure a bit smaller v2") as the cb_list is immediately
clobbered by the kfree_rcu.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111381
Fixes: d3862e44daa7 ("dma-buf/sw-sync: Fix locking around sync_timeline lists")
References: 0e2f733addbf ("dma-buf: make dma_fence structure a bit smaller v2")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: <stable@vger.kernel.org> # v4.14+
---
 drivers/dma-buf/sw_sync.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 051f6c2873c7..27b1d549ed38 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -132,17 +132,14 @@  static void timeline_fence_release(struct dma_fence *fence)
 {
 	struct sync_pt *pt = dma_fence_to_sync_pt(fence);
 	struct sync_timeline *parent = dma_fence_parent(fence);
+	unsigned long flags;
 
+	spin_lock_irqsave(fence->lock, flags);
 	if (!list_empty(&pt->link)) {
-		unsigned long flags;
-
-		spin_lock_irqsave(fence->lock, flags);
-		if (!list_empty(&pt->link)) {
-			list_del(&pt->link);
-			rb_erase(&pt->node, &parent->pt_tree);
-		}
-		spin_unlock_irqrestore(fence->lock, flags);
+		list_del(&pt->link);
+		rb_erase(&pt->node, &parent->pt_tree);
 	}
+	spin_unlock_irqrestore(fence->lock, flags);
 
 	sync_timeline_put(parent);
 	dma_fence_free(fence);

Comments

Am 12.08.19 um 17:42 schrieb Chris Wilson:
> During release of the syncpt, we remove it from the list of syncpt and

> the tree, but only if it is not already been removed. However, during

> signaling, we first remove the syncpt from the list. So, if we

> concurrently free and signal the syncpt, the free may decide that it is

> not part of the tree and immediately free itself -- meanwhile the

> signaler goes onto to use the now freed datastructure.

>

> In particular, we get struct by commit 0e2f733addbf ("dma-buf: make

> dma_fence structure a bit smaller v2") as the cb_list is immediately

> clobbered by the kfree_rcu.

>

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111381

> Fixes: d3862e44daa7 ("dma-buf/sw-sync: Fix locking around sync_timeline lists")

> References: 0e2f733addbf ("dma-buf: make dma_fence structure a bit smaller v2")

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

> Cc: Sumit Semwal <sumit.semwal@linaro.org>

> Cc: Sean Paul <seanpaul@chromium.org>

> Cc: Gustavo Padovan <gustavo@padovan.org>

> Cc: Christian König <christian.koenig@amd.com>

> Cc: <stable@vger.kernel.org> # v4.14+


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


> ---

>   drivers/dma-buf/sw_sync.c | 13 +++++--------

>   1 file changed, 5 insertions(+), 8 deletions(-)

>

> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c

> index 051f6c2873c7..27b1d549ed38 100644

> --- a/drivers/dma-buf/sw_sync.c

> +++ b/drivers/dma-buf/sw_sync.c

> @@ -132,17 +132,14 @@ static void timeline_fence_release(struct dma_fence *fence)

>   {

>   	struct sync_pt *pt = dma_fence_to_sync_pt(fence);

>   	struct sync_timeline *parent = dma_fence_parent(fence);

> +	unsigned long flags;

>   

> +	spin_lock_irqsave(fence->lock, flags);

>   	if (!list_empty(&pt->link)) {

> -		unsigned long flags;

> -

> -		spin_lock_irqsave(fence->lock, flags);

> -		if (!list_empty(&pt->link)) {

> -			list_del(&pt->link);

> -			rb_erase(&pt->node, &parent->pt_tree);

> -		}

> -		spin_unlock_irqrestore(fence->lock, flags);

> +		list_del(&pt->link);

> +		rb_erase(&pt->node, &parent->pt_tree);

>   	}

> +	spin_unlock_irqrestore(fence->lock, flags);

>   

>   	sync_timeline_put(parent);

>   	dma_fence_free(fence);
Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: d3862e44daa7 dma-buf/sw-sync: Fix locking around sync_timeline lists.

The bot has tested the following trees: v5.2.8, v4.19.66, v4.14.138, v4.9.189.

v5.2.8: Build OK!
v4.19.66: Build OK!
v4.14.138: Build OK!
v4.9.189: Failed to apply! Possible dependencies:
    Unable to calculate


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

--
Thanks,
Sasha
Hi Sasha,

On Mon, Aug 12, 2019 at 07:05:47PM +0000, Sasha Levin wrote:
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: d3862e44daa7 dma-buf/sw-sync: Fix locking around sync_timeline lists.
> 
> The bot has tested the following trees: v5.2.8, v4.19.66, v4.14.138, v4.9.189.
> 
> v5.2.8: Build OK!
> v4.19.66: Build OK!
> v4.14.138: Build OK!
> v4.9.189: Failed to apply! Possible dependencies:
>     Unable to calculate
> 
> 
> NOTE: The patch will not be queued to stable trees until it is upstream.
> 
> How should we proceed with this patch?

The backporting instruction has an explicit # v4.14+ in there, so failure
to apply to older kernels is expected.

Can you perhaps teach this trick to your script perhaps? Iirc we're using
the official format even.
-Daniel
On Wed, Aug 14, 2019 at 07:24:15PM +0200, Daniel Vetter wrote:
>Hi Sasha,
>
>On Mon, Aug 12, 2019 at 07:05:47PM +0000, Sasha Levin wrote:
>> Hi,
>>
>> [This is an automated email]
>>
>> This commit has been processed because it contains a "Fixes:" tag,
>> fixing commit: d3862e44daa7 dma-buf/sw-sync: Fix locking around sync_timeline lists.
>>
>> The bot has tested the following trees: v5.2.8, v4.19.66, v4.14.138, v4.9.189.
>>
>> v5.2.8: Build OK!
>> v4.19.66: Build OK!
>> v4.14.138: Build OK!
>> v4.9.189: Failed to apply! Possible dependencies:
>>     Unable to calculate
>>
>>
>> NOTE: The patch will not be queued to stable trees until it is upstream.
>>
>> How should we proceed with this patch?
>
>The backporting instruction has an explicit # v4.14+ in there, so failure
>to apply to older kernels is expected.
>
>Can you perhaps teach this trick to your script perhaps? Iirc we're using
>the official format even.

Hey Daniel,

The script knows how to read stable tags :)

It tested out 4.9 because the commit also has a fixes tag pointing to
d3862e44daa7 ("dma-buf/sw-sync: Fix locking around sync_timeline
lists."), which was backported to 4.9.

Should this not be backported to 4.9, even though the commit it fixes is
there?

--
Thanks,
Sasha
On Wed, Aug 14, 2019 at 09:46:41PM -0400, Sasha Levin wrote:
> On Wed, Aug 14, 2019 at 07:24:15PM +0200, Daniel Vetter wrote:
> > Hi Sasha,
> > 
> > On Mon, Aug 12, 2019 at 07:05:47PM +0000, Sasha Levin wrote:
> > > Hi,
> > > 
> > > [This is an automated email]
> > > 
> > > This commit has been processed because it contains a "Fixes:" tag,
> > > fixing commit: d3862e44daa7 dma-buf/sw-sync: Fix locking around sync_timeline lists.
> > > 
> > > The bot has tested the following trees: v5.2.8, v4.19.66, v4.14.138, v4.9.189.
> > > 
> > > v5.2.8: Build OK!
> > > v4.19.66: Build OK!
> > > v4.14.138: Build OK!
> > > v4.9.189: Failed to apply! Possible dependencies:
> > >     Unable to calculate
> > > 
> > > 
> > > NOTE: The patch will not be queued to stable trees until it is upstream.
> > > 
> > > How should we proceed with this patch?
> > 
> > The backporting instruction has an explicit # v4.14+ in there, so failure
> > to apply to older kernels is expected.
> > 
> > Can you perhaps teach this trick to your script perhaps? Iirc we're using
> > the official format even.
> 
> Hey Daniel,
> 
> The script knows how to read stable tags :)
> 
> It tested out 4.9 because the commit also has a fixes tag pointing to
> d3862e44daa7 ("dma-buf/sw-sync: Fix locking around sync_timeline
> lists."), which was backported to 4.9.

Ah makes sense, might be good to add a bit of output explaining that.

> Should this not be backported to 4.9, even though the commit it fixes is
> there?

I guess it might actually be needed there.
-Daniel