[PATCH] ASoC: soc-compress: Fix deadlock in soc_compr_open_fe

[PATCH] ASoC: soc-compress: Fix deadlock in soc_compr_open_fe

[yixuanjiang]

Modify the error handling flow by release lock.
The require pcm_mutex will keep holding if open fail.

Fixes: aa9ff6a4955f ("ASoC: soc-compress: Reposition and add pcm_mutex")
Signed-off-by: yixuanjiang <yixuanjiang@google.com>
Cc: stable@vger.kernel.org # v5.15+
---
 sound/soc/soc-compress.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 256e45001f85..b6727b91dd85 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -166,6 +166,7 @@ static int soc_compr_open_fe(struct snd_compr_stream *cstream)
 	snd_soc_dai_compr_shutdown(cpu_dai, cstream, 1);
 out:
 	dpcm_path_put(&list);
+	mutex_unlock(&fe->card->pcm_mutex);
 be_err:
 	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
 	mutex_unlock(&fe->card->mutex);
-- 
2.41.0.162.gfafddb0af9-goog

Re: [PATCH] ASoC: soc-compress: Fix deadlock in soc_compr_open_fe

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

On Tue, Jun 13, 2023 at 02:23:50PM +0800, yixuanjiang wrote:
> Modify the error handling flow by release lock.
> The require pcm_mutex will keep holding if open fail.

> +++ b/sound/soc/soc-compress.c
> @@ -166,6 +166,7 @@ static int soc_compr_open_fe(struct snd_compr_stream *cstream)
>  	snd_soc_dai_compr_shutdown(cpu_dai, cstream, 1);
>  out:
>  	dpcm_path_put(&list);
> +	mutex_unlock(&fe->card->pcm_mutex);
>  be_err:

This is really hard to follow due to the lack of any mutex_lock()s in
the function, I think because this is intended to undo
snd_soc_dpcm_mutex_lock(fe) but if that's the case why is it not using
snd_soc_dpcm_mutex_unlock(fe) like the success path does?  Given the use
of classes not doing that looks like it'll create lockdep issues.

I'd expect the unlock to match the lock.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: soc-compress: Fix deadlock in soc_compr_open_fe

[Carlos Llamas]

On Thu, Jun 15, 2023 at 01:56:35AM +0100, Mark Brown wrote:
> On Tue, Jun 13, 2023 at 02:23:50PM +0800, yixuanjiang wrote:
> > Modify the error handling flow by release lock.
> > The require pcm_mutex will keep holding if open fail.
> 
> > +++ b/sound/soc/soc-compress.c
> > @@ -166,6 +166,7 @@ static int soc_compr_open_fe(struct snd_compr_stream *cstream)
> >  	snd_soc_dai_compr_shutdown(cpu_dai, cstream, 1);
> >  out:
> >  	dpcm_path_put(&list);
> > +	mutex_unlock(&fe->card->pcm_mutex);
> >  be_err:
> 
> This is really hard to follow due to the lack of any mutex_lock()s in
> the function, I think because this is intended to undo
> snd_soc_dpcm_mutex_lock(fe) but if that's the case why is it not using
> snd_soc_dpcm_mutex_unlock(fe) like the success path does?  Given the use
> of classes not doing that looks like it'll create lockdep issues.
> 
> I'd expect the unlock to match the lock.

Yes, and judging from the context of the patch I believe this was based
off of stable 5.15.y tree. The locking has been refactored since. So
Yixuan, please rebase/adjust your patch on top of Linus's mainline tree
and resend. Thanks!

--
Carlos Llamas