[alsa-devel] [PATCH] ALSA: PCM: check if ops are defined before suspending PCM

[Takashi Iwai]

On Mon, 11 Feb 2019 16:41:31 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> On 2/9/19 3:27 AM, Takashi Iwai wrote:
> > On Sat, 09 Feb 2019 00:29:53 +0100,
> > Pierre-Louis Bossart wrote:
> >> From: Ranjani Sridharan <ranjani.sridharan at linux.intel.com>
> >>
> >> BE dai links only have internal PCM's and their substream ops may
> >> not be set. Suspending these PCM's will result in their
> >>   ops->trigger() being invoked and cause a kernel oops.
> >> So skip suspending PCM's if their ops are NULL.
> >>
> >> Signed-off-by: Ranjani Sridharan <ranjani.sridharan at linux.intel.com>
> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> >> ---
> >>   sound/core/pcm_native.c | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> >> index 818dff1de545..b6e158ce6650 100644
> >> --- a/sound/core/pcm_native.c
> >> +++ b/sound/core/pcm_native.c
> >> @@ -1506,6 +1506,14 @@ int snd_pcm_suspend_all(struct snd_pcm *pcm)
> >>   			/* FIXME: the open/close code should lock this as well */
> >>   			if (substream->runtime == NULL)
> >>   				continue;
> >> +
> >> +			/*
> >> +			 * Skip BE dai link PCM's that are internal and may
> >> +			 * not have their substream ops set.
> >> +			 */
> >> +			if (!substream->ops)
> >> +				continue;
> >> +
> >>   			err = snd_pcm_suspend(substream);
> >>   			if (err < 0 && err != -EBUSY)
> >>   				return err;
> > Basically it's OK and safe to apply this check.  We may need to add
> > such sanity checks in more places if this really hits.
> >
> > But I still wonder how this can go through.  Is substream->runtime set
> > even if substream->ops is NULL?  The substream->runtime is assigned
> > dynamically at opening a substream via snd_pcm_attach_substream(), so
> > without opening it, it must be NULL.
> 
> This error case was exposed when we tried to get rid of
> snd_pcm_suspend() per your recommendation, and use snd_soc_suspend()
> instead to do the work for us.
> 
> In the case of back-ends, all initializations are bypassed in
> soc_new_pcm() - see below a code snippet - and the ops aren't set
> before suspend is called.
> The complete thread where we discussed this is at
> https://github.com/thesofproject/linux/pull/582

Thanks, now I took a look at the code.  And, this surfaced that the
another part of the problem is that DPCM does the substream open
handling by itself in soc-pcm.c.  Oh well.  I'm afraid that we have
some hidden bugs there that may lead to a crash easily.  (Fortunately
(or unfortunately) fuzzer isn't performed on ASoC because we have no
virtual device driver :)

IMO, some of DPCM code should be raised to the upper level, to ALSA
PCM core.  The current code is still in a rough form of early
plumbing.

In anyway, I merged the patch now with a bit more comments.


Thanks!

Takashi