[PATCH] iucv: fix lockless sk_for_each in afiucv_netdev_event

[Haoran Wang]

afiucv_netdev_event() traverses iucv_sk_list.head with sk_for_each()
without holding iucv_sk_list.lock:

    /* net/iucv/af_iucv.c:2218 */
    sk_for_each(sk, &iucv_sk_list.head) {
        iucv = iucv_sk(sk);
        if ((iucv->hs_dev == event_dev) &&
            (sk->sk_state == IUCV_CONNECTED)) {
            if (event == NETDEV_GOING_DOWN)
                iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
            sk->sk_state = IUCV_DISCONN;
            sk->sk_state_change(sk);
        }
    }

The list is mutated under write_lock_bh() by iucv_sock_link() and
iucv_sock_unlink() (called from the socket create/close paths), so a
concurrent close can unlink an sk while the notifier is traversing the
hlist.  The sk_for_each() next pointer may dereference an sk that is
being freed, and the per-socket work then runs on an sk whose lifetime
is not held. 

The other traversals of the same list already take the lock.
iucv_callback_connreq() and afiucv_hs_rcv() both wrap sk_for_each() in
read_lock(&iucv_sk_list.lock) ... read_unlock(). 

Take iucv_sk_list.lock around the traversal, matching the other two
sites.  The per-socket work (iucv_send_ctrl() reaching
sock_alloc_send_skb(), and the state change) may sleep on allocation
and cannot run under read_lock_bh(), so the fix collects the matching
sockets in two passes under the lock.  Count them first, then on the
second pass take a reference with sock_hold() on each and store it
into a kmalloc'd array, drop the lock, and run iucv_send_ctrl(),
sk_state_change() and sock_put() outside it.  sock_hold() pins each
sk for the out-of-lock work, so a concurrent close can no longer free
it from under the notifier.


Signed-off-by: Haoran Wang <haoranwangsec@xxxxxxxxx>
---
 net/iucv/af_iucv.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 4e5cc9da6..d2977b018 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -2214,18 +2214,49 @@ static int afiucv_netdev_event(struct notifier_block *this,
 
 	switch (event) {
 	case NETDEV_REBOOT:
-	case NETDEV_GOING_DOWN:
+	case NETDEV_GOING_DOWN: {
+		struct sock **matches;
+		int i, n = 0, count = 0;
+
+		read_lock(&iucv_sk_list.lock);
+		sk_for_each(sk, &iucv_sk_list.head) {
+			iucv = iucv_sk(sk);
+			if (iucv->hs_dev == event_dev &&
+			    sk->sk_state == IUCV_CONNECTED)
+				count++;
+		}
+		read_unlock(&iucv_sk_list.lock);
+
+		if (!count)
+			break;
+		matches = kmalloc_array(count, sizeof(*matches), GFP_KERNEL);
+		if (!matches)
+			break;
+
+		read_lock(&iucv_sk_list.lock);
 		sk_for_each(sk, &iucv_sk_list.head) {
+			if (n == count)
+				break;
 			iucv = iucv_sk(sk);
-			if ((iucv->hs_dev == event_dev) &&
-			    (sk->sk_state == IUCV_CONNECTED)) {
-				if (event == NETDEV_GOING_DOWN)
-					iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
-				sk->sk_state = IUCV_DISCONN;
-				sk->sk_state_change(sk);
+			if (iucv->hs_dev == event_dev &&
+			    sk->sk_state == IUCV_CONNECTED) {
+				sock_hold(sk);
+				matches[n++] = sk;
 			}
 		}
+		read_unlock(&iucv_sk_list.lock);
+
+		for (i = 0; i < n; i++) {
+			sk = matches[i];
+			if (event == NETDEV_GOING_DOWN)
+				iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
+			sk->sk_state = IUCV_DISCONN;
+			sk->sk_state_change(sk);
+			sock_put(sk);
+		}
+		kfree(matches);
 		break;
+	}
 	case NETDEV_DOWN:
 	case NETDEV_UNREGISTER:
 	default: