From: Takashi Iwai <tiwai@suse.de> To: Jia-Ju Bai <baijiaju1990@gmail.com> Cc: perex@perex.cz, tiwai@suse.com, rfontana@redhat.com, tglx@linutronix.de, allison@lohutok.net, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] ALSA: cmipci: Fix possible a data race in snd_cmipci_interrupt() Date: Sun, 12 Jan 2020 09:20:17 +0100 [thread overview] Message-ID: <s5h5zhhkrwe.wl-tiwai@suse.de> (raw) In-Reply-To: <20200111163027.27135-1-baijiaju1990@gmail.com> On Sat, 11 Jan 2020 17:30:27 +0100, Jia-Ju Bai wrote: > > The functions snd_cmipci_interrupt() and snd_cmipci_capture_trigger() > may be concurrently executed. > > The function snd_cmipci_capture_trigger() calls > snd_cmipci_pcm_trigger(). In snd_cmipci_pcm_trigger(), the variable > rec->running is written with holding a spinlock cm->reg_lock. But in > snd_cmipci_interrupt(), the identical variable cm->channel[0].running > or cm->channel[1].running is read without holding this spinlock. Thus, > a possible data race may occur. > > To fix this data race, in snd_cmipci_interrupt(), the variables > cm->channel[0].running and cm->channel[1].running are read with holding > the spinlock cm->reg_lock. > > This data race is found by the runtime testing of our tool DILP-2. > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> Thanks for the patch. That's indeed a kind of race, but this change won't fix anything in practice, though. The inconsistent running flag between those places, there are two cases: - running became 0 to 1; this cannot happen, as the irq isn't issued before the stream gets started - running became 1 to 0; this means that the stream gets stopped between two points, and it's not better to call snd_pcm_period_elapsed() for an already stopped stream. Takashi > --- > sound/pci/cmipci.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/sound/pci/cmipci.c b/sound/pci/cmipci.c > index dd9d62e2b633..f791152ec48f 100644 > --- a/sound/pci/cmipci.c > +++ b/sound/pci/cmipci.c > @@ -1430,7 +1430,7 @@ static int snd_cmipci_capture_spdif_hw_free(struct snd_pcm_substream *subs) > static irqreturn_t snd_cmipci_interrupt(int irq, void *dev_id) > { > struct cmipci *cm = dev_id; > - unsigned int status, mask = 0; > + unsigned int run_flag0, run_flag1, status, mask = 0; > > /* fastpath out, to ease interrupt sharing */ > status = snd_cmipci_read(cm, CM_REG_INT_STATUS); > @@ -1445,15 +1445,17 @@ static irqreturn_t snd_cmipci_interrupt(int irq, void *dev_id) > mask |= CM_CH1_INT_EN; > snd_cmipci_clear_bit(cm, CM_REG_INT_HLDCLR, mask); > snd_cmipci_set_bit(cm, CM_REG_INT_HLDCLR, mask); > + run_flag0 = cm->channel[0].running; > + run_flag1 = cm->channel[1].running; > spin_unlock(&cm->reg_lock); > > if (cm->rmidi && (status & CM_UARTINT)) > snd_mpu401_uart_interrupt(irq, cm->rmidi->private_data); > > if (cm->pcm) { > - if ((status & CM_CHINT0) && cm->channel[0].running) > + if ((status & CM_CHINT0) && run_flag0) > snd_pcm_period_elapsed(cm->channel[0].substream); > - if ((status & CM_CHINT1) && cm->channel[1].running) > + if ((status & CM_CHINT1) && run_flag1) > snd_pcm_period_elapsed(cm->channel[1].substream); > } > return IRQ_HANDLED; > -- > 2.17.1 >
WARNING: multiple messages have this Message-ID (diff)
From: Takashi Iwai <tiwai@suse.de> To: Jia-Ju Bai <baijiaju1990@gmail.com> Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, tiwai@suse.com, rfontana@redhat.com, tglx@linutronix.de, allison@lohutok.net Subject: Re: [alsa-devel] [PATCH] ALSA: cmipci: Fix possible a data race in snd_cmipci_interrupt() Date: Sun, 12 Jan 2020 09:20:17 +0100 [thread overview] Message-ID: <s5h5zhhkrwe.wl-tiwai@suse.de> (raw) In-Reply-To: <20200111163027.27135-1-baijiaju1990@gmail.com> On Sat, 11 Jan 2020 17:30:27 +0100, Jia-Ju Bai wrote: > > The functions snd_cmipci_interrupt() and snd_cmipci_capture_trigger() > may be concurrently executed. > > The function snd_cmipci_capture_trigger() calls > snd_cmipci_pcm_trigger(). In snd_cmipci_pcm_trigger(), the variable > rec->running is written with holding a spinlock cm->reg_lock. But in > snd_cmipci_interrupt(), the identical variable cm->channel[0].running > or cm->channel[1].running is read without holding this spinlock. Thus, > a possible data race may occur. > > To fix this data race, in snd_cmipci_interrupt(), the variables > cm->channel[0].running and cm->channel[1].running are read with holding > the spinlock cm->reg_lock. > > This data race is found by the runtime testing of our tool DILP-2. > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> Thanks for the patch. That's indeed a kind of race, but this change won't fix anything in practice, though. The inconsistent running flag between those places, there are two cases: - running became 0 to 1; this cannot happen, as the irq isn't issued before the stream gets started - running became 1 to 0; this means that the stream gets stopped between two points, and it's not better to call snd_pcm_period_elapsed() for an already stopped stream. Takashi > --- > sound/pci/cmipci.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/sound/pci/cmipci.c b/sound/pci/cmipci.c > index dd9d62e2b633..f791152ec48f 100644 > --- a/sound/pci/cmipci.c > +++ b/sound/pci/cmipci.c > @@ -1430,7 +1430,7 @@ static int snd_cmipci_capture_spdif_hw_free(struct snd_pcm_substream *subs) > static irqreturn_t snd_cmipci_interrupt(int irq, void *dev_id) > { > struct cmipci *cm = dev_id; > - unsigned int status, mask = 0; > + unsigned int run_flag0, run_flag1, status, mask = 0; > > /* fastpath out, to ease interrupt sharing */ > status = snd_cmipci_read(cm, CM_REG_INT_STATUS); > @@ -1445,15 +1445,17 @@ static irqreturn_t snd_cmipci_interrupt(int irq, void *dev_id) > mask |= CM_CH1_INT_EN; > snd_cmipci_clear_bit(cm, CM_REG_INT_HLDCLR, mask); > snd_cmipci_set_bit(cm, CM_REG_INT_HLDCLR, mask); > + run_flag0 = cm->channel[0].running; > + run_flag1 = cm->channel[1].running; > spin_unlock(&cm->reg_lock); > > if (cm->rmidi && (status & CM_UARTINT)) > snd_mpu401_uart_interrupt(irq, cm->rmidi->private_data); > > if (cm->pcm) { > - if ((status & CM_CHINT0) && cm->channel[0].running) > + if ((status & CM_CHINT0) && run_flag0) > snd_pcm_period_elapsed(cm->channel[0].substream); > - if ((status & CM_CHINT1) && cm->channel[1].running) > + if ((status & CM_CHINT1) && run_flag1) > snd_pcm_period_elapsed(cm->channel[1].substream); > } > return IRQ_HANDLED; > -- > 2.17.1 > _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
next prev parent reply other threads:[~2020-01-12 8:20 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-11 16:30 [PATCH] ALSA: cmipci: Fix possible a data race in snd_cmipci_interrupt() Jia-Ju Bai 2020-01-11 16:30 ` [alsa-devel] " Jia-Ju Bai 2020-01-12 8:20 ` Takashi Iwai [this message] 2020-01-12 8:20 ` Takashi Iwai 2020-01-13 8:20 ` Jia-Ju Bai 2020-01-13 8:20 ` [alsa-devel] " Jia-Ju Bai 2020-01-13 8:40 ` Takashi Iwai 2020-01-13 8:40 ` [alsa-devel] " Takashi Iwai
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=s5h5zhhkrwe.wl-tiwai@suse.de \ --to=tiwai@suse.de \ --cc=allison@lohutok.net \ --cc=alsa-devel@alsa-project.org \ --cc=baijiaju1990@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=perex@perex.cz \ --cc=rfontana@redhat.com \ --cc=tglx@linutronix.de \ --cc=tiwai@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.