Re: [RFC] slot allocator - waitqueue use review needed (Re: Orangefs ABI documentation)

[Al Viro <viro@ZenIV.linux.org.uk>]

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 <viro@zeniv.linux.org.uk>
---
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;						\
 		}							\