| Message ID | 87o8xax88g.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0ced7b050224b18ca73e38e7068f36be8e708c06 |
| Headers | show |
| Series | ASoC: fixup topology dai_link remove issue | expand |
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
> 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.
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. > >
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",