svn commit: r323594 - head/sys/kern

[Gleb Smirnoff]

Author: glebius
Date: Thu Sep 14 18:05:54 2017
New Revision: 323594
URL: https://svnweb.freebsd.org/changeset/base/323594

Log:
  Fix locking in soisconnected().
  
  When a newborn socket moves from incomplete queue to complete
  one, we need to obtain the listening socket lock after the child,
  which is a wrong order.  The old code did that in potentially
  endless loop of mtx_trylock().  The new one does only one attempt
  of mtx_trylock(), and in case of failure references listening
  socket, unlocks child and locks everything in right order.  In
  case if listening socket shuts down during that, just bail out.
  
  Reported & tested by:	Jason Eggleston <jeggleston llnw.com>
  Reported & tested by:	Jason Wolfe <jason llnw.com>

Modified:
  head/sys/kern/uipc_socket.c

Modified: head/sys/kern/uipc_socket.c
==============================================================================
--- head/sys/kern/uipc_socket.c	Thu Sep 14 17:29:51 2017	(r323593)
+++ head/sys/kern/uipc_socket.c	Thu Sep 14 18:05:54 2017	(r323594)
@@ -3688,24 +3688,41 @@ soisconnecting(struct socket *so)
 void
 soisconnected(struct socket *so)
 {
-	struct socket *head;
-	int ret;
 
-	/*
-	 * XXXGL: this is the only place where we acquire socket locks
-	 * in reverse order: first child, then listening socket.  To
-	 * avoid possible LOR, use try semantics.
-	 */
-restart:
 	SOCK_LOCK(so);
-	if ((head = so->so_listen) != NULL &&
-	    __predict_false(SOLISTEN_TRYLOCK(head) == 0)) {
-		SOCK_UNLOCK(so);
-		goto restart;
-	}
 	so->so_state &= ~(SS_ISCONNECTING|SS_ISDISCONNECTING|SS_ISCONFIRMING);
 	so->so_state |= SS_ISCONNECTED;
-	if (head != NULL && (so->so_qstate == SQ_INCOMP)) {
+
+	if (so->so_qstate == SQ_INCOMP) {
+		struct socket *head = so->so_listen;
+		int ret;
+
+		KASSERT(head, ("%s: so %p on incomp of NULL", __func__, so));
+		/*
+		 * Promoting a socket from incomplete queue to complete, we
+		 * need to go through reverse order of locking.  We first do
+		 * trylock, and if that doesn't succeed, we go the hard way
+		 * leaving a reference and rechecking consistency after proper
+		 * locking.
+		 */
+		if (__predict_false(SOLISTEN_TRYLOCK(head) == 0)) {
+			soref(head);
+			SOCK_UNLOCK(so);
+			SOLISTEN_LOCK(head);
+			SOCK_LOCK(so);
+			if (__predict_false(head != so->so_listen)) {
+				/*
+				 * The socket went off the listen queue,
+				 * should be lost race to close(2) of sol.
+				 * The socket is about to soabort().
+				 */
+				SOCK_UNLOCK(so);
+				sorele(head);
+				return;
+			}
+			/* Not the last one, as so holds a ref. */
+			refcount_release(&head->so_count);
+		}
 again:
 		if ((so->so_options & SO_ACCEPTFILTER) == 0) {
 			TAILQ_REMOVE(&head->sol_incomp, so, so_list);
@@ -3734,8 +3751,6 @@ again:
 		}
 		return;
 	}
-	if (head != NULL)
-		SOLISTEN_UNLOCK(head);
 	SOCK_UNLOCK(so);
 	wakeup(&so->so_timeo);
 	sorwakeup(so);