From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from zeniv.linux.org.uk ([195.92.253.2]:58262 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755027AbcBPT2m (ORCPT ); Tue, 16 Feb 2016 14:28:42 -0500 Date: Tue, 16 Feb 2016 19:28:39 +0000 From: Al Viro To: Linus Torvalds Cc: Martin Brandenburg , linux-fsdevel , Stephen Rothwell , Ingo Molnar , Mike Marshall Subject: Re: [RFC] slot allocator - waitqueue use review needed (Re: Orangefs ABI documentation) Message-ID: <20160216192838.GA1664@ZenIV.linux.org.uk> References: <20160211004432.GM17997@ZenIV.linux.org.uk> <20160212042757.GP17997@ZenIV.linux.org.uk> <20160213174738.GR17997@ZenIV.linux.org.uk> <20160214025615.GU17997@ZenIV.linux.org.uk> <20160214034608.GV17997@ZenIV.linux.org.uk> <20160216021228.GA17997@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160216021228.GA17997@ZenIV.linux.org.uk> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Feb 16, 2016 at 02:12:28AM +0000, Al Viro wrote: > I wonder if wait_event_interruptible_exclusive_locked{,_irq}() is > vulnerable to the same problem; plain one is used in fuse, irq - in > gadgetfs and the latter looks somewhat fishy in that respect... > > orangefs one fixed, folded and pushed, but I hadn't really looked into > fuse and gadgetfs enough to tell if they have similar problems... fuse_dev_do_read() looks broken - it assumes that there will be an eventual call of request_end() and we'll issue a wakeup there. With the switch to wait_event_interruptible_exclusive_locked() (last summer, AFAICS) that's no longer true - we'll bugger off with -ERESTARTSYS without waking anyone up, even if we were the (exclusive) recepient of wakeup and the signal has arrived right after that. I'm looking through gadgetfs, but it seems that the only user of _irq variant is also broken the same way. It certainly looks like an accident waiting to happen, especially since the regular variants (sans _locked) have subtly different semantics there. Linus, what do you think about the following: [PATCH] Fix the lost wakeup bug in wait_event_interruptible_exclusive_locked() wait_event_interruptible_exclusive() used to have an unpleasant problem - it might have eaten a wakeup, only to be be hit by a signal immediately after that. In such case wakeup wasn't passed to the next waiter. That was fixed in commit 777c6c5 ("wait: prevent exclusive waiter starvation") back in 2009. A year later ..._locked() analogue had been introduced with exact same problem. Passing a wakeup further in such case (exclusive, caught signal after we'd already eaten a wakeup) is always legitimate - that is exactly what would've happened if we caught signal first. __wake_up_common() would skip decrementing nr_exclusive after try_to_wake_up() would return false. Signed-off-by: Al Viro --- diff --git a/include/linux/wait.h b/include/linux/wait.h index 513b36f..675ff32 100644 --- a/include/linux/wait.h +++ b/include/linux/wait.h @@ -575,6 +575,9 @@ do { \ __add_wait_queue_tail(&(wq), &__wait); \ set_current_state(TASK_INTERRUPTIBLE); \ if (signal_pending(current)) { \ + if (exclusive && list_empty(&__wait.task_list)) \ + __wake_up_locked_key(&(wq), \ + TASK_INTERRUPTIBLE, NULL); \ __ret = -ERESTARTSYS; \ break; \ } \