[PATCH v2 2/2] s390/sclp: Ensure no callback gets called after sclp_unregister() returns

[Alexander Egorenkov <egorenar@linux.ibm.com>]

There is a potential race condition between sclp_unregister()
and sclp_dispatch_evbufs()/sclp_dispatch_state_change(). As a result,
it is not guaranteed that the callbacks registered with sclp_register()
will not get called one more time (and no more than one) after
sclp_unregister() returns.

Example of a race situation
===========================

         CPU A (SCLP)                 CPU B (driver using SCLP API)

--> sclp_dispatch_evbufs()
 --> spin_unlock(&sclp_lock)

                                    --> sclp_unregister()
				     --> spin_lock(&sclp_lock)
				     --> remove struct sclp_register from list
				     --> spin_unlock(&sclp_lock)
                                    <-- sclp_unregister()

                                    --> free struct sclp_register

 --> receiver_fn()
  --> illegal memory access

 --> spin_lock(&sclp_lock)
<-- sclp_dispatch_evbufs()

To guarantee that this race situation never occurs, it must be ensured
that sclp_unregister() waits for the last callback invocation by
sclp_dispatch_evbufs()/sclp_dispatch_state_change() to return.
The basic idea is to use a single completion per struct sclp_register
to block in sclp_unregister() until the last callback completes.
It is sufficient to use only one completion per struct sclp_register
because the callbacks state_change_fn() and receiver_fn() never get called
in parallel.

Initially, the completion is marked as done in sclp_register() and every
time when sclp_dispatch_evbufs()/sclp_dispatch_state_change() calls
one of the callbacks, the completion is marked as incomplete before
its invocation and as done afterwards. The call to wait_for_completion()
in sclp_unregister() shall block until the last invocation of the callback
finishes. And if no callback is pending or being invoked by
sclp_dispatch_evbufs()/sclp_dispatch_state_change() then the completion
is always marked as done and sclp_unregister() shall never block in
wait_for_completion() and return quickly.

To mark the completion as incomplete, reinit_completion() is needed and it
must be guaranteed that no racy wait_for_completion() calls are going on in
parallel. This is achieved by calling reinit_completion() with the SCLP
spinlock held. The following situations must be considered:
1. No callback invocation is pending or in progress when
   wait_for_completion() is called from sclp_unregister(). This is
   a regular situation occurring in most of the cases. wait_for_completion()
   will return quickly because the completion is marked as done. When
   wait_for_completion() gets called, the callbacks no longer exist in
   the list sclp_reg_list, and, therefore,
   sclp_dispatch_evbufs()/sclp_dispatch_state_change() would not be able
   to obtain one of them and invoke reinit_completion() while
   a wait_for_completion() call is going on in parallel.
2. One of the callbacks is pending or in progress when
   sclp_unregister() gets called. This is a rare situation.
   The SCLP spinlock is either being held by
   sclp_dispatch_evbufs()/sclp_dispatch_state_change() or not. In the former
   case, sclp_unregister() would block in the SCLP spinlock trying to
   remove the callbacks from the list and not enter wait_for_completion().
   The SCLP spinlock is released after a reinit_complete() call, therefore,
   it will not race with a wait_for_complete() call in sclp_unregister().
   If the SCLP spinlock is not held by
   sclp_dispatch_evbufs()/sclp_dispatch_state_change(), then
   a reinit_complete() call must have been completed already and
   it will not race with a wait_for_complete() call in sclp_unregister().

With this change sclp_unregister() may no longer be invoked
from atomic context or registered callbacks. But this should not be a
problem because this is no regular use case and no driver using
sclp_register()/sclp_unregister() requires this at the moment and
likely should not require it in the future.

Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
---
 drivers/s390/char/sclp.c | 19 +++++++++++++++----
 drivers/s390/char/sclp.h |  2 ++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/char/sclp.c b/drivers/s390/char/sclp.c
index 98e334724a62..8954f4260401 100644
--- a/drivers/s390/char/sclp.c
+++ b/drivers/s390/char/sclp.c
@@ -565,9 +565,11 @@ sclp_dispatch_evbufs(struct sccb_header *sccb)
 				 evbuf, !reg);
 
 		if (reg && reg->receiver_fn) {
+			reinit_completion(&reg->done);
 			spin_unlock_irqrestore(&sclp_lock, flags);
 			reg->receiver_fn(evbuf);
 			spin_lock_irqsave(&sclp_lock, flags);
+			complete(&reg->done);
 		} else if (reg == NULL)
 			rc = -EOPNOTSUPP;
 	}
@@ -773,8 +775,8 @@ sclp_dispatch_state_change(void)
 	sccb_mask_t receive_mask;
 	sccb_mask_t send_mask;
 
+	spin_lock_irqsave(&sclp_lock, flags);
 	do {
-		spin_lock_irqsave(&sclp_lock, flags);
 		reg = NULL;
 		list_for_each(l, &sclp_reg_list) {
 			reg = list_entry(l, struct sclp_register, list);
@@ -788,15 +790,18 @@ sclp_dispatch_state_change(void)
 			} else
 				reg = NULL;
 		}
-		spin_unlock_irqrestore(&sclp_lock, flags);
 		if (reg && reg->state_change_fn) {
+			reinit_completion(&reg->done);
+			spin_unlock_irqrestore(&sclp_lock, flags);
 			/* STCG: State-change callback (b=callback) */
 			sclp_trace(2, "STCG", 0, (u64)reg->state_change_fn,
 				   false);
-
 			reg->state_change_fn(reg);
+			spin_lock_irqsave(&sclp_lock, flags);
+			complete(&reg->done);
 		}
 	} while (reg);
+	spin_unlock_irqrestore(&sclp_lock, flags);
 }
 
 struct sclp_statechangebuf {
@@ -885,6 +890,8 @@ sclp_register(struct sclp_register *reg)
 	/* Trigger initial state change callback */
 	reg->sclp_receive_mask = 0;
 	reg->sclp_send_mask = 0;
+	init_completion(&reg->done);
+	complete(&reg->done);
 	list_add(&reg->list, &sclp_reg_list);
 	spin_unlock_irqrestore(&sclp_lock, flags);
 	rc = sclp_init_mask(1);
@@ -898,7 +905,10 @@ sclp_register(struct sclp_register *reg)
 
 EXPORT_SYMBOL(sclp_register);
 
-/* Unregister event listener. */
+/* Unregister event listener.
+ * It can sleep and, therefore, may not get called from atomic context.
+ * And neither it can get called from a receive_fn() callback because
+ * struct sclp_register must remain valid after the call. */
 void
 sclp_unregister(struct sclp_register *reg)
 {
@@ -911,6 +921,7 @@ sclp_unregister(struct sclp_register *reg)
 	list_del(&reg->list);
 	spin_unlock_irqrestore(&sclp_lock, flags);
 	sclp_init_mask(1);
+	wait_for_completion(&reg->done);
 }
 
 EXPORT_SYMBOL(sclp_unregister);
diff --git a/drivers/s390/char/sclp.h b/drivers/s390/char/sclp.h
index b31a680e0871..b04f6287ccfc 100644
--- a/drivers/s390/char/sclp.h
+++ b/drivers/s390/char/sclp.h
@@ -11,6 +11,7 @@
 
 #include <linux/types.h>
 #include <linux/list.h>
+#include <linux/completion.h>
 #include <asm/asm-extable.h>
 #include <asm/machine.h>
 #include <asm/sclp.h>
@@ -275,6 +276,7 @@ struct sclp_register {
 	void (*state_change_fn)(struct sclp_register *);
 	/* called for events in cp_receive_mask/sclp_receive_mask */
 	void (*receiver_fn)(struct evbuf_header *);
+	struct completion done;
 };
 
 /* externals from sclp.c */
-- 
2.53.0