[RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Kuninori Morimoto]

Current ALSA SoC Sound Card basically consists of CPU/Codec/Platform
drivers. If system uses Kernel modules, we can disable these drivers
by using rmmod command. In such case, we can't disable
CPU/Codec/Platform driver without disabling Sound Card driver.

But on the other hand, we can disable these drivers by using unbind
command. In such case, we can disable these drivers randomly.
In this case, we can create dirty Sound Card which is missing necessary
components.

(1) If user disabled Sound Card first, but did nothing to other drivers,
user can't use Sound because Sound Card is no longer exists.
(2) If user disabled CPU/Codec/Platform driver randomly, but did nothing
to Sound Card, user still be able to use Sound Card, because dirty Sound
Card which is missing necessary components still exists.
In this case, Sound system will be crashed if user started sound
playback/capture. But we can't block such random unbind now.

To avoid Sound Card crash in (2) case, what we can do now is, add dirty
flag on Sound Card, and avoid to open Sound Card.
This patch solved this issue.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
Mark, Lars-Peter

This is still RFC, but I think we need to consider about this issue.
I tested this patch on my system, and it can block to sound card crash
for me

 include/sound/soc.h  |  6 ++++--
 sound/soc/soc-core.c | 12 +++++++++++-
 sound/soc/soc-pcm.c  | 20 ++++++++++++++++++++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 316fdce..4813fc5 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1106,8 +1106,6 @@ struct snd_soc_card {
 	struct mutex mutex;
 	struct mutex dapm_mutex;
 
-	bool instantiated;
-
 	int (*probe)(struct snd_soc_card *card);
 	int (*late_probe)(struct snd_soc_card *card);
 	int (*remove)(struct snd_soc_card *card);
@@ -1197,6 +1195,10 @@ struct snd_soc_card {
 	u32 pop_time;
 
 	void *drvdata;
+
+	/* bit field */
+	u32 instantiated:1;
+	u32 dirty:1;
 };
 
 /* SoC machine DAI configuration, glues a codec and cpu DAI together */
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 07e4eec..a18e106 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2882,7 +2882,15 @@ int snd_soc_unregister_card(struct snd_soc_card *card)
 {
 	if (card->instantiated) {
 		card->instantiated = false;
-		snd_soc_dapm_shutdown(card);
+		if (!card->dirty) {
+			/*
+			 * In case of unbind, CPU/Codec/Platform component can
+			 * be unbinded *before* Card unbind. In such case
+			 * (= card->dirty) Card connected DAPMs are already
+			 * doesn't exist.
+			 */
+			snd_soc_dapm_shutdown(card);
+		}
 		soc_cleanup_card_resources(card);
 		dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
 	}
@@ -3236,6 +3244,8 @@ static void snd_soc_component_cleanup(struct snd_soc_component *component)
 
 static void snd_soc_component_del_unlocked(struct snd_soc_component *component)
 {
+	if (component->card)
+		component->card->dirty = 1;
 	list_del(&component->list);
 }
 
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index efc5831..c861aba 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -455,6 +455,16 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	const char *codec_dai_name = "multicodec";
 	int i, ret = 0;
 
+	if (rtd->card->dirty) {
+		/*
+		 * In case of unbind, each CPU/Codec/Platform component can
+		 * be unbinded randomly. In such case (= card->dirty)
+		 * Sound Card is no longer safety. Don't open it.
+		 */
+		dev_warn(cpu_dai->dev, "Sound SoC Card is dirty, Recheck your system\n");
+		return -ENXIO;
+	}
+
 	pinctrl_pm_select_default_state(cpu_dai->dev);
 	for (i = 0; i < rtd->num_codecs; i++)
 		pinctrl_pm_select_default_state(rtd->codec_dais[i]->dev);
@@ -2577,6 +2587,16 @@ static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream)
 	int ret;
 	int stream = fe_substream->stream;
 
+	if (fe->card->dirty) {
+		/*
+		 * In case of unbind, each CPU/Codec/Platform component can
+		 * be unbinded randomly. In such case (= card->dirty)
+		 * Sound Card is no longer safety. Don't open it.
+		 */
+		dev_warn(fe->dev, "Sound SoC Card is dirty, Recheck your system\n");
+		return -ENXIO;
+	}
+
 	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
 	fe->dpcm[stream].runtime = fe_substream->runtime;
 
-- 
1.9.1

Re: [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Geert Uytterhoeven]

Hi Morimoto-san,

On Wed, Mar 29, 2017 at 4:45 AM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> Current ALSA SoC Sound Card basically consists of CPU/Codec/Platform
> drivers. If system uses Kernel modules, we can disable these drivers
> by using rmmod command. In such case, we can't disable
> CPU/Codec/Platform driver without disabling Sound Card driver.
>
> But on the other hand, we can disable these drivers by using unbind
> command. In such case, we can disable these drivers randomly.
> In this case, we can create dirty Sound Card which is missing necessary
> components.
>
> (1) If user disabled Sound Card first, but did nothing to other drivers,
> user can't use Sound because Sound Card is no longer exists.
> (2) If user disabled CPU/Codec/Platform driver randomly, but did nothing
> to Sound Card, user still be able to use Sound Card, because dirty Sound
> Card which is missing necessary components still exists.
> In this case, Sound system will be crashed if user started sound
> playback/capture. But we can't block such random unbind now.
>
> To avoid Sound Card crash in (2) case, what we can do now is, add dirty
> flag on Sound Card, and avoid to open Sound Card.
> This patch solved this issue.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c

> @@ -3236,6 +3244,8 @@ static void snd_soc_component_cleanup(struct snd_soc_component *component)
>
>  static void snd_soc_component_del_unlocked(struct snd_soc_component *component)
>  {
> +       if (component->card)
> +               component->card->dirty = 1;

Currently this is the only assignment to .dirty.
Can this be undone later, when the driver is bound again?

>         list_del(&component->list);
>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Geert Uytterhoeven]

Hi Morimoto-san,

On Wed, Mar 29, 2017 at 4:45 AM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> Current ALSA SoC Sound Card basically consists of CPU/Codec/Platform
> drivers. If system uses Kernel modules, we can disable these drivers
> by using rmmod command. In such case, we can't disable
> CPU/Codec/Platform driver without disabling Sound Card driver.
>
> But on the other hand, we can disable these drivers by using unbind
> command. In such case, we can disable these drivers randomly.
> In this case, we can create dirty Sound Card which is missing necessary
> components.
>
> (1) If user disabled Sound Card first, but did nothing to other drivers,
> user can't use Sound because Sound Card is no longer exists.
> (2) If user disabled CPU/Codec/Platform driver randomly, but did nothing
> to Sound Card, user still be able to use Sound Card, because dirty Sound
> Card which is missing necessary components still exists.
> In this case, Sound system will be crashed if user started sound
> playback/capture. But we can't block such random unbind now.
>
> To avoid Sound Card crash in (2) case, what we can do now is, add dirty
> flag on Sound Card, and avoid to open Sound Card.
> This patch solved this issue.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c

> @@ -3236,6 +3244,8 @@ static void snd_soc_component_cleanup(struct snd_soc_component *component)
>
>  static void snd_soc_component_del_unlocked(struct snd_soc_component *component)
>  {
> +       if (component->card)
> +               component->card->dirty = 1;

Currently this is the only assignment to .dirty.
Can this be undone later, when the driver is bound again?

>         list_del(&component->list);
>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Kuninori Morimoto]

Hi Geert

> > Current ALSA SoC Sound Card basically consists of CPU/Codec/Platform
> > drivers. If system uses Kernel modules, we can disable these drivers
> > by using rmmod command. In such case, we can't disable
> > CPU/Codec/Platform driver without disabling Sound Card driver.
> >
> > But on the other hand, we can disable these drivers by using unbind
> > command. In such case, we can disable these drivers randomly.
> > In this case, we can create dirty Sound Card which is missing necessary
> > components.
> >
> > (1) If user disabled Sound Card first, but did nothing to other drivers,
> > user can't use Sound because Sound Card is no longer exists.
> > (2) If user disabled CPU/Codec/Platform driver randomly, but did nothing
> > to Sound Card, user still be able to use Sound Card, because dirty Sound
> > Card which is missing necessary components still exists.
> > In this case, Sound system will be crashed if user started sound
> > playback/capture. But we can't block such random unbind now.
> >
> > To avoid Sound Card crash in (2) case, what we can do now is, add dirty
> > flag on Sound Card, and avoid to open Sound Card.
> > This patch solved this issue.
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> > --- a/sound/soc/soc-core.c
> > +++ b/sound/soc/soc-core.c
> 
> > @@ -3236,6 +3244,8 @@ static void snd_soc_component_cleanup(struct snd_soc_component *component)
> >
> >  static void snd_soc_component_del_unlocked(struct snd_soc_component *component)
> >  {
> > +       if (component->card)
> > +               component->card->dirty = 1;
> 
> Currently this is the only assignment to .dirty.
> Can this be undone later, when the driver is bound again?

Does this "undone" mean "card->dirty = 0" ?

Card itself will be freed if Card was unbind.
Thus, card will be re-created if it was bind again.
.dirty will be automatically 0 cleared.

Best regards
---
Kuninori Morimoto

Re: [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Mark Brown]

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

On Wed, Mar 29, 2017 at 02:45:37AM +0000, Kuninori Morimoto wrote:

> To avoid Sound Card crash in (2) case, what we can do now is, add dirty
> flag on Sound Card, and avoid to open Sound Card.
> This patch solved this issue.

I think this is a good direction to at least start to mitigate these
problems (which we really should be doing) and hopefully make it easier
to do further improvements in future.  There's obviously more places
where we should be checking the flag (controls for example) but they can
be added later.  One thing I would like to see is instead of setting the
flag directly when we see a problem call a function to do it.  That way
if we want to improve things in the future we can do that without having
to update the callers again.

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

Re: [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Kuninori Morimoto]

Hi Mark

Thank you for your feedback

> > To avoid Sound Card crash in (2) case, what we can do now is, add dirty
> > flag on Sound Card, and avoid to open Sound Card.
> > This patch solved this issue.
> 
> I think this is a good direction to at least start to mitigate these
> problems (which we really should be doing) and hopefully make it easier
> to do further improvements in future.  There's obviously more places
> where we should be checking the flag (controls for example) but they can
> be added later.  One thing I would like to see is instead of setting the
> flag directly when we see a problem call a function to do it.  That way
> if we want to improve things in the future we can do that without having
> to update the callers again.

I guess you are pointing about snd_soc_dapm_shutdown() in snd_soc_unregister_card() ?
If so, I agree. Actually, I don't like this kind of adhoc handling.
I want to explain about it.

Order
[1] Disable Sound Card         -> Disable CPU/Codec/Platform or do nothing
[2] Disable CPU/Codec/Platform -> Disable Card or do nothing

Operation
1) shutdown all Card connected DAPM
2) cleanup Card resource.

In case of order [1], operation 1) -> 2) is no problem, because all card connected DAPM
exists on Card.
But, in case of order [2], operation 1) will try to call disconnected DAPM.
The DAPM disconnection from Card is done by snd_soc_dapm_free() which is called from
soc_remove_component().
This soc_remove_component() will be called with "remove_order" from operation 2).
One note is that reordering operation to 2) -> 1) will avoid this crash issue,
but it is no meaning, because 1) will do nothing ;)

Here, other solution is calling soc_remove_component() without "remove_order"
or calling snd_soc_dapm_free() when CPU/Codec/Platform are disabled.
But, 1 bad-point is that disabled CPU/Codec/Platform have no chance to
be called about snd_soc_dapm_set_bias_level() (= 1) operation).
Of course, adhoc function can solve this issue too.

If you are OK, I can work for this idea and remove adhoc operation
from snd_soc_unregister_card()

Best regards
---
Kuninori Morimoto

Re: [alsa-devel] [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Takashi Iwai]

On Thu, 30 Mar 2017 23:53:34 +0200,
Mark Brown wrote:
> 
> On Wed, Mar 29, 2017 at 02:45:37AM +0000, Kuninori Morimoto wrote:
> 
> > To avoid Sound Card crash in (2) case, what we can do now is, add dirty
> > flag on Sound Card, and avoid to open Sound Card.
> > This patch solved this issue.
> 
> I think this is a good direction to at least start to mitigate these
> problems (which we really should be doing) and hopefully make it easier
> to do further improvements in future.  There's obviously more places
> where we should be checking the flag (controls for example) but they can
> be added later.  One thing I would like to see is instead of setting the
> flag directly when we see a problem call a function to do it.  That way
> if we want to improve things in the future we can do that without having
> to update the callers again.

BTW, ALSA core has snd_card_disconnect() that does this kind of
shut-up from user-space.  It was introduced for hot-unplug, but
basically unbinding is the software hot-unplug.  So, if ASoC won't
rebind a once-unbound component, you can simply call
snd_card_disconnect() at the component unbinding time to assure that
no further user actions can be done.


Takashi

Re: [alsa-devel] [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Kuninori Morimoto]

Hi Takashi-san

> > I think this is a good direction to at least start to mitigate these
> > problems (which we really should be doing) and hopefully make it easier
> > to do further improvements in future.  There's obviously more places
> > where we should be checking the flag (controls for example) but they can
> > be added later.  One thing I would like to see is instead of setting the
> > flag directly when we see a problem call a function to do it.  That way
> > if we want to improve things in the future we can do that without having
> > to update the callers again.
> 
> BTW, ALSA core has snd_card_disconnect() that does this kind of
> shut-up from user-space.  It was introduced for hot-unplug, but
> basically unbinding is the software hot-unplug.  So, if ASoC won't
> rebind a once-unbound component, you can simply call
> snd_card_disconnect() at the component unbinding time to assure that
> no further user actions can be done.

Thanks. I checked about snd_card_disconnect(), and it will be called
from snd_card_free(). And it will be called from snd_soc_unregister_card()
So, we can call snd_soc_unregister_card() whenever CPU/Codec/Platform
were unregsiterd.

This method also solve random unbind/bind Oops too.
Here, random unbind/bind example is that
expected correct operation is unbind all CPU/Codec/Platfrom/Card,
and then, bind all CPU/Codec/Platfrom/Card again.
(here unbind order can be random)
But this case, we will get Oops if unbind Codec -> bind Codec -> unbind Card.
Using snd_soc_unregister_card() can solve this issue too.

Best regards
---
Kuninori Morimoto

Re: [alsa-devel] [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Takashi Iwai]

On Mon, 03 Apr 2017 08:29:34 +0200,
Kuninori Morimoto wrote:
> 
> 
> Hi Takashi-san
> 
> > > I think this is a good direction to at least start to mitigate these
> > > problems (which we really should be doing) and hopefully make it easier
> > > to do further improvements in future.  There's obviously more places
> > > where we should be checking the flag (controls for example) but they can
> > > be added later.  One thing I would like to see is instead of setting the
> > > flag directly when we see a problem call a function to do it.  That way
> > > if we want to improve things in the future we can do that without having
> > > to update the callers again.
> > 
> > BTW, ALSA core has snd_card_disconnect() that does this kind of
> > shut-up from user-space.  It was introduced for hot-unplug, but
> > basically unbinding is the software hot-unplug.  So, if ASoC won't
> > rebind a once-unbound component, you can simply call
> > snd_card_disconnect() at the component unbinding time to assure that
> > no further user actions can be done.
> 
> Thanks. I checked about snd_card_disconnect(), and it will be called
> from snd_card_free(). And it will be called from snd_soc_unregister_card()

Yes, snd_card_free() assures the disconnection at first, syncs the all
settled down, then releases the resources.

> So, we can call snd_soc_unregister_card() whenever CPU/Codec/Platform
> were unregsiterd.

In theory yes, but you should be careful to do so, e.g. make sure that
it won't be called again by the removal/unbind of other components /
drivers.

I suggested snd_card_disconnect() because it doesn't release resources
by itself, but it just disconnects from the further accesses.  So,
double-free won't happen in this case.  It makes the hotunplug safer
as long as the drivers manage the resource releases properly.


Takashi

> This method also solve random unbind/bind Oops too.
> Here, random unbind/bind example is that
> expected correct operation is unbind all CPU/Codec/Platfrom/Card,
> and then, bind all CPU/Codec/Platfrom/Card again.
> (here unbind order can be random)
> But this case, we will get Oops if unbind Codec -> bind Codec -> unbind Card.
> Using snd_soc_unregister_card() can solve this issue too.
> 
> Best regards
> ---
> Kuninori Morimoto
> 

Re: [alsa-devel] [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Kuninori Morimoto]

Hi Takashi-san

> > So, we can call snd_soc_unregister_card() whenever CPU/Codec/Platform
> > were unregsiterd.
> 
> In theory yes, but you should be careful to do so, e.g. make sure that
> it won't be called again by the removal/unbind of other components /
> drivers.
> 
> I suggested snd_card_disconnect() because it doesn't release resources
> by itself, but it just disconnects from the further accesses.  So,
> double-free won't happen in this case.  It makes the hotunplug safer
> as long as the drivers manage the resource releases properly.

I had checked many unbind/bind pattern/order on 2nd [RFC] patch which I posted.
At first, I believe Oops on unbind/bind issue was solved on it.
2nd, if my understanding was correct, it doesn't have double-free issue,
or something like that.
But, I'm not 100% sure about 2nd, thus it has [RFC] on patch.

Best regards
---
Kuninori Morimoto

Re: [alsa-devel] [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Takashi Iwai]

On Mon, 03 Apr 2017 10:26:05 +0200,
Kuninori Morimoto wrote:
> 
> 
> Hi Takashi-san
> 
> > > So, we can call snd_soc_unregister_card() whenever CPU/Codec/Platform
> > > were unregsiterd.
> > 
> > In theory yes, but you should be careful to do so, e.g. make sure that
> > it won't be called again by the removal/unbind of other components /
> > drivers.
> > 
> > I suggested snd_card_disconnect() because it doesn't release resources
> > by itself, but it just disconnects from the further accesses.  So,
> > double-free won't happen in this case.  It makes the hotunplug safer
> > as long as the drivers manage the resource releases properly.
> 
> I had checked many unbind/bind pattern/order on 2nd [RFC] patch which I posted.
> At first, I believe Oops on unbind/bind issue was solved on it.
> 2nd, if my understanding was correct, it doesn't have double-free issue,
> or something like that.
> But, I'm not 100% sure about 2nd, thus it has [RFC] on patch.

Ah, I see that snd_soc_unregister_card() has the check of
card->instantiated, so it should be fine to call multiple times.


thanks,

Takashi

Re: [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Takashi Iwai]

On Mon, 03 Apr 2017 10:26:05 +0200,
Kuninori Morimoto wrote:
> 
> 
> Hi Takashi-san
> 
> > > So, we can call snd_soc_unregister_card() whenever CPU/Codec/Platform
> > > were unregsiterd.
> > 
> > In theory yes, but you should be careful to do so, e.g. make sure that
> > it won't be called again by the removal/unbind of other components /
> > drivers.
> > 
> > I suggested snd_card_disconnect() because it doesn't release resources
> > by itself, but it just disconnects from the further accesses.  So,
> > double-free won't happen in this case.  It makes the hotunplug safer
> > as long as the drivers manage the resource releases properly.
> 
> I had checked many unbind/bind pattern/order on 2nd [RFC] patch which I posted.
> At first, I believe Oops on unbind/bind issue was solved on it.
> 2nd, if my understanding was correct, it doesn't have double-free issue,
> or something like that.
> But, I'm not 100% sure about 2nd, thus it has [RFC] on patch.

Ah, I see that snd_soc_unregister_card() has the check of
card->instantiated, so it should be fine to call multiple times.


thanks,

Takashi

Re: [alsa-devel] [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Mark Brown]

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

On Fri, Mar 31, 2017 at 09:48:02AM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > flag directly when we see a problem call a function to do it.  That way
> > if we want to improve things in the future we can do that without having
> > to update the callers again.

> BTW, ALSA core has snd_card_disconnect() that does this kind of
> shut-up from user-space.  It was introduced for hot-unplug, but
> basically unbinding is the software hot-unplug.  So, if ASoC won't
> rebind a once-unbound component, you can simply call
> snd_card_disconnect() at the component unbinding time to assure that
> no further user actions can be done.

Ah, that's exactly the sort of improvement I was thinking of!

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

Re: [alsa-devel] [RFC][PATCH] ASoC: soc-core: verify Sound Card normality

[Mark Brown]

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

On Mon, Apr 03, 2017 at 10:37:42AM +0200, Takashi Iwai wrote:

> Ah, I see that snd_soc_unregister_card() has the check of
> card->instantiated, so it should be fine to call multiple times.

Yeah, we'd run into that often enough that it's worth handling nicely.

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