[PATCH 00/10] dmaengine: fix race with vchan_complete

[PATCH 00/10] dmaengine: fix race with vchan_complete

[Peter Ujfalusi]

Hi,

With the introduction of .device_synchronize callback it was thought that the
race caused crash observed in vchan_complete is fixed, but unfortunately it can
still happen.

The observed scenario (really hard to reproduce) is:
Cyclic mode
- DMA period interrupt
 - call to vchan_cyclic_callback() which sets vc->cyclic to vd and schedules the
   vchan_complete tasklet
- .terminate_all is called
 - we make sure that no further DMA irqs are going to be handled, but the
   tasklet had been already scheduled
 - we free up the descriptor to avoid leaking memory
- the vchan_complete tasklet starts to execute and it checks vc->cyclic, which
  is not NULL, saves the vd pointer (which points to an already freed up memory)
  and try to access it later to call the callback
- the tasklet_kill() in .device_synchronize will make sure that the tasklet is
  going to finish it's execution if it is already scheduled, it can only help if
  the tasklet is yet to be executed.

At this point it is just matter of luck if the vc->cyclic is still pointing to
an unchanged memory location or it is taken into use and thus it is corrupted.

My first approach was to just set vc->cyclic to NULL in the .terminate_all
callback, but that still have theoritical race: if the vchan_complete is
executing and it saves the vd from vc->cyclic (protected by the vc->lock). If at
that point the .terminate_all is called it will wait for the lock and start
executing. _if_ the .terminate_all free up the vd before the vchan_complete
reaches the point when it is going to call the callback, then we have the race.

The series will do this:
- the drivers should call vchan_terminate_vdesc() instead of directly freeing up
  the descriptor. vchan_terminate_vdesc() will save the vd as vc->vd_terminated
  and will set the vc->cyclic to NULL, all while holding the lock.
- the drivers must implement the .device_synchronize callback and within the
  vchan_synchronize() we free up the vc->vd_terminated after we killed the
  tasklet.

I have tested this on platforms using TI's eDMA and sDMA and have not seen any
side effect so far and a client tested similar set on a setup where it was easy
to reproduce the race.

By looking for similar patterns in other drivers I have implemented the fix for
the ones where it looked straight forward.

Regards,
Peter
---
Peter Ujfalusi (10):
  dmaengine: virt-dma: Add helper to free/reuse a descriptor
  dmaengine: virt-dma: Support for race free transfer termination
  dmaengine: omap-dma: Use vchan_terminate_vdesc() instead of desc_free
  dmaengine: edma: Use vchan_terminate_vdesc() instead of desc_free
  dmaengine: bcm2835-dma: Use vchan_terminate_vdesc() instead of
    desc_free
  dmaengine: dma-jz4780: Use vchan_terminate_vdesc() instead of
    desc_free
  dmaengine: amba-pl08x: Use vchan_terminate_vdesc() instead of
    desc_free
  dmaengine: img-mdc-dma: Use vchan_terminate_vdesc() instead of
    desc_free
  dmaengine: k3dma: Use vchan_terminate_vdesc() instead of desc_free
  dmaengine: s3c24xx-dma: Use vchan_terminate_vdesc() instead of
    desc_free

 drivers/dma/amba-pl08x.c  | 11 ++++++++++-
 drivers/dma/bcm2835-dma.c | 10 +++++++++-
 drivers/dma/dma-jz4780.c  | 10 +++++++++-
 drivers/dma/edma.c        |  7 ++-----
 drivers/dma/img-mdc-dma.c | 17 ++++++++++++-----
 drivers/dma/k3dma.c       | 10 +++++++++-
 drivers/dma/omap-dma.c    |  2 +-
 drivers/dma/s3c24xx-dma.c | 11 ++++++++++-
 drivers/dma/virt-dma.c    |  5 +----
 drivers/dma/virt-dma.h    | 44 ++++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 107 insertions(+), 20 deletions(-)

-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 01/10] dmaengine: virt-dma: Add helper to free/reuse a descriptor

[Peter Ujfalusi]

The vchan_vdesc_fini() can be used to free or reuse a given descriptor
after it has been marked as completed.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/virt-dma.c |  5 +----
 drivers/dma/virt-dma.h | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
index 545e97279083..88ad8ed2a8d6 100644
--- a/drivers/dma/virt-dma.c
+++ b/drivers/dma/virt-dma.c
@@ -107,10 +107,7 @@ static void vchan_complete(unsigned long arg)
 		dmaengine_desc_get_callback(&vd->tx, &cb);
 
 		list_del(&vd->node);
-		if (dmaengine_desc_test_reuse(&vd->tx))
-			list_add(&vd->node, &vc->desc_allocated);
-		else
-			vc->desc_free(vd);
+		vchan_vdesc_fini(vd);
 
 		dmaengine_desc_callback_invoke(&cb, NULL);
 	}
diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h
index 3f776a46a29c..2edb05505102 100644
--- a/drivers/dma/virt-dma.h
+++ b/drivers/dma/virt-dma.h
@@ -103,6 +103,20 @@ static inline void vchan_cookie_complete(struct virt_dma_desc *vd)
 	tasklet_schedule(&vc->task);
 }
 
+/**
+ * vchan_vdesc_fini - Free or reuse a descriptor
+ * @vd: virtual descriptor to free/reuse
+ */
+static inline void vchan_vdesc_fini(struct virt_dma_desc *vd)
+{
+	struct virt_dma_chan *vc = to_virt_chan(vd->tx.chan);
+
+	if (dmaengine_desc_test_reuse(&vd->tx))
+		list_add(&vd->node, &vc->desc_allocated);
+	else
+		vc->desc_free(vd);
+}
+
 /**
  * vchan_cyclic_callback - report the completion of a period
  * @vd: virtual descriptor
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 01/10] dmaengine: virt-dma: Add helper to free/reuse a descriptor

[Linus Walleij]

On Tue, Nov 14, 2017 at 3:32 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> The vchan_vdesc_fini() can be used to free or reuse a given descriptor
> after it has been marked as completed.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 02/10] dmaengine: virt-dma: Support for race free transfer termination

[Peter Ujfalusi]

Even with the introduced vchan_synchronize() we can face race when
terminating a cyclic transfer.

If the terminate_all is called after the interrupt handler called
vchan_cyclic_callback(), but before the vchan_complete tasklet is called:
vc->cyclic is set to the cyclic descriptor, but the descriptor itself was
freed up in the driver's terminate_all() callback.
When the vhan_complete() is executed it will try to fetch the vc->cyclic
vdesc, but the pointer is pointing now to uninitialized memory leading to
(hard to reproduce) kernel crash.

In order to fix this, drivers should:
- call vchan_terminate_vdesc() from their terminate_all callback instead
calling their free_desc function to free up the descriptor.
- implement device_synchronize callback and call vchan_synchronize().

This way we can make sure that the descriptor is only going to be freed up
after the vchan_callback was executed in a safe manner.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/virt-dma.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h
index 2edb05505102..b09b75ab0751 100644
--- a/drivers/dma/virt-dma.h
+++ b/drivers/dma/virt-dma.h
@@ -35,6 +35,7 @@ struct virt_dma_chan {
 	struct list_head desc_completed;
 
 	struct virt_dma_desc *cyclic;
+	struct virt_dma_desc *vd_terminated;
 };
 
 static inline struct virt_dma_chan *to_virt_chan(struct dma_chan *chan)
@@ -129,6 +130,25 @@ static inline void vchan_cyclic_callback(struct virt_dma_desc *vd)
 	tasklet_schedule(&vc->task);
 }
 
+/**
+ * vchan_terminate_vdesc - Disable pending cyclic callback
+ * @vd: virtual descriptor to be terminated
+ *
+ * vc.lock must be held by caller
+ */
+static inline void vchan_terminate_vdesc(struct virt_dma_desc *vd)
+{
+	struct virt_dma_chan *vc = to_virt_chan(vd->tx.chan);
+
+	/* free up stuck descriptor */
+	if (vc->vd_terminated)
+		vchan_vdesc_fini(vc->vd_terminated);
+
+	vc->vd_terminated = vd;
+	if (vc->cyclic == vd)
+		vc->cyclic = NULL;
+}
+
 /**
  * vchan_next_desc - peek at the next descriptor to be processed
  * @vc: virtual channel to obtain descriptor from
@@ -182,10 +202,20 @@ static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
  * Makes sure that all scheduled or active callbacks have finished running. For
  * proper operation the caller has to ensure that no new callbacks are scheduled
  * after the invocation of this function started.
+ * Free up the terminated cyclic descriptor to prevent memory leakage.
  */
 static inline void vchan_synchronize(struct virt_dma_chan *vc)
 {
+	unsigned long flags;
+
 	tasklet_kill(&vc->task);
+
+	spin_lock_irqsave(&vc->lock, flags);
+	if (vc->vd_terminated) {
+		vchan_vdesc_fini(vc->vd_terminated);
+		vc->vd_terminated = NULL;
+	}
+	spin_unlock_irqrestore(&vc->lock, flags);
 }
 
 #endif
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 02/10] dmaengine: virt-dma: Support for race free transfer termination

[Linus Walleij]

On Tue, Nov 14, 2017 at 3:32 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> Even with the introduced vchan_synchronize() we can face race when
> terminating a cyclic transfer.
>
> If the terminate_all is called after the interrupt handler called
> vchan_cyclic_callback(), but before the vchan_complete tasklet is called:
> vc->cyclic is set to the cyclic descriptor, but the descriptor itself was
> freed up in the driver's terminate_all() callback.
> When the vhan_complete() is executed it will try to fetch the vc->cyclic
> vdesc, but the pointer is pointing now to uninitialized memory leading to
> (hard to reproduce) kernel crash.
>
> In order to fix this, drivers should:
> - call vchan_terminate_vdesc() from their terminate_all callback instead
> calling their free_desc function to free up the descriptor.
> - implement device_synchronize callback and call vchan_synchronize().
>
> This way we can make sure that the descriptor is only going to be freed up
> after the vchan_callback was executed in a safe manner.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 03/10] dmaengine: omap-dma: Use vchan_terminate_vdesc() instead of desc_free

[Peter Ujfalusi]

To avoid race with vchan_complete, use the race free way to terminate
running transfer.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/omap-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
index f6dd849159d8..d21c19822feb 100644
--- a/drivers/dma/omap-dma.c
+++ b/drivers/dma/omap-dma.c
@@ -1311,7 +1311,7 @@ static int omap_dma_terminate_all(struct dma_chan *chan)
 	 * c->desc is NULL and exit.)
 	 */
 	if (c->desc) {
-		omap_dma_desc_free(&c->desc->vd);
+		vchan_terminate_vdesc(&c->desc->vd);
 		c->desc = NULL;
 		/* Avoid stopping the dma twice */
 		if (!c->paused)
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 04/10] dmaengine: edma: Use vchan_terminate_vdesc() instead of desc_free

[Peter Ujfalusi]

To avoid race with vchan_complete, use the race free way to terminate
running transfer.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 9364a3ed345a..948df1ab5f1a 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -860,11 +860,8 @@ static int edma_terminate_all(struct dma_chan *chan)
 		/* Move the cyclic channel back to default queue */
 		if (!echan->tc && echan->edesc->cyclic)
 			edma_assign_channel_eventq(echan, EVENTQ_DEFAULT);
-		/*
-		 * free the running request descriptor
-		 * since it is not in any of the vdesc lists
-		 */
-		edma_desc_free(&echan->edesc->vdesc);
+
+		vchan_terminate_vdesc(&echan->edesc->vdesc);
 		echan->edesc = NULL;
 	}
 
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 05/10] dmaengine: bcm2835-dma: Use vchan_terminate_vdesc() instead of desc_free

[Peter Ujfalusi]

To avoid race with vchan_complete, use the race free way to terminate
running transfer.

Implement the device_synchronize callback to make sure that the terminated
descriptor is freed.

CC: Martin Sperl <kernel@martin.sperl.org>
CC: Eric Anholt <eric@anholt.net>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/bcm2835-dma.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 6204cc32d09c..847f84a41a69 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -812,7 +812,7 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
 	 * c->desc is NULL and exit.)
 	 */
 	if (c->desc) {
-		bcm2835_dma_desc_free(&c->desc->vd);
+		vchan_terminate_vdesc(&c->desc->vd);
 		c->desc = NULL;
 		bcm2835_dma_abort(c->chan_base);
 
@@ -836,6 +836,13 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
 	return 0;
 }
 
+static void bcm2835_dma_synchronize(struct dma_chan *chan)
+{
+	struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
+
+	vchan_synchronize(&c->vc);
+}
+
 static int bcm2835_dma_chan_init(struct bcm2835_dmadev *d, int chan_id,
 				 int irq, unsigned int irq_flags)
 {
@@ -942,6 +949,7 @@ static int bcm2835_dma_probe(struct platform_device *pdev)
 	od->ddev.device_prep_dma_memcpy = bcm2835_dma_prep_dma_memcpy;
 	od->ddev.device_config = bcm2835_dma_slave_config;
 	od->ddev.device_terminate_all = bcm2835_dma_terminate_all;
+	od->ddev.device_synchronize = bcm2835_dma_synchronize;
 	od->ddev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
 	od->ddev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
 	od->ddev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV) |
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 05/10] dmaengine: bcm2835-dma: Use vchan_terminate_vdesc() instead of desc_free

[Eric Anholt]

Peter Ujfalusi <peter.ujfalusi@ti.com> writes:

> To avoid race with vchan_complete, use the race free way to terminate
> running transfer.
>
> Implement the device_synchronize callback to make sure that the terminated
> descriptor is freed.
>
> CC: Martin Sperl <kernel@martin.sperl.org>
> CC: Eric Anholt <eric@anholt.net>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

I haven't fully followed the series, but thanks for porting your fix to
other platforms!

Acked-by: Eric Anholt <eric@anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171115/173153af/attachment.sig>

[PATCH 05/10] dmaengine: bcm2835-dma: Use vchan_terminate_vdesc() instead of desc_free

[Peter Ujfalusi]

Eric,

On 11/15/2017 08:53 PM, Eric Anholt wrote:
> Peter Ujfalusi <peter.ujfalusi@ti.com> writes:
> 
>> To avoid race with vchan_complete, use the race free way to terminate
>> running transfer.
>>
>> Implement the device_synchronize callback to make sure that the terminated
>> descriptor is freed.
>>
>> CC: Martin Sperl <kernel@martin.sperl.org>
>> CC: Eric Anholt <eric@anholt.net>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> I haven't fully followed the series, but thanks for porting your fix to
> other platforms!
I have seen similar patterns in these drivers and it was the right thing to do
imho.
Unfortunately I can not test on other platforms than eDMA and sDMA, but I
firmly believe that based on the usage it should be fine as it survives my
torture tests.

It would be great to see some Tested-by from others to have more coverage.

> Acked-by: Eric Anholt <eric@anholt.net>

Thank you!


-- 
P?ter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 06/10] dmaengine: dma-jz4780: Use vchan_terminate_vdesc() instead of desc_free

[Peter Ujfalusi]

To avoid race with vchan_complete, use the race free way to terminate
running transfer.

Implement the device_synchronize callback to make sure that the terminated
descriptor is freed.

CC: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/dma-jz4780.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 7373b7a555ec..85820a2d69d4 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -511,7 +511,7 @@ static int jz4780_dma_terminate_all(struct dma_chan *chan)
 	/* Clear the DMA status and stop the transfer. */
 	jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0);
 	if (jzchan->desc) {
-		jz4780_dma_desc_free(&jzchan->desc->vdesc);
+		vchan_terminate_vdesc(&jzchan->desc->vdesc);
 		jzchan->desc = NULL;
 	}
 
@@ -523,6 +523,13 @@ static int jz4780_dma_terminate_all(struct dma_chan *chan)
 	return 0;
 }
 
+static void jz4780_dma_synchronize(struct dma_chan *chan)
+{
+	struct jz4780_dma_chan *jzchan = to_jz4780_dma_chan(chan);
+
+	vchan_synchronize(&jzchan->vchan);
+}
+
 static int jz4780_dma_config(struct dma_chan *chan,
 	struct dma_slave_config *config)
 {
@@ -813,6 +820,7 @@ static int jz4780_dma_probe(struct platform_device *pdev)
 	dd->device_prep_dma_memcpy = jz4780_dma_prep_dma_memcpy;
 	dd->device_config = jz4780_dma_config;
 	dd->device_terminate_all = jz4780_dma_terminate_all;
+	dd->device_synchronize = jz4780_dma_synchronize;
 	dd->device_tx_status = jz4780_dma_tx_status;
 	dd->device_issue_pending = jz4780_dma_issue_pending;
 	dd->src_addr_widths = JZ_DMA_BUSWIDTHS;
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 07/10] dmaengine: amba-pl08x: Use vchan_terminate_vdesc() instead of desc_free

[Peter Ujfalusi]

To avoid race with vchan_complete, use the race free way to terminate
running transfer.

Implement the device_synchronize callback to make sure that the terminated
descriptor is freed.

CC: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/amba-pl08x.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index b52b0d55247e..97483df1f82e 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -2182,7 +2182,7 @@ static int pl08x_terminate_all(struct dma_chan *chan)
 	}
 	/* Dequeue jobs and free LLIs */
 	if (plchan->at) {
-		pl08x_desc_free(&plchan->at->vd);
+		vchan_terminate_vdesc(&plchan->at->vd);
 		plchan->at = NULL;
 	}
 	/* Dequeue jobs not yet fired as well */
@@ -2193,6 +2193,13 @@ static int pl08x_terminate_all(struct dma_chan *chan)
 	return 0;
 }
 
+static void pl08x_synchronize(struct dma_chan *chan)
+{
+	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
+
+	vchan_synchronize(&plchan->vc);
+}
+
 static int pl08x_pause(struct dma_chan *chan)
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
@@ -2773,6 +2780,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 	pl08x->memcpy.device_pause = pl08x_pause;
 	pl08x->memcpy.device_resume = pl08x_resume;
 	pl08x->memcpy.device_terminate_all = pl08x_terminate_all;
+	pl08x->memcpy.device_synchronize = pl08x_synchronize;
 	pl08x->memcpy.src_addr_widths = PL80X_DMA_BUSWIDTHS;
 	pl08x->memcpy.dst_addr_widths = PL80X_DMA_BUSWIDTHS;
 	pl08x->memcpy.directions = BIT(DMA_MEM_TO_MEM);
@@ -2802,6 +2810,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 		pl08x->slave.device_pause = pl08x_pause;
 		pl08x->slave.device_resume = pl08x_resume;
 		pl08x->slave.device_terminate_all = pl08x_terminate_all;
+		pl08x->slave.device_synchronize = pl08x_synchronize;
 		pl08x->slave.src_addr_widths = PL80X_DMA_BUSWIDTHS;
 		pl08x->slave.dst_addr_widths = PL80X_DMA_BUSWIDTHS;
 		pl08x->slave.directions =
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 07/10] dmaengine: amba-pl08x: Use vchan_terminate_vdesc() instead of desc_free

[Linus Walleij]

On Tue, Nov 14, 2017 at 3:32 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> To avoid race with vchan_complete, use the race free way to terminate
> running transfer.
>
> Implement the device_synchronize callback to make sure that the terminated
> descriptor is freed.
>
> CC: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

I had to read patch 1 before I understood how the descriptor
gets free:ed now, but now I see it :)
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

(Good work with hunting down these corner cases, I'm
very happy you're doing this!)

Yours,
Linus Walleij

[PATCH 08/10] dmaengine: img-mdc-dma: Use vchan_terminate_vdesc() instead of desc_free

[Peter Ujfalusi]

To avoid race with vchan_complete, use the race free way to terminate
running transfer.

Implement the device_synchronize callback to make sure that the terminated
descriptor is freed.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/img-mdc-dma.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/img-mdc-dma.c b/drivers/dma/img-mdc-dma.c
index 0391f930aecc..25cec9c243e1 100644
--- a/drivers/dma/img-mdc-dma.c
+++ b/drivers/dma/img-mdc-dma.c
@@ -694,7 +694,6 @@ static unsigned int mdc_get_new_events(struct mdc_chan *mchan)
 static int mdc_terminate_all(struct dma_chan *chan)
 {
 	struct mdc_chan *mchan = to_mdc_chan(chan);
-	struct mdc_tx_desc *mdesc;
 	unsigned long flags;
 	LIST_HEAD(head);
 
@@ -703,21 +702,28 @@ static int mdc_terminate_all(struct dma_chan *chan)
 	mdc_chan_writel(mchan, MDC_CONTROL_AND_STATUS_CANCEL,
 			MDC_CONTROL_AND_STATUS);
 
-	mdesc = mchan->desc;
-	mchan->desc = NULL;
+	if (mchan->desc) {
+		vchan_terminate_vdesc(&mchan->desc->vd);
+		mchan->desc = NULL;
+	}
 	vchan_get_all_descriptors(&mchan->vc, &head);
 
 	mdc_get_new_events(mchan);
 
 	spin_unlock_irqrestore(&mchan->vc.lock, flags);
 
-	if (mdesc)
-		mdc_desc_free(&mdesc->vd);
 	vchan_dma_desc_free_list(&mchan->vc, &head);
 
 	return 0;
 }
 
+static void mdc_synchronize(struct dma_chan *chan)
+{
+	struct mdc_chan *mchan = to_mdc_chan(chan);
+
+	vchan_synchronize(&mchan->vc);
+}
+
 static int mdc_slave_config(struct dma_chan *chan,
 			    struct dma_slave_config *config)
 {
@@ -952,6 +958,7 @@ static int mdc_dma_probe(struct platform_device *pdev)
 	mdma->dma_dev.device_tx_status = mdc_tx_status;
 	mdma->dma_dev.device_issue_pending = mdc_issue_pending;
 	mdma->dma_dev.device_terminate_all = mdc_terminate_all;
+	mdma->dma_dev.device_synchronize = mdc_synchronize;
 	mdma->dma_dev.device_config = mdc_slave_config;
 
 	mdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 09/10] dmaengine: k3dma: Use vchan_terminate_vdesc() instead of desc_free

[Peter Ujfalusi]

To avoid race with vchan_complete, use the race free way to terminate
running transfer.

Implement the device_synchronize callback to make sure that the terminated
descriptor is freed.

CC: Zhangfei Gao <zhangfei.gao@linaro.org>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/k3dma.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
index 01d2a750a621..26b67455208f 100644
--- a/drivers/dma/k3dma.c
+++ b/drivers/dma/k3dma.c
@@ -719,7 +719,7 @@ static int k3_dma_terminate_all(struct dma_chan *chan)
 		c->phy = NULL;
 		p->vchan = NULL;
 		if (p->ds_run) {
-			k3_dma_free_desc(&p->ds_run->vd);
+			vchan_terminate_vdesc(&p->ds_run->vd);
 			p->ds_run = NULL;
 		}
 		p->ds_done = NULL;
@@ -730,6 +730,13 @@ static int k3_dma_terminate_all(struct dma_chan *chan)
 	return 0;
 }
 
+static void k3_dma_synchronize(struct dma_chan *chan)
+{
+	struct k3_dma_chan *c = to_k3_chan(chan);
+
+	vchan_synchronize(&c->vc);
+}
+
 static int k3_dma_transfer_pause(struct dma_chan *chan)
 {
 	struct k3_dma_chan *c = to_k3_chan(chan);
@@ -868,6 +875,7 @@ static int k3_dma_probe(struct platform_device *op)
 	d->slave.device_pause = k3_dma_transfer_pause;
 	d->slave.device_resume = k3_dma_transfer_resume;
 	d->slave.device_terminate_all = k3_dma_terminate_all;
+	d->slave.device_synchronize = k3_dma_synchronize;
 	d->slave.copy_align = DMAENGINE_ALIGN_8_BYTES;
 
 	/* init virtual channel */
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 09/10] dmaengine: k3dma: Use vchan_terminate_vdesc() instead of desc_free

[zhangfei]

On 2017?11?14? 22:32, Peter Ujfalusi wrote:
> To avoid race with vchan_complete, use the race free way to terminate
> running transfer.
>
> Implement the device_synchronize callback to make sure that the terminated
> descriptor is freed.
>
> CC: Zhangfei Gao <zhangfei.gao@linaro.org>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Thanks Peter.

Acked-by: Zhangfei Gao <zhangfei.gao@linaro.org>

[PATCH 10/10] dmaengine: s3c24xx-dma: Use vchan_terminate_vdesc() instead of desc_free

[Peter Ujfalusi]

To avoid race with vchan_complete, use the race free way to terminate
running transfer.

Implement the device_synchronize callback to make sure that the terminated
descriptor is freed.

CC: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/s3c24xx-dma.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/s3c24xx-dma.c b/drivers/dma/s3c24xx-dma.c
index f04c4702d98b..cd92d696bcf9 100644
--- a/drivers/dma/s3c24xx-dma.c
+++ b/drivers/dma/s3c24xx-dma.c
@@ -732,7 +732,7 @@ static int s3c24xx_dma_terminate_all(struct dma_chan *chan)
 
 	/* Dequeue current job */
 	if (s3cchan->at) {
-		s3c24xx_dma_desc_free(&s3cchan->at->vd);
+		vchan_terminate_vdesc(&s3cchan->at->vd);
 		s3cchan->at = NULL;
 	}
 
@@ -744,6 +744,13 @@ static int s3c24xx_dma_terminate_all(struct dma_chan *chan)
 	return ret;
 }
 
+static void s3c24xx_dma_synchronize(struct dma_chan *chan)
+{
+	struct s3c24xx_dma_chan *s3cchan = to_s3c24xx_dma_chan(chan);
+
+	vchan_synchronize(&s3cchan->vc);
+}
+
 static void s3c24xx_dma_free_chan_resources(struct dma_chan *chan)
 {
 	/* Ensure all queued descriptors are freed */
@@ -1282,6 +1289,7 @@ static int s3c24xx_dma_probe(struct platform_device *pdev)
 	s3cdma->memcpy.device_issue_pending = s3c24xx_dma_issue_pending;
 	s3cdma->memcpy.device_config = s3c24xx_dma_set_runtime_config;
 	s3cdma->memcpy.device_terminate_all = s3c24xx_dma_terminate_all;
+	s3cdma->memcpy.device_synchronize = s3c24xx_dma_synchronize;
 
 	/* Initialize slave engine for SoC internal dedicated peripherals */
 	dma_cap_set(DMA_SLAVE, s3cdma->slave.cap_mask);
@@ -1296,6 +1304,7 @@ static int s3c24xx_dma_probe(struct platform_device *pdev)
 	s3cdma->slave.device_prep_dma_cyclic = s3c24xx_dma_prep_dma_cyclic;
 	s3cdma->slave.device_config = s3c24xx_dma_set_runtime_config;
 	s3cdma->slave.device_terminate_all = s3c24xx_dma_terminate_all;
+	s3cdma->slave.device_synchronize = s3c24xx_dma_synchronize;
 	s3cdma->slave.filter.map = pdata->slave_map;
 	s3cdma->slave.filter.mapcnt = pdata->slavecnt;
 	s3cdma->slave.filter.fn = s3c24xx_dma_filter;
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 00/10] dmaengine: fix race with vchan_complete

[Peter Ujfalusi]

Hi,

Russell, I have missed you from the To: list of this series by mistake,
I'm sorry about that.

- P?ter

On 2017-11-14 16:32, Peter Ujfalusi wrote:
> Hi,
> 
> With the introduction of .device_synchronize callback it was thought that the
> race caused crash observed in vchan_complete is fixed, but unfortunately it can
> still happen.
> 
> The observed scenario (really hard to reproduce) is:
> Cyclic mode
> - DMA period interrupt
>  - call to vchan_cyclic_callback() which sets vc->cyclic to vd and schedules the
>    vchan_complete tasklet
> - .terminate_all is called
>  - we make sure that no further DMA irqs are going to be handled, but the
>    tasklet had been already scheduled
>  - we free up the descriptor to avoid leaking memory
> - the vchan_complete tasklet starts to execute and it checks vc->cyclic, which
>   is not NULL, saves the vd pointer (which points to an already freed up memory)
>   and try to access it later to call the callback
> - the tasklet_kill() in .device_synchronize will make sure that the tasklet is
>   going to finish it's execution if it is already scheduled, it can only help if
>   the tasklet is yet to be executed.
> 
> At this point it is just matter of luck if the vc->cyclic is still pointing to
> an unchanged memory location or it is taken into use and thus it is corrupted.
> 
> My first approach was to just set vc->cyclic to NULL in the .terminate_all
> callback, but that still have theoritical race: if the vchan_complete is
> executing and it saves the vd from vc->cyclic (protected by the vc->lock). If at
> that point the .terminate_all is called it will wait for the lock and start
> executing. _if_ the .terminate_all free up the vd before the vchan_complete
> reaches the point when it is going to call the callback, then we have the race.
> 
> The series will do this:
> - the drivers should call vchan_terminate_vdesc() instead of directly freeing up
>   the descriptor. vchan_terminate_vdesc() will save the vd as vc->vd_terminated
>   and will set the vc->cyclic to NULL, all while holding the lock.
> - the drivers must implement the .device_synchronize callback and within the
>   vchan_synchronize() we free up the vc->vd_terminated after we killed the
>   tasklet.
> 
> I have tested this on platforms using TI's eDMA and sDMA and have not seen any
> side effect so far and a client tested similar set on a setup where it was easy
> to reproduce the race.
> 
> By looking for similar patterns in other drivers I have implemented the fix for
> the ones where it looked straight forward.
> 
> Regards,
> Peter
> ---
> Peter Ujfalusi (10):
>   dmaengine: virt-dma: Add helper to free/reuse a descriptor
>   dmaengine: virt-dma: Support for race free transfer termination
>   dmaengine: omap-dma: Use vchan_terminate_vdesc() instead of desc_free
>   dmaengine: edma: Use vchan_terminate_vdesc() instead of desc_free
>   dmaengine: bcm2835-dma: Use vchan_terminate_vdesc() instead of
>     desc_free
>   dmaengine: dma-jz4780: Use vchan_terminate_vdesc() instead of
>     desc_free
>   dmaengine: amba-pl08x: Use vchan_terminate_vdesc() instead of
>     desc_free
>   dmaengine: img-mdc-dma: Use vchan_terminate_vdesc() instead of
>     desc_free
>   dmaengine: k3dma: Use vchan_terminate_vdesc() instead of desc_free
>   dmaengine: s3c24xx-dma: Use vchan_terminate_vdesc() instead of
>     desc_free
> 
>  drivers/dma/amba-pl08x.c  | 11 ++++++++++-
>  drivers/dma/bcm2835-dma.c | 10 +++++++++-
>  drivers/dma/dma-jz4780.c  | 10 +++++++++-
>  drivers/dma/edma.c        |  7 ++-----
>  drivers/dma/img-mdc-dma.c | 17 ++++++++++++-----
>  drivers/dma/k3dma.c       | 10 +++++++++-
>  drivers/dma/omap-dma.c    |  2 +-
>  drivers/dma/s3c24xx-dma.c | 11 ++++++++++-
>  drivers/dma/virt-dma.c    |  5 +----
>  drivers/dma/virt-dma.h    | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  10 files changed, 107 insertions(+), 20 deletions(-)
> 

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 00/10] dmaengine: fix race with vchan_complete

[Vinod Koul]

On Tue, Nov 14, 2017 at 04:32:02PM +0200, Peter Ujfalusi wrote:
> Hi,
> 
> With the introduction of .device_synchronize callback it was thought that the
> race caused crash observed in vchan_complete is fixed, but unfortunately it can
> still happen.
> 
> The observed scenario (really hard to reproduce) is:
> Cyclic mode
> - DMA period interrupt
>  - call to vchan_cyclic_callback() which sets vc->cyclic to vd and schedules the
>    vchan_complete tasklet
> - .terminate_all is called
>  - we make sure that no further DMA irqs are going to be handled, but the
>    tasklet had been already scheduled
>  - we free up the descriptor to avoid leaking memory
> - the vchan_complete tasklet starts to execute and it checks vc->cyclic, which
>   is not NULL, saves the vd pointer (which points to an already freed up memory)
>   and try to access it later to call the callback
> - the tasklet_kill() in .device_synchronize will make sure that the tasklet is
>   going to finish it's execution if it is already scheduled, it can only help if
>   the tasklet is yet to be executed.
> 
> At this point it is just matter of luck if the vc->cyclic is still pointing to
> an unchanged memory location or it is taken into use and thus it is corrupted.
> 
> My first approach was to just set vc->cyclic to NULL in the .terminate_all
> callback, but that still have theoritical race: if the vchan_complete is
> executing and it saves the vd from vc->cyclic (protected by the vc->lock). If at
> that point the .terminate_all is called it will wait for the lock and start
> executing. _if_ the .terminate_all free up the vd before the vchan_complete
> reaches the point when it is going to call the callback, then we have the race.
> 
> The series will do this:
> - the drivers should call vchan_terminate_vdesc() instead of directly freeing up
>   the descriptor. vchan_terminate_vdesc() will save the vd as vc->vd_terminated
>   and will set the vc->cyclic to NULL, all while holding the lock.
> - the drivers must implement the .device_synchronize callback and within the
>   vchan_synchronize() we free up the vc->vd_terminated after we killed the
>   tasklet.
> 
> I have tested this on platforms using TI's eDMA and sDMA and have not seen any
> side effect so far and a client tested similar set on a setup where it was easy
> to reproduce the race.
> 
> By looking for similar patterns in other drivers I have implemented the fix for
> the ones where it looked straight forward.

Applied now, thanks for the cleanup! And looks like mail servers are fixed

-- 
~Vinod