On Thu, 23 Mar 2017 19:39:55 +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 calls snd_pcm_stream_lock, player->substream is already null.
>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxx>
>
> ---
> V2: Add spinlock to protect player/reader->substream
> ---
> sound/soc/sti/uniperif.h | 1 +
> sound/soc/sti/uniperif_player.c | 34 +++++++++++++++++++++++-----------
> sound/soc/sti/uniperif_reader.c | 24 ++++++++++++++++++++----
> 3 files changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/sound/soc/sti/uniperif.h b/sound/soc/sti/uniperif.h
> index d487dd2..cfcb0ea 100644
> --- a/sound/soc/sti/uniperif.h
> +++ b/sound/soc/sti/uniperif.h
> @@ -1299,6 +1299,7 @@ struct uniperif {
> int ver; /* IP version, used by register access macros */
> struct regmap_field *clk_sel;
> struct regmap_field *valid_sel;
> + spinlock_t irq_lock; /* used to prevent race condition with IRQ */
>
> /* capabilities */
> const struct snd_pcm_hardware *hw;
> diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
> index 60ae31a..8f3a582 100644
> --- a/sound/soc/sti/uniperif_player.c
> +++ b/sound/soc/sti/uniperif_player.c
> @@ -65,10 +65,13 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id)
> unsigned int status;
> unsigned int tmp;
>
> - if (player->state == UNIPERIF_STATE_STOPPED) {
> - /* Unexpected IRQ: do nothing */
> - return IRQ_NONE;
> - }
> + spin_lock(&player->irq_lock);
> + if (!player->substream)
> + goto IRQ_END;
This will be NULL-dereference, as it calls snd_pcm_stream_unlock() there.
Also, use lower letters for labels.
> +
> + snd_pcm_stream_lock(player->substream);
Actually we don't need to take this lock here any longer since we have
irq_lock. Better to keep the stream lock only around the stop.
> + if (player->state == UNIPERIF_STATE_STOPPED)
> + goto IRQ_END;
>
> /* Get interrupt status & clear them immediately */
> status = GET_UNIPERIF_ITS(player);
> @@ -88,9 +91,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);
BTW, these three calls can be simplified with snd_pcm_stop_xrun().
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
