| Message ID | 20200111163027.27135-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: cmipci: Fix possible a data race in snd_cmipci_interrupt() | expand |
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 >
On 2020/1/12 16:20, Takashi Iwai wrote: > 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. Thanks for the reply :) I am not sure to understand your words. Do you mean that this code should be also protected by the spinlock? if (cm->pcm) { if ((status & CM_CHINT0) && cm->channel[0].running) snd_pcm_period_elapsed(cm->channel[0].substream); if ((status & CM_CHINT1) && cm->channel[1].running) snd_pcm_period_elapsed(cm->channel[1].substream); } Best wishes, Jia-Ju Bai
On Mon, 13 Jan 2020 09:20:37 +0100, Jia-Ju Bai wrote: > > > > On 2020/1/12 16:20, Takashi Iwai wrote: > > 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. > > Thanks for the reply :) > > I am not sure to understand your words. > > Do you mean that this code should be also protected by the spinlock? > if (cm->pcm) { > if ((status & CM_CHINT0) && cm->channel[0].running) > snd_pcm_period_elapsed(cm->channel[0].substream); > if ((status & CM_CHINT1) && cm->channel[1].running) > snd_pcm_period_elapsed(cm->channel[1].substream); > } No, it can't be protected as it would lead to ABBA deadlock. That said, it's rather safe to leave the code as is. thanks, Takashi
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;
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> --- sound/pci/cmipci.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)