[alsa-devel] [PATCH 0/2] ASoC: fixup topology dai_link remove issue

[alsa-devel] [PATCH 0/2] ASoC: fixup topology dai_link remove issue

[Kuninori Morimoto]

Hi Mark
Cc Pierre-Louis, Takashi-san

Currently, I'm focusing to ASoC cleanup / balance-up.
But, it found more unbalance issue, and Intel noticed about it.
These patches fix dai_link remove issue on topology.

I want to get Acked-by or Reviewed-by from Takashi-san
for 2) patch if possible.

These are already tested by Intel CI, and all issues were solved.
(https://github.com/thesofproject/linux/pull/1504)
Extra Tested-by / Reviewed-by are very welcome from Intel

Kuninori Morimoto (2):
  1) ASoC: soc-component: tidyup snd_soc_pcm_component_new/free() parameter
  2) ASoC: soc-pcm: remove soc_pcm_private_free()

 include/sound/soc-component.h |  4 ++--
 sound/soc/soc-component.c     |  8 +++-----
 sound/soc/soc-core.c          | 19 +++++++++++--------
 sound/soc/soc-pcm.c           | 12 +-----------
 4 files changed, 17 insertions(+), 26 deletions(-)

-- 
2.7.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 1/2] ASoC: soc-component: tidyup snd_soc_pcm_component_new/free() parameter

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

This patch uses rtd instead of pcm at snd_soc_pcm_component_new/free()
parameter.
This is prepare for dai_link remove bug fix on topology.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc-component.h | 4 ++--
 sound/soc/soc-component.c     | 8 +++-----
 sound/soc/soc-pcm.c           | 4 ++--
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 6aa3ecb..f8fadf0 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -412,7 +412,7 @@ struct page *snd_soc_pcm_component_page(struct snd_pcm_substream *substream,
 					unsigned long offset);
 int snd_soc_pcm_component_mmap(struct snd_pcm_substream *substream,
 			       struct vm_area_struct *vma);
-int snd_soc_pcm_component_new(struct snd_pcm *pcm);
-void snd_soc_pcm_component_free(struct snd_pcm *pcm);
+int snd_soc_pcm_component_new(struct snd_soc_pcm_runtime *rtd);
+void snd_soc_pcm_component_free(struct snd_soc_pcm_runtime *rtd);
 
 #endif /* __SOC_COMPONENT_H */
diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
index 98ef066..1590e80 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -498,9 +498,8 @@ int snd_soc_pcm_component_mmap(struct snd_pcm_substream *substream,
 	return -EINVAL;
 }
 
-int snd_soc_pcm_component_new(struct snd_pcm *pcm)
+int snd_soc_pcm_component_new(struct snd_soc_pcm_runtime *rtd)
 {
-	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_component *component;
 	int ret;
@@ -516,13 +515,12 @@ int snd_soc_pcm_component_new(struct snd_pcm *pcm)
 	return 0;
 }
 
-void snd_soc_pcm_component_free(struct snd_pcm *pcm)
+void snd_soc_pcm_component_free(struct snd_soc_pcm_runtime *rtd)
 {
-	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_component *component;
 
 	for_each_rtd_components(rtd, rtdcom, component)
 		if (component->driver->pcm_destruct)
-			component->driver->pcm_destruct(component, pcm);
+			component->driver->pcm_destruct(component, rtd->pcm);
 }
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 4bf71e3..c624d30 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2898,7 +2898,7 @@ static void soc_pcm_private_free(struct snd_pcm *pcm)
 
 	/* need to sync the delayed work before releasing resources */
 	flush_delayed_work(&rtd->delayed_work);
-	snd_soc_pcm_component_free(pcm);
+	snd_soc_pcm_component_free(rtd);
 }
 
 /* create a new pcm */
@@ -3036,7 +3036,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 	if (capture)
 		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops);
 
-	ret = snd_soc_pcm_component_new(pcm);
+	ret = snd_soc_pcm_component_new(rtd);
 	if (ret < 0) {
 		dev_err(rtd->dev, "ASoC: pcm constructor failed: %d\n", ret);
 		return ret;
-- 
2.7.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 2/2] ASoC: soc-pcm: remove soc_pcm_private_free()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

soc-topology adds extra dai_link by using snd_soc_add_dai_link(),
and removes it by snd_soc_romove_dai_link().

This snd_soc_add/remove_dai_link() and/or its related
functions are unbalanced before, and now, these are balance-uped.
But, it finds the random operation issue, and it is reported by
Pierre-Louis.

When card was released, topology will call snd_soc_remove_dai_link()
via (A).

	static void soc_cleanup_card_resources(struct snd_soc_card *card)
	{
		struct snd_soc_dai_link *link, *_link;

		/* This should be called before snd_card_free() */
	(A)	soc_remove_link_components(card);

		/* free the ALSA card at first; this syncs with pending operations */
		if (card->snd_card) {
	(B)		snd_card_free(card->snd_card);
			card->snd_card = NULL;
		}

		/* remove and free each DAI */
	(X)	soc_remove_link_dais(card);

		for_each_card_links_safe(card, link, _link)
	(C)		snd_soc_remove_dai_link(card, link);

		...
	}

At (A), topology calls snd_soc_remove_dai_link().
Then topology rtd, and its related all data are freed.

Next, (B) is called, and then, pcm->private_free = soc_pcm_private_free()
is called.

	static void soc_pcm_private_free(struct snd_pcm *pcm)
	{
		struct snd_soc_pcm_runtime *rtd = pcm->private_data;

		/* need to sync the delayed work before releasing resources */
		flush_delayed_work(&rtd->delayed_work);
		snd_soc_pcm_component_free(rtd);
	}

Here, it gets rtd via pcm->private_data.
But, topology related rtd are already freed at (A).
Normal sound card has no damage, becase it frees rtd at (C).

These are finalizing rtd related data.
Thus, these should be called when rtd was freed, not sound card
was freed. It is very natural and understandable.

In other words, pcm->private_free = soc_pcm_private_free()
is no longer needed.

Extra issue is that there is zero chance to call
soc_remove_dai() for topology related dai at (X).
Because (A) removes rtd connection from card too, and,
(X) is based on card connected rtd.

This means, (X) need to be called before (C) (= for normal sound)
and (A) (= for topology).

Now, I want to focus this patch which is the reason why
snd_card_free() = (B) is located there.

	commit 4efda5f2130da033aeedc5b3205569893b910de2
	("ASoC: Fix use-after-free at card unregistration")

Original snd_card_free() was called last of this function.
But moved to top to avoid use-after-free issue.
The issue was happen at soc_pcm_free() which was pcm->private_free,
today it is updated/renamed to soc_pcm_private_free().

In other words, (B) need to be called before (C) (= for normal sound)
and (A) (= for topology), because it needs (not yet freed) rtd.
But, (A) need to be called before (B),
because it needs card->snd_card pointer.

If we call flush_delayed_work() and snd_soc_pcm_component_free()
(= same as soc_pcm_private_free()) when rtd was freed (= (C), (A)),
there is no reason to call snd_card_free() at top of this function.
It can be called end of this function, again.

But, in such case, it will likely break unbind again, as Takashi-san
reported. When unbind is performed in a busy state, the code may
release still-in-use resources.
At least we need to call snd_card_disconnect_sync() at the first place.

The final code will be...

	static void soc_cleanup_card_resources(struct snd_soc_card *card)
	{
		struct snd_soc_dai_link *link, *_link;

		if (card->snd_card)
	(Z)		snd_card_disconnect_sync(card->snd_card);

	(X)	soc_remove_link_dais(card);
	(A)	soc_remove_link_components(card);

		for_each_card_links_safe(card, link, _link)
	(C)		snd_soc_remove_dai_link(card, link);

		...
		if (card->snd_card) {
	(B)		snd_card_free(card->snd_card);
			card->snd_card = NULL;
		}
	}

To avoid release still-in-use resources,
call snd_card_disconnect_sync() at (Z).

(X) is needed for both non-topology and topology.

    topology removes rtd via (A), and
non topology removes rtd via (C).

snd_card_free() is no longer related to use-after-free issue.
Thus, locating (B) is no problem.

Fixes: df95a16d2a9626 ("ASoC: soc-core: fix RIP warning on card removal")
Fixes: bc7a9091e5b927 ("ASoC: soc-core: add soc_unbind_dai_link()")
Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/soc-core.c | 19 +++++++++++--------
 sound/soc/soc-pcm.c  | 10 ----------
 2 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 977a7bf..e3a53ef 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -419,6 +419,9 @@ static void soc_free_pcm_runtime(struct snd_soc_pcm_runtime *rtd)
 
 	list_del(&rtd->list);
 
+	flush_delayed_work(&rtd->delayed_work);
+	snd_soc_pcm_component_free(rtd);
+
 	/*
 	 * we don't need to call kfree() for rtd->dev
 	 * see
@@ -1945,19 +1948,14 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card,
 {
 	struct snd_soc_dai_link *link, *_link;
 
-	/* This should be called before snd_card_free() */
-	soc_remove_link_components(card);
-
-	/* free the ALSA card at first; this syncs with pending operations */
-	if (card->snd_card) {
-		snd_card_free(card->snd_card);
-		card->snd_card = NULL;
-	}
+	if (card->snd_card)
+		snd_card_disconnect_sync(card->snd_card);
 
 	snd_soc_dapm_shutdown(card);
 
 	/* remove and free each DAI */
 	soc_remove_link_dais(card);
+	soc_remove_link_components(card);
 
 	for_each_card_links_safe(card, link, _link)
 		snd_soc_remove_dai_link(card, link);
@@ -1972,6 +1970,11 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card,
 	/* remove the card */
 	if (card_probed && card->remove)
 		card->remove(card);
+
+	if (card->snd_card) {
+		snd_card_free(card->snd_card);
+		card->snd_card = NULL;
+	}
 }
 
 static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index c624d30..2c4f50c 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2892,15 +2892,6 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
 	return ret;
 }
 
-static void soc_pcm_private_free(struct snd_pcm *pcm)
-{
-	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
-
-	/* need to sync the delayed work before releasing resources */
-	flush_delayed_work(&rtd->delayed_work);
-	snd_soc_pcm_component_free(rtd);
-}
-
 /* create a new pcm */
 int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 {
@@ -3042,7 +3033,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 		return ret;
 	}
 
-	pcm->private_free = soc_pcm_private_free;
 	pcm->no_device_suspend = true;
 out:
 	dev_info(rtd->card->dev, "%s <-> %s mapping ok\n",
-- 
2.7.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 0/2] ASoC: fixup topology dai_link remove issue

[Pierre-Louis Bossart]

On 11/17/19 7:49 PM, Kuninori Morimoto wrote:
> 
> Hi Mark
> Cc Pierre-Louis, Takashi-san
> 
> Currently, I'm focusing to ASoC cleanup / balance-up.
> But, it found more unbalance issue, and Intel noticed about it.
> These patches fix dai_link remove issue on topology.
> 
> I want to get Acked-by or Reviewed-by from Takashi-san
> for 2) patch if possible.
> 
> These are already tested by Intel CI, and all issues were solved.
> (https://github.com/thesofproject/linux/pull/1504)
> Extra Tested-by / Reviewed-by are very welcome from Intel

if you don't mind I'd like to retest this new series, it's based on a 
different tip and is not exactly the same as before.

e.g. we tested this:

-	/* free the ALSA card at first; this syncs with pending operations */
-	if (card->snd_card) {
-		snd_card_free(card->snd_card);
-		card->snd_card = NULL;
-	}
+	if (card->snd_card)
+		snd_card_disconnect_sync(card->snd_card);

  	/* remove and free each DAI */
  	soc_remove_link_dais(card);
+	soc_remove_link_components(card);


and the new code shows this

-	/* free the ALSA card at first; this syncs with pending operations */
-	if (card->snd_card) {
-		snd_card_free(card->snd_card);
-		card->snd_card = NULL;
-	}
+	if (card->snd_card)
+		snd_card_disconnect_sync(card->snd_card);

  	snd_soc_dapm_shutdown(card); <<< not tested yet.

  	/* remove and free each DAI */
  	soc_remove_link_dais(card);
+	soc_remove_link_components(card);



> 
> Kuninori Morimoto (2):
>    1) ASoC: soc-component: tidyup snd_soc_pcm_component_new/free() parameter
>    2) ASoC: soc-pcm: remove soc_pcm_private_free()
> 
>   include/sound/soc-component.h |  4 ++--
>   sound/soc/soc-component.c     |  8 +++-----
>   sound/soc/soc-core.c          | 19 +++++++++++--------
>   sound/soc/soc-pcm.c           | 12 +-----------
>   4 files changed, 17 insertions(+), 26 deletions(-)
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 0/2] ASoC: fixup topology dai_link remove issue

[Kuninori Morimoto]

Hi Pierre-Louis

> > These are already tested by Intel CI, and all issues were solved.
> > (https://github.com/thesofproject/linux/pull/1504)
> > Extra Tested-by / Reviewed-by are very welcome from Intel
> 
> if you don't mind I'd like to retest this new series, it's based on a
> different tip and is not exactly the same as before.

Yes, of course.
Sorry I didn't mention about it.

> -	/* free the ALSA card at first; this syncs with pending operations */
> -	if (card->snd_card) {
> -		snd_card_free(card->snd_card);
> -		card->snd_card = NULL;
> -	}
> +	if (card->snd_card)
> +		snd_card_disconnect_sync(card->snd_card);
> 
>  	snd_soc_dapm_shutdown(card); <<< not tested yet.
> 
>  	/* remove and free each DAI */
>  	soc_remove_link_dais(card);
> +	soc_remove_link_components(card);

Yes.
It is from

2a6f0892bda954dc2688b002060093ee0fe38528
("ASoC: soc-core: call snd_soc_dapm_shutdown() at soc_cleanup_card_resources()")


Thank you for your help !!
Best regards
---
Kuninori Morimoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 0/2] ASoC: fixup topology dai_link remove issue

[Pierre-Louis Bossart]

On 11/18/19 7:10 PM, Kuninori Morimoto wrote:
> 
> Hi Pierre-Louis
> 
>>> These are already tested by Intel CI, and all issues were solved.
>>> (https://github.com/thesofproject/linux/pull/1504)
>>> Extra Tested-by / Reviewed-by are very welcome from Intel
>>
>> if you don't mind I'd like to retest this new series, it's based on a
>> different tip and is not exactly the same as before.
> 
> Yes, of course.
> Sorry I didn't mention about it.
> 
>> -	/* free the ALSA card at first; this syncs with pending operations */
>> -	if (card->snd_card) {
>> -		snd_card_free(card->snd_card);
>> -		card->snd_card = NULL;
>> -	}
>> +	if (card->snd_card)
>> +		snd_card_disconnect_sync(card->snd_card);
>>
>>   	snd_soc_dapm_shutdown(card); <<< not tested yet.
>>
>>   	/* remove and free each DAI */
>>   	soc_remove_link_dais(card);
>> +	soc_remove_link_components(card);
> 
> Yes.
> It is from
> 
> 2a6f0892bda954dc2688b002060093ee0fe38528
> ("ASoC: soc-core: call snd_soc_dapm_shutdown() at soc_cleanup_card_resources()")

No regression detected so from the Intel side we're good with this patchset.
Thanks Morimoto-san for this comprehensive analysis, really nice work!

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 0/2] ASoC: fixup topology dai_link remove issue

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 278 bytes --]

On Tue, Nov 19, 2019 at 07:37:09AM -0600, Pierre-Louis Bossart wrote:

> No regression detected so from the Intel side we're good with this patchset.
> Thanks Morimoto-san for this comprehensive analysis, really nice work!

Yes, thanks indeed for this - it's really great work.

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: soc-pcm: remove soc_pcm_private_free()" to the asoc tree

[Mark Brown]

The patch

   ASoC: soc-pcm: remove soc_pcm_private_free()

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 0ced7b050224b18ca73e38e7068f36be8e708c06 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Date: Mon, 18 Nov 2019 10:51:11 +0900
Subject: [PATCH] ASoC: soc-pcm: remove soc_pcm_private_free()

soc-topology adds extra dai_link by using snd_soc_add_dai_link(),
and removes it by snd_soc_romove_dai_link().

This snd_soc_add/remove_dai_link() and/or its related
functions are unbalanced before, and now, these are balance-uped.
But, it finds the random operation issue, and it is reported by
Pierre-Louis.

When card was released, topology will call snd_soc_remove_dai_link()
via (A).

	static void soc_cleanup_card_resources(struct snd_soc_card *card)
	{
		struct snd_soc_dai_link *link, *_link;

		/* This should be called before snd_card_free() */
	(A)	soc_remove_link_components(card);

		/* free the ALSA card at first; this syncs with pending operations */
		if (card->snd_card) {
	(B)		snd_card_free(card->snd_card);
			card->snd_card = NULL;
		}

		/* remove and free each DAI */
	(X)	soc_remove_link_dais(card);

		for_each_card_links_safe(card, link, _link)
	(C)		snd_soc_remove_dai_link(card, link);

		...
	}

At (A), topology calls snd_soc_remove_dai_link().
Then topology rtd, and its related all data are freed.

Next, (B) is called, and then, pcm->private_free = soc_pcm_private_free()
is called.

	static void soc_pcm_private_free(struct snd_pcm *pcm)
	{
		struct snd_soc_pcm_runtime *rtd = pcm->private_data;

		/* need to sync the delayed work before releasing resources */
		flush_delayed_work(&rtd->delayed_work);
		snd_soc_pcm_component_free(rtd);
	}

Here, it gets rtd via pcm->private_data.
But, topology related rtd are already freed at (A).
Normal sound card has no damage, becase it frees rtd at (C).

These are finalizing rtd related data.
Thus, these should be called when rtd was freed, not sound card
was freed. It is very natural and understandable.

In other words, pcm->private_free = soc_pcm_private_free()
is no longer needed.

Extra issue is that there is zero chance to call
soc_remove_dai() for topology related dai at (X).
Because (A) removes rtd connection from card too, and,
(X) is based on card connected rtd.

This means, (X) need to be called before (C) (= for normal sound)
and (A) (= for topology).

Now, I want to focus this patch which is the reason why
snd_card_free() = (B) is located there.

	commit 4efda5f2130da033aeedc5b3205569893b910de2
	("ASoC: Fix use-after-free at card unregistration")

Original snd_card_free() was called last of this function.
But moved to top to avoid use-after-free issue.
The issue was happen at soc_pcm_free() which was pcm->private_free,
today it is updated/renamed to soc_pcm_private_free().

In other words, (B) need to be called before (C) (= for normal sound)
and (A) (= for topology), because it needs (not yet freed) rtd.
But, (A) need to be called before (B),
because it needs card->snd_card pointer.

If we call flush_delayed_work() and snd_soc_pcm_component_free()
(= same as soc_pcm_private_free()) when rtd was freed (= (C), (A)),
there is no reason to call snd_card_free() at top of this function.
It can be called end of this function, again.

But, in such case, it will likely break unbind again, as Takashi-san
reported. When unbind is performed in a busy state, the code may
release still-in-use resources.
At least we need to call snd_card_disconnect_sync() at the first place.

The final code will be...

	static void soc_cleanup_card_resources(struct snd_soc_card *card)
	{
		struct snd_soc_dai_link *link, *_link;

		if (card->snd_card)
	(Z)		snd_card_disconnect_sync(card->snd_card);

	(X)	soc_remove_link_dais(card);
	(A)	soc_remove_link_components(card);

		for_each_card_links_safe(card, link, _link)
	(C)		snd_soc_remove_dai_link(card, link);

		...
		if (card->snd_card) {
	(B)		snd_card_free(card->snd_card);
			card->snd_card = NULL;
		}
	}

To avoid release still-in-use resources,
call snd_card_disconnect_sync() at (Z).

(X) is needed for both non-topology and topology.

    topology removes rtd via (A), and
non topology removes rtd via (C).

snd_card_free() is no longer related to use-after-free issue.
Thus, locating (B) is no problem.

Fixes: df95a16d2a9626 ("ASoC: soc-core: fix RIP warning on card removal")
Fixes: bc7a9091e5b927 ("ASoC: soc-core: add soc_unbind_dai_link()")
Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Link: https://lore.kernel.org/r/87o8xax88g.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/soc-core.c | 19 +++++++++++--------
 sound/soc/soc-pcm.c  | 10 ----------
 2 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 977a7bfad519..e3a53ef1db04 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -419,6 +419,9 @@ static void soc_free_pcm_runtime(struct snd_soc_pcm_runtime *rtd)
 
 	list_del(&rtd->list);
 
+	flush_delayed_work(&rtd->delayed_work);
+	snd_soc_pcm_component_free(rtd);
+
 	/*
 	 * we don't need to call kfree() for rtd->dev
 	 * see
@@ -1945,19 +1948,14 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card,
 {
 	struct snd_soc_dai_link *link, *_link;
 
-	/* This should be called before snd_card_free() */
-	soc_remove_link_components(card);
-
-	/* free the ALSA card at first; this syncs with pending operations */
-	if (card->snd_card) {
-		snd_card_free(card->snd_card);
-		card->snd_card = NULL;
-	}
+	if (card->snd_card)
+		snd_card_disconnect_sync(card->snd_card);
 
 	snd_soc_dapm_shutdown(card);
 
 	/* remove and free each DAI */
 	soc_remove_link_dais(card);
+	soc_remove_link_components(card);
 
 	for_each_card_links_safe(card, link, _link)
 		snd_soc_remove_dai_link(card, link);
@@ -1972,6 +1970,11 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card,
 	/* remove the card */
 	if (card_probed && card->remove)
 		card->remove(card);
+
+	if (card->snd_card) {
+		snd_card_free(card->snd_card);
+		card->snd_card = NULL;
+	}
 }
 
 static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index c624d30bfa3c..2c4f50c44591 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2892,15 +2892,6 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
 	return ret;
 }
 
-static void soc_pcm_private_free(struct snd_pcm *pcm)
-{
-	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
-
-	/* need to sync the delayed work before releasing resources */
-	flush_delayed_work(&rtd->delayed_work);
-	snd_soc_pcm_component_free(rtd);
-}
-
 /* create a new pcm */
 int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 {
@@ -3042,7 +3033,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 		return ret;
 	}
 
-	pcm->private_free = soc_pcm_private_free;
 	pcm->no_device_suspend = true;
 out:
 	dev_info(rtd->card->dev, "%s <-> %s mapping ok\n",
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: soc-component: tidyup snd_soc_pcm_component_new/free() parameter" to the asoc tree

[Mark Brown]

The patch

   ASoC: soc-component: tidyup snd_soc_pcm_component_new/free() parameter

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From b2b2afbb48eac7215f951a8a462aa6837e0d495f Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Date: Mon, 18 Nov 2019 10:50:32 +0900
Subject: [PATCH] ASoC: soc-component: tidyup snd_soc_pcm_component_new/free()
 parameter

This patch uses rtd instead of pcm at snd_soc_pcm_component_new/free()
parameter.
This is prepare for dai_link remove bug fix on topology.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Link: https://lore.kernel.org/r/87pnhqx89j.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/sound/soc-component.h | 4 ++--
 sound/soc/soc-component.c     | 8 +++-----
 sound/soc/soc-pcm.c           | 4 ++--
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 6aa3ecb7b6d3..f8fadf0e696a 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -412,7 +412,7 @@ struct page *snd_soc_pcm_component_page(struct snd_pcm_substream *substream,
 					unsigned long offset);
 int snd_soc_pcm_component_mmap(struct snd_pcm_substream *substream,
 			       struct vm_area_struct *vma);
-int snd_soc_pcm_component_new(struct snd_pcm *pcm);
-void snd_soc_pcm_component_free(struct snd_pcm *pcm);
+int snd_soc_pcm_component_new(struct snd_soc_pcm_runtime *rtd);
+void snd_soc_pcm_component_free(struct snd_soc_pcm_runtime *rtd);
 
 #endif /* __SOC_COMPONENT_H */
diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
index 98ef0666add2..1590e805d016 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -498,9 +498,8 @@ int snd_soc_pcm_component_mmap(struct snd_pcm_substream *substream,
 	return -EINVAL;
 }
 
-int snd_soc_pcm_component_new(struct snd_pcm *pcm)
+int snd_soc_pcm_component_new(struct snd_soc_pcm_runtime *rtd)
 {
-	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_component *component;
 	int ret;
@@ -516,13 +515,12 @@ int snd_soc_pcm_component_new(struct snd_pcm *pcm)
 	return 0;
 }
 
-void snd_soc_pcm_component_free(struct snd_pcm *pcm)
+void snd_soc_pcm_component_free(struct snd_soc_pcm_runtime *rtd)
 {
-	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
 	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_component *component;
 
 	for_each_rtd_components(rtd, rtdcom, component)
 		if (component->driver->pcm_destruct)
-			component->driver->pcm_destruct(component, pcm);
+			component->driver->pcm_destruct(component, rtd->pcm);
 }
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 4bf71e3211d8..c624d30bfa3c 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2898,7 +2898,7 @@ static void soc_pcm_private_free(struct snd_pcm *pcm)
 
 	/* need to sync the delayed work before releasing resources */
 	flush_delayed_work(&rtd->delayed_work);
-	snd_soc_pcm_component_free(pcm);
+	snd_soc_pcm_component_free(rtd);
 }
 
 /* create a new pcm */
@@ -3036,7 +3036,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 	if (capture)
 		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops);
 
-	ret = snd_soc_pcm_component_new(pcm);
+	ret = snd_soc_pcm_component_new(rtd);
 	if (ret < 0) {
 		dev_err(rtd->dev, "ASoC: pcm constructor failed: %d\n", ret);
 		return ret;
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 2/2] ASoC: soc-pcm: remove soc_pcm_private_free()

[Enric Balletbo Serra]

Dear all,

Missatge de Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> del
dia dl., 18 de nov. 2019 a les 2:52:
>
>
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> soc-topology adds extra dai_link by using snd_soc_add_dai_link(),
> and removes it by snd_soc_romove_dai_link().
>
> This snd_soc_add/remove_dai_link() and/or its related
> functions are unbalanced before, and now, these are balance-uped.
> But, it finds the random operation issue, and it is reported by
> Pierre-Louis.
>
> When card was released, topology will call snd_soc_remove_dai_link()
> via (A).
>
>         static void soc_cleanup_card_resources(struct snd_soc_card *card)
>         {
>                 struct snd_soc_dai_link *link, *_link;
>
>                 /* This should be called before snd_card_free() */
>         (A)     soc_remove_link_components(card);
>
>                 /* free the ALSA card at first; this syncs with pending operations */
>                 if (card->snd_card) {
>         (B)             snd_card_free(card->snd_card);
>                         card->snd_card = NULL;
>                 }
>
>                 /* remove and free each DAI */
>         (X)     soc_remove_link_dais(card);
>
>                 for_each_card_links_safe(card, link, _link)
>         (C)             snd_soc_remove_dai_link(card, link);
>
>                 ...
>         }
>
> At (A), topology calls snd_soc_remove_dai_link().
> Then topology rtd, and its related all data are freed.
>
> Next, (B) is called, and then, pcm->private_free = soc_pcm_private_free()
> is called.
>
>         static void soc_pcm_private_free(struct snd_pcm *pcm)
>         {
>                 struct snd_soc_pcm_runtime *rtd = pcm->private_data;
>
>                 /* need to sync the delayed work before releasing resources */
>                 flush_delayed_work(&rtd->delayed_work);
>                 snd_soc_pcm_component_free(rtd);
>         }
>
> Here, it gets rtd via pcm->private_data.
> But, topology related rtd are already freed at (A).
> Normal sound card has no damage, becase it frees rtd at (C).
>
> These are finalizing rtd related data.
> Thus, these should be called when rtd was freed, not sound card
> was freed. It is very natural and understandable.
>
> In other words, pcm->private_free = soc_pcm_private_free()
> is no longer needed.
>
> Extra issue is that there is zero chance to call
> soc_remove_dai() for topology related dai at (X).
> Because (A) removes rtd connection from card too, and,
> (X) is based on card connected rtd.
>
> This means, (X) need to be called before (C) (= for normal sound)
> and (A) (= for topology).
>
> Now, I want to focus this patch which is the reason why
> snd_card_free() = (B) is located there.
>
>         commit 4efda5f2130da033aeedc5b3205569893b910de2
>         ("ASoC: Fix use-after-free at card unregistration")
>
> Original snd_card_free() was called last of this function.
> But moved to top to avoid use-after-free issue.
> The issue was happen at soc_pcm_free() which was pcm->private_free,
> today it is updated/renamed to soc_pcm_private_free().
>
> In other words, (B) need to be called before (C) (= for normal sound)
> and (A) (= for topology), because it needs (not yet freed) rtd.
> But, (A) need to be called before (B),
> because it needs card->snd_card pointer.
>
> If we call flush_delayed_work() and snd_soc_pcm_component_free()
> (= same as soc_pcm_private_free()) when rtd was freed (= (C), (A)),
> there is no reason to call snd_card_free() at top of this function.
> It can be called end of this function, again.
>
> But, in such case, it will likely break unbind again, as Takashi-san
> reported. When unbind is performed in a busy state, the code may
> release still-in-use resources.
> At least we need to call snd_card_disconnect_sync() at the first place.
>
> The final code will be...
>
>         static void soc_cleanup_card_resources(struct snd_soc_card *card)
>         {
>                 struct snd_soc_dai_link *link, *_link;
>
>                 if (card->snd_card)
>         (Z)             snd_card_disconnect_sync(card->snd_card);
>
>         (X)     soc_remove_link_dais(card);
>         (A)     soc_remove_link_components(card);
>
>                 for_each_card_links_safe(card, link, _link)
>         (C)             snd_soc_remove_dai_link(card, link);
>
>                 ...
>                 if (card->snd_card) {
>         (B)             snd_card_free(card->snd_card);
>                         card->snd_card = NULL;
>                 }
>         }
>
> To avoid release still-in-use resources,
> call snd_card_disconnect_sync() at (Z).
>
> (X) is needed for both non-topology and topology.
>
>     topology removes rtd via (A), and
> non topology removes rtd via (C).
>
> snd_card_free() is no longer related to use-after-free issue.
> Thus, locating (B) is no problem.
>
> Fixes: df95a16d2a9626 ("ASoC: soc-core: fix RIP warning on card removal")
> Fixes: bc7a9091e5b927 ("ASoC: soc-core: add soc_unbind_dai_link()")
> Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---

I didn't look into detail yet, but after applying this patch my
Samsung Chromebook Plus started to show different warnings like this,
probably caused because in my case the driver is deferring?

I'll try to take a look, but if anyone already knows the cause, please
let me know.

Thanks,
 Enric

[   10.873336] ------------[ cut here ]------------
[   10.878507] WARNING: CPU: 4 PID: 45 at kernel/workqueue.c:3032
__flush_work.isra.45+0x210/0x230
[   10.888226] Modules linked in: snd_soc_rockchip_i2s(+)
vctrl_regulator snd_soc_rk3399_gru_sound snd_soc_rt5514 cec gpu_sched
snd_soc_max98357a kfifo_buf i2c_hid cros_e
c_sensors_core snd_soc_da7219 snd_soc_rockchip_pcm phy_rockchip_pcie
snd_soc_rt5514_spi rockchip_saradc sbs_battery pwm_bl
cros_usbpd_charger cros_ec_chardev cros_usbpd_l
ogger pcie_rockchip_host pwm_cros_ec rockchip_thermal snd_soc_rl6231
ip_tables x_tables ipv6 nf_defrag_ipv6
[   10.931870] CPU: 4 PID: 45 Comm: kworker/4:1 Not tainted 5.4.0+
#322
[   10.938967] Hardware name: Google Kevin (DT)
[   10.943738] Workqueue: events deferred_probe_work_func
[   10.949476] pstate: 00000005 (nzcv daif -PAN -UAO)
[   10.954825] pc : __flush_work.isra.45+0x210/0x230
[   10.960068] lr : flush_delayed_work+0x34/0x58
[   10.964929] sp : ffff800011d4b930
[   10.968625] x29: ffff800011d4b930 x28: 00000000fffffdfb
[   10.974547] x27: ffff80001129b530 x26: ffff8000111a6a08
[   10.980469] x25: 0000000000000003 x24: 0000000000000001
[   10.986399] x23: ffff800011a7a980 x22: ffff8000118998c8
[   10.992329] x21: 0000000000000000 x20: ffff800008df1000
[   10.998260] x19: ffff0000ed60f698 x18: 0000000000000001
[   11.004190] x17: 0000000000000001 x16: ffff800011e7f000
[   11.010120] x15: ffffffffffffffff x14: 0000000000000000
[   11.016051] x13: 0000000000000000 x12: 0000000000000020
[   11.021981] x11: 0000000000000008 x10: 0101010101010101
[   11.027912] x9 : 0000000000000000 x8 : 7f7f7f7f7f7f7f7f
[   11.033842] x7 : ffff0000f5557340 x6 : 0080808080808080
[   11.039778] x5 : dead000000000100 x4 : 0000000000000000
[   11.045711] x3 : 0000000000000000 x2 : 9def22a3228fd300
[   11.051642] x1 : 0000000000000000 x0 : 0000000000000000
[   11.057565] Call trace:
[   11.060294]  __flush_work.isra.45+0x210/0x230
[   11.065157]  flush_delayed_work+0x34/0x58
[   11.069623]  soc_free_pcm_runtime.part.13+0x40/0x60
[   11.075074]  snd_soc_remove_dai_link+0x54/0x60
[   11.075076]  soc_cleanup_card_resources+0x160/0x2a8
[   11.075078]  snd_soc_bind_card+0x264/0xa10
[   11.075080]  snd_soc_register_card+0xf4/0x108
[   11.075088]  devm_snd_soc_register_card+0x40/0x90
[   11.100189]  rockchip_sound_probe+0x204/0x2e4 [snd_soc_rk3399_gru_sound]
[   11.107770]  platform_drv_probe+0x50/0xa0
[   11.112244]  really_probe+0xd4/0x308
[   11.116234]  driver_probe_device+0x54/0xe8
[   11.122457]  __device_attach_driver+0x80/0xb8
[   11.127312]  bus_for_each_drv+0x78/0xc8
[   11.131788]  __device_attach+0xd4/0x130
[   11.136070]  device_initial_probe+0x10/0x18
[   11.140738]  bus_probe_device+0x90/0x98
[   11.145020]  deferred_probe_work_func+0x6c/0xa0
[   11.155998]  process_one_work+0x1e0/0x358
[   11.160474]  worker_thread+0x40/0x488
[   11.164562]  kthread+0x118/0x120
[   11.168163]  ret_from_fork+0x10/0x18
[   11.172347] ---[ end trace dc22ca199fcfcf7c ]---



>  sound/soc/soc-core.c | 19 +++++++++++--------
>  sound/soc/soc-pcm.c  | 10 ----------
>  2 files changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 977a7bf..e3a53ef 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -419,6 +419,9 @@ static void soc_free_pcm_runtime(struct snd_soc_pcm_runtime *rtd)
>
>         list_del(&rtd->list);
>
> +       flush_delayed_work(&rtd->delayed_work);
> +       snd_soc_pcm_component_free(rtd);
> +
>         /*
>          * we don't need to call kfree() for rtd->dev
>          * see
> @@ -1945,19 +1948,14 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card,
>  {
>         struct snd_soc_dai_link *link, *_link;
>
> -       /* This should be called before snd_card_free() */
> -       soc_remove_link_components(card);
> -
> -       /* free the ALSA card at first; this syncs with pending operations */
> -       if (card->snd_card) {
> -               snd_card_free(card->snd_card);
> -               card->snd_card = NULL;
> -       }
> +       if (card->snd_card)
> +               snd_card_disconnect_sync(card->snd_card);
>
>         snd_soc_dapm_shutdown(card);
>
>         /* remove and free each DAI */
>         soc_remove_link_dais(card);
> +       soc_remove_link_components(card);
>
>         for_each_card_links_safe(card, link, _link)
>                 snd_soc_remove_dai_link(card, link);
> @@ -1972,6 +1970,11 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card,
>         /* remove the card */
>         if (card_probed && card->remove)
>                 card->remove(card);
> +
> +       if (card->snd_card) {
> +               snd_card_free(card->snd_card);
> +               card->snd_card = NULL;
> +       }
>  }
>
>  static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister)
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index c624d30..2c4f50c 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2892,15 +2892,6 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
>         return ret;
>  }
>
> -static void soc_pcm_private_free(struct snd_pcm *pcm)
> -{
> -       struct snd_soc_pcm_runtime *rtd = pcm->private_data;
> -
> -       /* need to sync the delayed work before releasing resources */
> -       flush_delayed_work(&rtd->delayed_work);
> -       snd_soc_pcm_component_free(rtd);
> -}
> -
>  /* create a new pcm */
>  int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>  {
> @@ -3042,7 +3033,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>                 return ret;
>         }
>
> -       pcm->private_free = soc_pcm_private_free;
>         pcm->no_device_suspend = true;
>  out:
>         dev_info(rtd->card->dev, "%s <-> %s mapping ok\n",
> --
> 2.7.4
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 2/2] ASoC: soc-pcm: remove soc_pcm_private_free()

[Daniel Baluta]

> I didn't look into detail yet, but after applying this patch my
> Samsung Chromebook Plus started to show different warnings like this,
> probably caused because in my case the driver is deferring?
> 
> I'll try to take a look, but if anyone already knows the cause,
> please
> let me know.
> 

Hi Enric,

Can you try:

https://patchwork.kernel.org/patch/11265061/

It should be already in Mark's tree.


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 2/2] ASoC: soc-pcm: remove soc_pcm_private_free()

[Enric Balletbo Serra]

Hi Daniel,

Missatge de Daniel Baluta <daniel.baluta@nxp.com> del dia dj., 5 de
des. 2019 a les 13:54:
>
>
> > I didn't look into detail yet, but after applying this patch my
> > Samsung Chromebook Plus started to show different warnings like this,
> > probably caused because in my case the driver is deferring?
> >
> > I'll try to take a look, but if anyone already knows the cause,
> > please
> > let me know.
> >
>
> Hi Enric,
>
> Can you try:
>
> https://patchwork.kernel.org/patch/11265061/
>

Actually I picked

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=for-5.5&id=4bf2e385aa59c2fae5f880aa25cfd2b470109093

which is supposed to land in this release cycle, so all fine.

Thanks,
  Enric

> It should be already in Mark's tree.
>
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel