[PATCH 0/4] dmaengine: Add transfer termination synchronization support

[PATCH 0/4] dmaengine: Add transfer termination synchronization support

[Lars-Peter Clausen]

The DMAengine API has a long standing issue that is inherent to the API
itself. For a client that calls dmaengine_terminate_all() it is not
possible to properly synchronize the completion of any currently running
complete callbacks to the current context. This means it is possible to end
up with a use-after-free race condition if client frees resources that are
accessed in a complete callback before the complete callback has finished
running.

This patch series introduces a new explicit synchronization primitive to
the DMAengine API which allows clients to ensure that all complete
callbacks have finished running. This allows them to safely free any
resources that might be accessed in a complete callback.

The series for now only implements synchronization support for a single
driver and only updates single client to make use of the new API. If there
is agreement on the general approach more will follow.

- Lars

Lars-Peter Clausen (4):
  dmaengine: Add transfer termination synchronization support
  dmaengine: virt-dma: Add synchronization helper function
  dmaengine: axi_dmac: Add synchronization support
  ALSA: pcm_dmaengine: Properly synchronize DMA on shutdown

 Documentation/dmaengine/client.txt   | 38 ++++++++++++++-
 Documentation/dmaengine/provider.txt | 20 +++++++-
 drivers/dma/dma-axi-dmac.c           |  8 ++++
 drivers/dma/dmaengine.c              |  5 +-
 drivers/dma/virt-dma.h               | 13 ++++++
 include/linux/dmaengine.h            | 90 ++++++++++++++++++++++++++++++++++++
 sound/core/pcm_dmaengine.c           |  9 ++--
 7 files changed, 175 insertions(+), 8 deletions(-)

-- 
2.1.4

[PATCH 1/4] dmaengine: Add transfer termination synchronization support

[Lars-Peter Clausen]

The DMAengine API has a long standing race condition that is inherent to
the API itself. Calling dmaengine_terminate_all() is supposed to stop and
abort any pending or active transfers that have previously been submitted.
Unfortunately it is possible that this operation races against a currently
running (or with some drivers also scheduled) completion callback.

Since the API allows dmaengine_terminate_all() to be called from atomic
context as well as from within a completion callback it is not possible to
synchronize to the execution of the completion callback from within
dmaengine_terminate_all() itself.

This means that a user of the DMAengine API does not know when it is safe
to free resources used in the completion callback, which can result in a
use-after-free race condition.

This patch addresses the issue by introducing an explicit synchronization
primitive to the DMAengine API called dmaengine_synchronize().

The existing dmaengine_terminate_all() is deprecated in favor of
dmaengine_terminate_sync() and dmaengine_terminate_async(). The former
aborts all pending and active transfers and synchronizes to the current
context, meaning it will wait until all running completion callbacks have
finished. This means it is only possible to call this function from
non-atomic context. The later function does not synchronize, but can still
be used in atomic context or from within a complete callback. It has to be
followed up by dmaengine_synchronize() before a client can free the
resources used in a completion callback.

In addition to this the semantics of the device_terminate_all() callback
are slightly relaxed by this patch. It is now OK for a driver to only
schedule the termination of the active transfer, but does not necessarily
have to wait until the DMA controller has completely stopped. The driver
must ensure though that the controller has stopped and no longer accesses
any memory when the device_synchronize() callback returns.

This was in part done since most drivers do not pay attention to this
anyway at the moment and to emphasize that this needs to be done when the
device_synchronize() callback is implemented. But it also helps with
implementing support for devices where stopping the controller can require
operations that may sleep.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 Documentation/dmaengine/client.txt   | 38 ++++++++++++++-
 Documentation/dmaengine/provider.txt | 20 +++++++-
 drivers/dma/dmaengine.c              |  5 +-
 include/linux/dmaengine.h            | 90 ++++++++++++++++++++++++++++++++++++
 4 files changed, 148 insertions(+), 5 deletions(-)

diff --git a/Documentation/dmaengine/client.txt b/Documentation/dmaengine/client.txt
index 11fb87f..d9f9f46 100644
--- a/Documentation/dmaengine/client.txt
+++ b/Documentation/dmaengine/client.txt
@@ -128,7 +128,7 @@ The slave DMA usage consists of following steps:
 	transaction.
 
 	For cyclic DMA, a callback function may wish to terminate the
-	DMA via dmaengine_terminate_all().
+	DMA via dmaengine_terminate_async().
 
 	Therefore, it is important that DMA engine drivers drop any
 	locks before calling the callback function which may cause a
@@ -166,12 +166,29 @@ The slave DMA usage consists of following steps:
 
 Further APIs:
 
-1. int dmaengine_terminate_all(struct dma_chan *chan)
+1. int dmaengine_terminate_sync(struct dma_chan *chan)
+   int dmaengine_terminate_async(struct dma_chan *chan)
+   int dmaengine_terminate_all(struct dma_chan *chan) /* DEPRECATED */
 
    This causes all activity for the DMA channel to be stopped, and may
    discard data in the DMA FIFO which hasn't been fully transferred.
    No callback functions will be called for any incomplete transfers.
 
+   Two variants of this function are available.
+
+   dmaengine_terminate_async() might not wait until the DMA has been fully
+   stopped or until any running complete callbacks have finished. But it is
+   possible to call dmaengine_terminate_async() from atomic context or from
+   within a complete callback. dmaengine_synchronize() must be called before it
+   is safe to free the memory accessed by the DMA transfer or free resources
+   accessed from within the complete callback.
+
+   dmaengine_terminate_sync() will wait for the transfer and any running
+   complete callbacks to finish before it returns. But the function must not be
+   called from atomic context or from within a complete callback.
+
+   dmaengine_terminate_all() is deprecated and should not be used in new code.
+
 2. int dmaengine_pause(struct dma_chan *chan)
 
    This pauses activity on the DMA channel without data loss.
@@ -197,3 +214,20 @@ Further APIs:
 	a running DMA channel.  It is recommended that DMA engine users
 	pause or stop (via dmaengine_terminate_all()) the channel before
 	using this API.
+
+5. void dmaengine_synchronize(struct dma_chan *chan)
+
+  Synchronize the termination of the DMA channel to the current context.
+
+  This function should be used after dmaengine_terminate_async() to synchronize
+  the termination of the DMA channel to the current context. The function will
+  wait for the transfer and any running complete callbacks to finish before it
+  returns.
+
+  If dmaengine_terminate_async() is used to stop the DMA channel this function
+  must be called before it is safe to free memory accessed by previously
+  submitted descriptors or to free any resources accessed within the complete
+  callback of previously submitted descriptors.
+
+  The behavior of this function is undefined if dma_async_issue_pending() has
+  been called between dmaengine_terminate_async() and this function.
diff --git a/Documentation/dmaengine/provider.txt b/Documentation/dmaengine/provider.txt
index 67d4ce4..122b7f4 100644
--- a/Documentation/dmaengine/provider.txt
+++ b/Documentation/dmaengine/provider.txt
@@ -327,8 +327,24 @@ supported.
 
    * device_terminate_all
      - Aborts all the pending and ongoing transfers on the channel
-     - This command should operate synchronously on the channel,
-       terminating right away all the channels
+     - For aborted transfers the complete callback should not be called
+     - Can be called from atomic context or from within a complete
+       callback of a descriptor. Must not sleep. Drivers must be able
+       to handle this correctly.
+     - Termination may be asynchronous. The driver does not have to
+       wait until the currently active transfer has completely stopped.
+       See device_synchronize.
+
+   * device_synchronize
+     - Must synchronize the termination of a channel to the current
+       context.
+     - Must make sure that memory for previously submitted
+       descriptors is no longer accessed by the DMA controller.
+     - Must make sure that all complete callbacks for previously
+       submitted descriptors have finished running and none are
+       scheduled to run.
+     - May sleep.
+
 
 Misc notes (stuff that should be documented, but don't really know
 where to put them)
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 3ecec14..d6fc82e 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -265,8 +265,11 @@ static void dma_chan_put(struct dma_chan *chan)
 	module_put(dma_chan_to_owner(chan));
 
 	/* This channel is not in use anymore, free it */
-	if (!chan->client_count && chan->device->device_free_chan_resources)
+	if (!chan->client_count && chan->device->device_free_chan_resources) {
+		/* Make sure all operations have completed */
+		dmaengine_synchronize(chan);
 		chan->device->device_free_chan_resources(chan);
+	}
 
 	/* If the channel is used via a DMA request router, free the mapping */
 	if (chan->router && chan->router->route_free) {
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 7ea9184..4cb5087 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -653,6 +653,8 @@ enum dmaengine_alignment {
  *	paused. Returns 0 or an error code
  * @device_terminate_all: Aborts all transfers on a channel. Returns 0
  *	or an error code
+ * @device_synchronize: Synchronizes the termination of a transfers to the
+ *  current context.
  * @device_tx_status: poll for transaction completion, the optional
  *	txstate parameter can be supplied with a pointer to get a
  *	struct with auxiliary transfer status information, otherwise the call
@@ -733,6 +735,7 @@ struct dma_device {
 	int (*device_pause)(struct dma_chan *chan);
 	int (*device_resume)(struct dma_chan *chan);
 	int (*device_terminate_all)(struct dma_chan *chan);
+	void (*device_synchronize)(struct dma_chan *chan);
 
 	enum dma_status (*device_tx_status)(struct dma_chan *chan,
 					    dma_cookie_t cookie,
@@ -824,6 +827,13 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_sg(
 			src_sg, src_nents, flags);
 }
 
+/**
+ * dmaengine_terminate_all() - Terminate all active DMA transfers
+ * @chan: The channel for which to terminate the transfers
+ *
+ * This function is DEPRECATED use either dmaengine_terminate_sync() or
+ * dmaengine_terminate_async() instead.
+ */
 static inline int dmaengine_terminate_all(struct dma_chan *chan)
 {
 	if (chan->device->device_terminate_all)
@@ -832,6 +842,86 @@ static inline int dmaengine_terminate_all(struct dma_chan *chan)
 	return -ENOSYS;
 }
 
+/**
+ * dmaengine_terminate_async() - Terminate all active DMA transfers
+ * @chan: The channel for which to terminate the transfers
+ *
+ * Calling this function will terminate all active and pending descriptors
+ * that have previously been submitted to the channel. It is not guaranteed
+ * though that the transfer for the active descriptor has stopped when the
+ * function returns. Furthermore it is possible the complete callback of a
+ * submitted transfer is still running when this function returns.
+ *
+ * dmaengine_synchronize() needs to be called before it is safe to free
+ * any memory that is accessed by previously submitted descriptors or before
+ * freeing any resources accessed from within the completion callback of any
+ * perviously submitted descriptors.
+ *
+ * This function can be called from atomic context as well as from within a
+ * complete callback of a descriptor submitted on the same channel.
+ *
+ * If none of the two conditions above apply consider using
+ * dmaengine_terminate_sync() instead.
+ */
+static inline int dmaengine_terminate_async(struct dma_chan *chan)
+{
+	if (chan->device->device_terminate_all)
+		return chan->device->device_terminate_all(chan);
+
+	return -EINVAL;
+}
+
+/**
+ * dmaengine_synchronize() - Synchronize DMA channel termination
+ * @chan: The channel to synchronize
+ *
+ * Synchronizes to the DMA channel termination to the current context. When this
+ * function returns it is guaranteed that all transfers for previously issued
+ * descriptors have stopped and and it is safe to free the memory assoicated
+ * with them. Furthermore it is guaranteed that all complete callback functions
+ * for a previously submitted descriptor have finished running and it is safe to
+ * free resources accessed from within the complete callbacks.
+ *
+ * The behavior of this function is undefined if dma_async_issue_pending() has
+ * been called between dmaengine_terminate_async() and this function.
+ *
+ * This function must only be called from non-atomic context and must not be
+ * called from within a complete callback of a descriptor submitted on the same
+ * channel.
+ */
+static inline void dmaengine_synchronize(struct dma_chan *chan)
+{
+	if (chan->device->device_synchronize)
+		chan->device->device_synchronize(chan);
+}
+
+/**
+ * dmaengine_terminate_sync() - Terminate all active DMA transfers
+ * @chan: The channel for which to terminate the transfers
+ *
+ * Calling this function will terminate all active and pending transfers
+ * that have previously been submitted to the channel. It is similar to
+ * dmaengine_terminate_async() but guarantees that the DMA transfer has actually
+ * stopped and that all complete callbacks have finished running when the
+ * function returns.
+ *
+ * This function must only be called from non-atomic context and must not be
+ * called from within a complete callback of a descriptor submitted on the same
+ * channel.
+ */
+static inline int dmaengine_terminate_sync(struct dma_chan *chan)
+{
+	int ret;
+
+	ret = dmaengine_terminate_async(chan);
+	if (ret)
+		return ret;
+
+	dmaengine_synchronize(chan);
+
+	return 0;
+}
+
 static inline int dmaengine_pause(struct dma_chan *chan)
 {
 	if (chan->device->device_pause)
-- 
2.1.4

Re: [PATCH 1/4] dmaengine: Add transfer termination synchronization support

[Andy Shevchenko]

On Tue, Oct 20, 2015 at 12:46 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> The DMAengine API has a long standing race condition that is inherent to
> the API itself. Calling dmaengine_terminate_all() is supposed to stop and
> abort any pending or active transfers that have previously been submitted.
> Unfortunately it is possible that this operation races against a currently
> running (or with some drivers also scheduled) completion callback.

[]

> +/**
> + * dmaengine_terminate_sync() - Terminate all active DMA transfers
> + * @chan: The channel for which to terminate the transfers
> + *
> + * Calling this function will terminate all active and pending transfers
> + * that have previously been submitted to the channel. It is similar to
> + * dmaengine_terminate_async() but guarantees that the DMA transfer has actually
> + * stopped and that all complete callbacks have finished running when the
> + * function returns.
> + *
> + * This function must only be called from non-atomic context and must not be
> + * called from within a complete callback of a descriptor submitted on the same
> + * channel.
> + */
> +static inline int dmaengine_terminate_sync(struct dma_chan *chan)
> +{
> +       int ret;

Might be a good idea to add might_sleep(); here.

> +
> +       ret = dmaengine_terminate_async(chan);
> +       if (ret)
> +               return ret;
> +
> +       dmaengine_synchronize(chan);
> +
> +       return 0;
> +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] dmaengine: Add transfer termination synchronization support

[Lars-Peter Clausen]

On 10/29/2015 10:59 PM, Andy Shevchenko wrote:
> On Tue, Oct 20, 2015 at 12:46 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> The DMAengine API has a long standing race condition that is inherent to
>> the API itself. Calling dmaengine_terminate_all() is supposed to stop and
>> abort any pending or active transfers that have previously been submitted.
>> Unfortunately it is possible that this operation races against a currently
>> running (or with some drivers also scheduled) completion callback.
> 
> []
> 
>> +/**
>> + * dmaengine_terminate_sync() - Terminate all active DMA transfers
>> + * @chan: The channel for which to terminate the transfers
>> + *
>> + * Calling this function will terminate all active and pending transfers
>> + * that have previously been submitted to the channel. It is similar to
>> + * dmaengine_terminate_async() but guarantees that the DMA transfer has actually
>> + * stopped and that all complete callbacks have finished running when the
>> + * function returns.
>> + *
>> + * This function must only be called from non-atomic context and must not be
>> + * called from within a complete callback of a descriptor submitted on the same
>> + * channel.
>> + */
>> +static inline int dmaengine_terminate_sync(struct dma_chan *chan)
>> +{
>> +       int ret;
> 
> Might be a good idea to add might_sleep(); here.

Sound like a good idea, thanks, I'll add it (in dmaengine_synchronize() though).

> 
>> +
>> +       ret = dmaengine_terminate_async(chan);
>> +       if (ret)
>> +               return ret;
>> +
>> +       dmaengine_synchronize(chan);
>> +
>> +       return 0;
>> +}
> 
> 

[PATCH 2/4] dmaengine: virt-dma: Add synchronization helper function

[Lars-Peter Clausen]

Add a synchronize helper function for the virt-dma library. The function
makes sure that any scheduled descriptor complete callbacks have finished
running before the function returns.

This needs to be called by drivers using virt-dma in their
device_synchronize() callback. Depending on the driver additional
operations might be necessary in addition to calling vchan_synchronize() to
ensure proper synchronization.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/dma/virt-dma.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h
index 181b9526..e93de76 100644
--- a/drivers/dma/virt-dma.h
+++ b/drivers/dma/virt-dma.h
@@ -151,4 +151,17 @@ static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
 	vchan_dma_desc_free_list(vc, &head);
 }
 
+/**
+ * vchan_synchronize() - synchronize callback execution to the current context
+ * @vc: virtual channel to synchronize
+ *
+ * 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.
+ */
+static inline void vchan_synchronize(struct virt_dma_chan *vc)
+{
+	tasklet_kill(&vc->task);
+}
+
 #endif
-- 
2.1.4

[PATCH 3/4] dmaengine: axi_dmac: Add synchronization support

[Lars-Peter Clausen]

Implement the new device_synchronize() callback to allow proper
synchronization when stopping a channel. Since the driver already makes
sure that no new complete callbacks are scheduled after the
device_terminate_all() callback has been called, all left to do in the
device_synchronize() callback is to wait for all currently running complete
callbacks to finish.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/dma/dma-axi-dmac.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
index 5b2395e..c346809 100644
--- a/drivers/dma/dma-axi-dmac.c
+++ b/drivers/dma/dma-axi-dmac.c
@@ -307,6 +307,13 @@ static int axi_dmac_terminate_all(struct dma_chan *c)
 	return 0;
 }
 
+static void axi_dmac_synchronize(struct dma_chan *c)
+{
+	struct axi_dmac_chan *chan = to_axi_dmac_chan(c);
+
+	vchan_synchronize(&chan->vchan);
+}
+
 static void axi_dmac_issue_pending(struct dma_chan *c)
 {
 	struct axi_dmac_chan *chan = to_axi_dmac_chan(c);
@@ -613,6 +620,7 @@ static int axi_dmac_probe(struct platform_device *pdev)
 	dma_dev->device_prep_dma_cyclic = axi_dmac_prep_dma_cyclic;
 	dma_dev->device_prep_interleaved_dma = axi_dmac_prep_interleaved;
 	dma_dev->device_terminate_all = axi_dmac_terminate_all;
+	dma_dev->device_synchronize = axi_dmac_synchronize;
 	dma_dev->dev = &pdev->dev;
 	dma_dev->chancnt = 1;
 	dma_dev->src_addr_widths = BIT(dmac->chan.src_width);
-- 
2.1.4

[PATCH 4/4] ALSA: pcm_dmaengine: Properly synchronize DMA on shutdown

[Lars-Peter Clausen]

Use the new dmaengine_synchronize() function to make sure that all complete
callbacks have finished running before the runtime data, which is accessed
in the completed callback, is freed.

This fixes a long standing use-after-free race condition that has been
observed on some systems.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 sound/core/pcm_dmaengine.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
index fba365a..697c166 100644
--- a/sound/core/pcm_dmaengine.c
+++ b/sound/core/pcm_dmaengine.c
@@ -202,13 +202,13 @@ int snd_dmaengine_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 		if (runtime->info & SNDRV_PCM_INFO_PAUSE)
 			dmaengine_pause(prtd->dma_chan);
 		else
-			dmaengine_terminate_all(prtd->dma_chan);
+			dmaengine_terminate_async(prtd->dma_chan);
 		break;
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		dmaengine_pause(prtd->dma_chan);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
-		dmaengine_terminate_all(prtd->dma_chan);
+		dmaengine_terminate_async(prtd->dma_chan);
 		break;
 	default:
 		return -EINVAL;
@@ -346,6 +346,7 @@ int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream)
 {
 	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
 
+	dmaengine_synchronize(prtd->dma_chan);
 	kfree(prtd);
 
 	return 0;
@@ -362,9 +363,11 @@ int snd_dmaengine_pcm_close_release_chan(struct snd_pcm_substream *substream)
 {
 	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
 
+	dmaengine_synchronize(prtd->dma_chan);
 	dma_release_channel(prtd->dma_chan);
+	kfree(prtd);
 
-	return snd_dmaengine_pcm_close(substream);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_close_release_chan);
 
-- 
2.1.4

Re: [PATCH 4/4] ALSA: pcm_dmaengine: Properly synchronize DMA on shutdown

[Takashi Iwai]

On Tue, 20 Oct 2015 11:46:31 +0200,
Lars-Peter Clausen wrote:
> 
> Use the new dmaengine_synchronize() function to make sure that all complete
> callbacks have finished running before the runtime data, which is accessed
> in the completed callback, is freed.
> 
> This fixes a long standing use-after-free race condition that has been
> observed on some systems.

What if a substream is restarted immediately after the stop?


Takashi

> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  sound/core/pcm_dmaengine.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
> index fba365a..697c166 100644
> --- a/sound/core/pcm_dmaengine.c
> +++ b/sound/core/pcm_dmaengine.c
> @@ -202,13 +202,13 @@ int snd_dmaengine_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
>  		if (runtime->info & SNDRV_PCM_INFO_PAUSE)
>  			dmaengine_pause(prtd->dma_chan);
>  		else
> -			dmaengine_terminate_all(prtd->dma_chan);
> +			dmaengine_terminate_async(prtd->dma_chan);
>  		break;
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>  		dmaengine_pause(prtd->dma_chan);
>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:
> -		dmaengine_terminate_all(prtd->dma_chan);
> +		dmaengine_terminate_async(prtd->dma_chan);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -346,6 +346,7 @@ int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream)
>  {
>  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
>  
> +	dmaengine_synchronize(prtd->dma_chan);
>  	kfree(prtd);
>  
>  	return 0;
> @@ -362,9 +363,11 @@ int snd_dmaengine_pcm_close_release_chan(struct snd_pcm_substream *substream)
>  {
>  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
>  
> +	dmaengine_synchronize(prtd->dma_chan);
>  	dma_release_channel(prtd->dma_chan);
> +	kfree(prtd);
>  
> -	return snd_dmaengine_pcm_close(substream);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_close_release_chan);
>  
> -- 
> 2.1.4
> 
> 

Re: [PATCH 4/4] ALSA: pcm_dmaengine: Properly synchronize DMA on shutdown

[Lars-Peter Clausen]

On 10/20/2015 01:17 PM, Takashi Iwai wrote:
> On Tue, 20 Oct 2015 11:46:31 +0200,
> Lars-Peter Clausen wrote:
>>
>> Use the new dmaengine_synchronize() function to make sure that all complete
>> callbacks have finished running before the runtime data, which is accessed
>> in the completed callback, is freed.
>>
>> This fixes a long standing use-after-free race condition that has been
>> observed on some systems.
> 
> What if a substream is restarted immediately after the stop?
> 

What can happen is that you get a complete callback and the associated
snd_pcm_period_elapsed() too early, before the period has actually elapsed,
but I don't think that this is a problem if the DMA driver properly
implements residue reporting.

This fails if we rely on period counting, but that is broken anyway and
already prone to other race conditions.

I've tested this series with xrun injection and some modifications to the
DMA driver to always trigger the race condition when the stream is stopped.
And I've not seen any issues after the transfer re-started. (There is a
dead-lock condition though but that does not seem to be related to this series)

Re: [PATCH 4/4] ALSA: pcm_dmaengine: Properly synchronize DMA on shutdown

[Takashi Iwai]

On Tue, 20 Oct 2015 13:40:00 +0200,
Lars-Peter Clausen wrote:
> 
> On 10/20/2015 01:17 PM, Takashi Iwai wrote:
> > On Tue, 20 Oct 2015 11:46:31 +0200,
> > Lars-Peter Clausen wrote:
> >>
> >> Use the new dmaengine_synchronize() function to make sure that all complete
> >> callbacks have finished running before the runtime data, which is accessed
> >> in the completed callback, is freed.
> >>
> >> This fixes a long standing use-after-free race condition that has been
> >> observed on some systems.
> > 
> > What if a substream is restarted immediately after the stop?
> > 
> 
> What can happen is that you get a complete callback and the associated
> snd_pcm_period_elapsed() too early, before the period has actually elapsed,
> but I don't think that this is a problem if the DMA driver properly
> implements residue reporting.
> 
> This fails if we rely on period counting, but that is broken anyway and
> already prone to other race conditions.
> 
> I've tested this series with xrun injection and some modifications to the
> DMA driver to always trigger the race condition when the stream is stopped.
> And I've not seen any issues after the transfer re-started. (There is a
> dead-lock condition though but that does not seem to be related to this series)

OK, then I'm fine with the changes.

I suppose this will go through dmaengine tree?  If so, feel free to
take my ack:
  Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH 4/4] ALSA: pcm_dmaengine: Properly synchronize DMA on shutdown

[Lars-Peter Clausen]

[...]
> I've tested this series with xrun injection and some modifications to the
> DMA driver to always trigger the race condition when the stream is stopped.
> And I've not seen any issues after the transfer re-started. (There is a
> dead-lock condition though but that does not seem to be related to this series)

Turns out that was a bug in the DMA driver that caused
snd_pcm_period_elapsed() to be called on the TRIGGER_START path. So the ALSA
side seems to be good.

Re: [PATCH 0/4] dmaengine: Add transfer termination synchronization support

[Vinod Koul]

On Tue, Oct 20, 2015 at 11:46:27AM +0200, Lars-Peter Clausen wrote:
> The DMAengine API has a long standing issue that is inherent to the API
> itself. For a client that calls dmaengine_terminate_all() it is not
> possible to properly synchronize the completion of any currently running
> complete callbacks to the current context. This means it is possible to end
> up with a use-after-free race condition if client frees resources that are
> accessed in a complete callback before the complete callback has finished
> running.
> 
> This patch series introduces a new explicit synchronization primitive to
> the DMAengine API which allows clients to ensure that all complete
> callbacks have finished running. This allows them to safely free any
> resources that might be accessed in a complete callback.
> 
> The series for now only implements synchronization support for a single
> driver and only updates single client to make use of the new API. If there
> is agreement on the general approach more will follow.

Thanks Lars for getting this done.

I have merged this to topic/test/async branch and this will be in next
kernel revision. We are too close to merge window right now

-- 
~Vinod