| Message ID | 1445334391-12272-5-git-send-email-lars@metafoo.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > >
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)
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
[...] > 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.
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);
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(-)