On 03/21/2017 11:38 AM, Takashi Iwai wrote:
On Tue, 21 Mar 2017 11:32:51 +0100, Arnaud Pouliquen wrote:
On 03/20/2017 06:42 PM, Takashi Iwai wrote:
On Mon, 20 Mar 2017 18:09:15 +0100, Arnaud Pouliquen wrote:
With RTlinux a race condition has been found that leads to NULL ptr crash:
- On CPU 0: uni_player_irq_handler is called to treat XRUN
"(player->state == UNIPERIF_STATE_STOPPED)" is FALSE so status is checked, dev_err(player->dev, "FIFO underflow error detected") is printed and then snd_pcm_stream_lock should be called to lock stream for stopping.
- On CPU 1: application stop and close the stream.
Issue is that the stop and shutdown functions are executed while "FIFO underflow error detected" is printed. So when CPU 0 call snd_pcm_stream_lock is player->substream is already null.
Hrm, how the first call of snd_pcm_stream_lock() with player->substream to be guaranteed to be non-NULL?
Not sure that it is possible to be 100% save, but player->substream should not be null...
Use case at risk: Stop request occurs just before IRQ.
- IRQ handler locked on snd_pcm_stream_lock(player->substream)
- uni_player_stop clear IRQ flags and set player->state ==
UNIPERIF_STATE_STOPPED. So no more IRQs after UNIPERIF_STATE_STOPPED state. 3) As stream is cleared after close, i guess that between stop and close following IRQ handler code should be executed
if (player->state == UNIPERIF_STATE_STOPPED) { /* Unexpected IRQ: do nothing */ snd_pcm_stream_unlock(player->substream); return IRQ_NONE; }
Anyway adding a check of player->substream in IRQ handler before locking seems more save.
Well, if the PCM stop happens right after the call of uni_player_irq_handler() but just before the spinlock call, you may still hit NULL dereference.
For me this is not possible... please tell me if i missed something.
Substream is set to null in snd_pcm_release so after the close. That why i set player->substream to NULL in uni_player_shutdown that corresponds to DAI shutdown op. So should not be possible to race the null condition. Only way should be that stop and close is executed between The uni_player_irq_handler call and the spinlock.
That is, it's not safe to refer to player->substream unless we ensure non-NULL in other way.
Not use it seems not possible as usage is to call snd_pcm_stop on underrun.
Thanks
Arnaud
thanks,
Takashi
I will add it in a V2
Thanks
Arnaud
Takashi
Signed-off-by: Arnaud Pouliquen arnaud.pouliquen@st.com
sound/soc/sti/uniperif_player.c | 9 +++------ sound/soc/sti/uniperif_reader.c | 8 +++++--- 2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c index 60ae31a..6fa9eee 100644 --- a/sound/soc/sti/uniperif_player.c +++ b/sound/soc/sti/uniperif_player.c @@ -65,8 +65,10 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id) unsigned int status; unsigned int tmp;
- snd_pcm_stream_lock(player->substream); if (player->state == UNIPERIF_STATE_STOPPED) { /* Unexpected IRQ: do nothing */
snd_pcm_stream_unlock(player->substream);@@ -88,9 +90,7 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id) SET_UNIPERIF_ITM_BCLR_FIFO_ERROR(player);
/* Stop the player */
snd_pcm_stream_lock(player->substream); snd_pcm_stop(player->substream, SNDRV_PCM_STATE_XRUN);snd_pcm_stream_unlock(player->substream);}
ret = IRQ_HANDLED;
@@ -104,9 +104,7 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id) SET_UNIPERIF_ITM_BCLR_DMA_ERROR(player);
/* Stop the player */
snd_pcm_stream_lock(player->substream);snd_pcm_stop(player->substream, SNDRV_PCM_STATE_XRUN);
snd_pcm_stream_unlock(player->substream);ret = IRQ_HANDLED; }
@@ -138,13 +136,12 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id) dev_err(player->dev, "Underflow recovery failed\n");
/* Stop the player */
snd_pcm_stream_lock(player->substream);snd_pcm_stop(player->substream, SNDRV_PCM_STATE_XRUN);
snd_pcm_stream_unlock(player->substream);ret = IRQ_HANDLED; }
- snd_pcm_stream_unlock(player->substream); return ret;
}
diff --git a/sound/soc/sti/uniperif_reader.c b/sound/soc/sti/uniperif_reader.c index 5992c6a..9273f59 100644 --- a/sound/soc/sti/uniperif_reader.c +++ b/sound/soc/sti/uniperif_reader.c @@ -46,9 +46,11 @@ static irqreturn_t uni_reader_irq_handler(int irq, void *dev_id) struct uniperif *reader = dev_id; unsigned int status;
- snd_pcm_stream_lock(reader->substream); if (reader->state == UNIPERIF_STATE_STOPPED) { /* Unexpected IRQ: do nothing */ dev_warn(reader->dev, "unexpected IRQ\n");
snd_pcm_stream_unlock(reader->substream);@@ -60,13 +62,13 @@ static irqreturn_t uni_reader_irq_handler(int irq, void *dev_id) if (unlikely(status & UNIPERIF_ITS_FIFO_ERROR_MASK(reader))) { dev_err(reader->dev, "FIFO error detected\n");
snd_pcm_stream_lock(reader->substream);snd_pcm_stop(reader->substream, SNDRV_PCM_STATE_XRUN);
snd_pcm_stream_unlock(reader->substream);return IRQ_HANDLED;
ret = IRQ_HANDLED;}
snd_pcm_stream_unlock(reader->substream);
return ret;
}
-- 1.9.1