From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 20F40C433E0 for ; Wed, 3 Mar 2021 09:47:42 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 467C364EE6 for ; Wed, 3 Mar 2021 09:47:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 467C364EE6 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 8094C18D0; Wed, 3 Mar 2021 10:46:49 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 8094C18D0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1614764859; bh=xvjmiPeZvk1l7RRonPUHq+e+OmPydUCgzrhKU/qnNTw=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=YsCb/WjMpm1fiGGxTIW2Q8z05IWInt+SirK3WrPLVaQnO3EuKhjd6+QTvpL1jDDeL Fc4wWQwwyFfOqi/QDy+JGe17+XvI4XlJvPxH+NZqsB6z7EoeL3+KMevNjAUvfV4tq0 NBlAB6suC0AQnW25dopkvMjyufOxMCFNIvb+Vv2U= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 08448F80257; Wed, 3 Mar 2021 10:46:49 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 3F6D3F8025E; Wed, 3 Mar 2021 10:46:47 +0100 (CET) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id C29F7F800B2 for ; Wed, 3 Mar 2021 10:46:39 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz C29F7F800B2 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 37D50AD87; Wed, 3 Mar 2021 09:46:39 +0000 (UTC) Date: Wed, 03 Mar 2021 10:46:38 +0100 Message-ID: From: Takashi Iwai To: "Gyeongtaek Lee" Subject: Re: [PATCH v5 1/1] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger() In-Reply-To: <1935972447.41614751502383.JavaMail.epsvc@epcpadp3> References: <1935972447.41614751502383.JavaMail.epsvc@epcpadp3> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Cc: alsa-devel@alsa-project.org, khw0178.kim@samsung.com, 'Kuninori Morimoto' , kimty@samsung.com, lgirdwood@gmail.com, 'Pierre-Louis Bossart' , broonie@kernel.org, hmseo@samsung.com, cpgs@samsung.com, donggyun.ko@samsung.com, s47.kang@samsung.com, pilsun.jang@samsung.com, tkjung@samsung.com X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" On Wed, 03 Mar 2021 07:01:34 +0100, Gyeongtaek Lee wrote: > > If stop by underrun and DPCM BE disconnection is run simultaneously, > data abort can be occurred by the sequence below. > > CPU0 CPU1 > dpcm_be_dai_trigger(): dpcm_be_disconnect(): > > for_each_dpcm_be(fe, stream, dpcm) { > > spin_lock_irqsave(&fe->card->dpcm_lock, flags); > list_del(&dpcm->list_be); > list_del(&dpcm->list_fe); > > spin_unlock_irqrestore(&fe->card->dpcm_lock, flags); > kfree(dpcm); > > struct snd_soc_pcm_runtime *be = dpcm->be; <-- Accessing freed memory > > To prevent this situation, dpcm_lock should be acquired during > iteration of dpcm list in dpcm_be_dai_trigger(). > > Signed-off-by: Gyeongtaek Lee > Cc: stable@vger.kernel.org I don't think this addresses the issues fully. This change fixes the traversal of broken linked list, but it still does use-after-free, because the BE object is being freed while it's executed. Instead, we need a serialization like your previous patch, but in a different way than the spinlock that can't be used for nonatomic case. thanks, Takashi > --- > include/sound/soc-dpcm.h | 5 ++++ > sound/soc/soc-pcm.c | 59 +++++++++++++++++++++++++++++++++------- > 2 files changed, 54 insertions(+), 10 deletions(-) > > diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h > index 0f6c50b17bba..599cd6054bc3 100644 > --- a/include/sound/soc-dpcm.h > +++ b/include/sound/soc-dpcm.h > @@ -103,6 +103,11 @@ struct snd_soc_dpcm_runtime { > int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */ > }; > > +struct snd_soc_dpcm_rtd_list { > + int num_rtds; > + struct snd_soc_pcm_runtime *rtds[0]; > +}; > + > #define for_each_dpcm_fe(be, stream, _dpcm) \ > list_for_each_entry(_dpcm, &(be)->dpcm[stream].fe_clients, list_fe) > > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index 14d85ca1e435..ccda0f77e827 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -2028,15 +2028,51 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream, > return ret; > } > > +static int dpcm_be_list_create(struct snd_soc_pcm_runtime *fe, int stream, > + struct snd_soc_dpcm_rtd_list **be_list) > +{ > + struct snd_soc_dpcm *dpcm; > + unsigned long flags; > + int size, i, ret = 0; > + > + spin_lock_irqsave(&fe->card->dpcm_lock, flags); > + > + size = 0; > + for_each_dpcm_be(fe, stream, dpcm) > + size++; > + > + *be_list = kzalloc(struct_size(*be_list, rtds, size), GFP_ATOMIC); > + if (*be_list) { > + i = 0; > + for_each_dpcm_be(fe, stream, dpcm) > + (*be_list)->rtds[i++] = dpcm->be; > + > + if (i != size) > + dev_err(fe->dev, "ASoC: abnormal change in be clients: %d -> %d\n", > + size, i); > + > + (*be_list)->num_rtds = i; > + } else { > + ret = -ENOMEM; > + } > + > + spin_unlock_irqrestore(&fe->card->dpcm_lock, flags); > + > + return ret; > +} > + > int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, > int cmd) > { > - struct snd_soc_dpcm *dpcm; > - int ret = 0; > + struct snd_soc_dpcm_rtd_list *be_list; > + int i, ret = 0; > > - for_each_dpcm_be(fe, stream, dpcm) { > + ret = dpcm_be_list_create(fe, stream, &be_list); > + if (ret < 0) > + return ret; > > - struct snd_soc_pcm_runtime *be = dpcm->be; > + for (i = 0; i < be_list->num_rtds; i++) { > + struct snd_soc_pcm_runtime *be = be_list->rtds[i]; > struct snd_pcm_substream *be_substream = > snd_soc_dpcm_get_substream(be, stream); > > @@ -2056,7 +2092,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, > > ret = soc_pcm_trigger(be_substream, cmd); > if (ret) > - return ret; > + break; > > be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; > break; > @@ -2066,7 +2102,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, > > ret = soc_pcm_trigger(be_substream, cmd); > if (ret) > - return ret; > + break; > > be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; > break; > @@ -2076,7 +2112,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, > > ret = soc_pcm_trigger(be_substream, cmd); > if (ret) > - return ret; > + break; > > be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; > break; > @@ -2090,7 +2126,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, > > ret = soc_pcm_trigger(be_substream, cmd); > if (ret) > - return ret; > + break; > > be->dpcm[stream].state = SND_SOC_DPCM_STATE_STOP; > break; > @@ -2103,7 +2139,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, > > ret = soc_pcm_trigger(be_substream, cmd); > if (ret) > - return ret; > + break; > > be->dpcm[stream].state = SND_SOC_DPCM_STATE_SUSPEND; > break; > @@ -2116,13 +2152,16 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, > > ret = soc_pcm_trigger(be_substream, cmd); > if (ret) > - return ret; > + break; > > be->dpcm[stream].state = SND_SOC_DPCM_STATE_PAUSED; > break; > } > + if (ret < 0) > + break; > } > > + kfree(be_list); > return ret; > } > EXPORT_SYMBOL_GPL(dpcm_be_dai_trigger); > -- > 2.21.0 > > >