From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754989AbbJIHqm (ORCPT ); Fri, 9 Oct 2015 03:46:42 -0400 Received: from mx2.suse.de ([195.135.220.15]:52551 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751229AbbJIHqk (ORCPT ); Fri, 9 Oct 2015 03:46:40 -0400 Date: Fri, 09 Oct 2015 09:46:44 +0200 Message-ID: From: Takashi Iwai To: "Kosuke Tatsukawa" Cc: "Jie Yang" , "Mark Brown" , "Jaroslav Kysela" , "Markus Elfring" , "alsa-devel@alsa-project.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] ALSA: seq_oss: fix waitqueue_active without memory barrier in snd-seq-oss In-Reply-To: <17EC94B0A072C34B8DCF0D30AD16044A028747B2@BPXM09GP.gisp.nec.co.jp> References: <17EC94B0A072C34B8DCF0D30AD16044A028747B2@BPXM09GP.gisp.nec.co.jp> 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/24.5 (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 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 09 Oct 2015 02:35:54 +0200, Kosuke Tatsukawa wrote: > > snd_seq_oss_readq_put_event() seems to be missing a memory barrier which > might cause the waker to not notice the waiter and miss sending a > wake_up as in the following figure. > > snd_seq_oss_readq_put_event snd_seq_oss_readq_wait > ------------------------------------------------------------------------ > /* wait_event_interruptible_timeout */ > /* __wait_event_interruptible_timeout */ > /* ___wait_event */ > for (;;) { prepare_to_wait_event(&wq, &__wait, > state); > spin_lock_irqsave(&q->lock, flags); > if (waitqueue_active(&q->midi_sleep)) > /* The CPU might reorder the test for > the waitqueue up here, before > prior writes complete */ > if ((q->qlen>0 || q->head==q->tail) > ... > __ret = schedule_timeout(__ret) > if (q->qlen >= q->maxlen - 1) { > memcpy(&q->q[q->tail], ev, sizeof(*ev)); > q->tail = (q->tail + 1) % q->maxlen; > q->qlen++; > ------------------------------------------------------------------------ > > There are two other place in sound/core/seq/oss/ which have similar > code. The attached patch removes the call to waitqueue_active() leaving > just wake_up() behind. This fixes the problem because the call to > spin_lock_irqsave() in wake_up() will be an ACQUIRE operation. > > I found this issue when I was looking through the linux source code > for places calling waitqueue_active() before wake_up*(), but without > preceding memory barriers, after sending a patch to fix a similar > issue in drivers/tty/n_tty.c (Details about the original issue can be > found here: https://lkml.org/lkml/2015/9/28/849). > > Signed-off-by: Kosuke Tatsukawa Thanks, applied now. Takashi > --- > sound/core/seq/oss/seq_oss_readq.c | 6 ++---- > sound/core/seq/oss/seq_oss_writeq.c | 4 +--- > 2 files changed, 3 insertions(+), 7 deletions(-) > > diff --git a/sound/core/seq/oss/seq_oss_readq.c b/sound/core/seq/oss/seq_oss_readq.c > index ccd8935..046cb58 100644 > --- a/sound/core/seq/oss/seq_oss_readq.c > +++ b/sound/core/seq/oss/seq_oss_readq.c > @@ -91,8 +91,7 @@ snd_seq_oss_readq_clear(struct seq_oss_readq *q) > q->head = q->tail = 0; > } > /* if someone sleeping, wake'em up */ > - if (waitqueue_active(&q->midi_sleep)) > - wake_up(&q->midi_sleep); > + wake_up(&q->midi_sleep); > q->input_time = (unsigned long)-1; > } > > @@ -138,8 +137,7 @@ snd_seq_oss_readq_put_event(struct seq_oss_readq *q, union evrec *ev) > q->qlen++; > > /* wake up sleeper */ > - if (waitqueue_active(&q->midi_sleep)) > - wake_up(&q->midi_sleep); > + wake_up(&q->midi_sleep); > > spin_unlock_irqrestore(&q->lock, flags); > > diff --git a/sound/core/seq/oss/seq_oss_writeq.c b/sound/core/seq/oss/seq_oss_writeq.c > index d50338b..1f6788a 100644 > --- a/sound/core/seq/oss/seq_oss_writeq.c > +++ b/sound/core/seq/oss/seq_oss_writeq.c > @@ -138,9 +138,7 @@ snd_seq_oss_writeq_wakeup(struct seq_oss_writeq *q, abstime_t time) > spin_lock_irqsave(&q->sync_lock, flags); > q->sync_time = time; > q->sync_event_put = 0; > - if (waitqueue_active(&q->sync_sleep)) { > - wake_up(&q->sync_sleep); > - } > + wake_up(&q->sync_sleep); > spin_unlock_irqrestore(&q->sync_lock, flags); > } > > -- > 1.7.1 >