11556 ip_attr.c functions need to not dereference conn_ixa directly after lock drop
Reviewed by: Jason King <jbk@joyent.com>
Reviewed by: Mike Gerdts <mgerdts@joyent.com>
Reviewed by: Andy Fiddaman <andy@omniosce.org>

@@ -22,10 +22,14 @@
 /*
  * Copyright (c) 2009, 2010, Oracle and/or its affiliates. All rights reserved.
  */
 /* Copyright (c) 1990 Mentat Inc. */
 
+/*
+ * Copyright 2019 Joyent, Inc.
+ */
+
 #include <sys/types.h>
 #include <sys/stream.h>
 #include <sys/strsun.h>
 #include <sys/zone.h>
 #include <sys/ddi.h>

@@ -95,20 +99,23 @@
 #include <sys/tsol/tnet.h>
 
 /*
  * Release a reference on ip_xmit_attr.
  * The reference is acquired by conn_get_ixa()
+ *
+ * This macro has a lowercase function-call version for callers outside
+ * this file.
  */
 #define IXA_REFRELE(ixa)                                        \
 {                                                               \
         if (atomic_dec_32_nv(&(ixa)->ixa_refcnt) == 0)  \
                 ixa_inactive(ixa);                              \
 }
 
 #define IXA_REFHOLD(ixa)                                        \
 {                                                               \
-        ASSERT((ixa)->ixa_refcnt != 0);                         \
+        ASSERT3U((ixa)->ixa_refcnt, !=, 0);                     \
         atomic_inc_32(&(ixa)->ixa_refcnt);                      \
 }
 
 /*
  * When we need to handle a transmit side asynchronous operation, then we need

@@ -744,40 +751,45 @@
 }
 
 static ip_xmit_attr_t *
 conn_get_ixa_impl(conn_t *connp, boolean_t replace, int kmflag)
 {
-        ip_xmit_attr_t  *ixa;
-        ip_xmit_attr_t  *oldixa;
+        ip_xmit_attr_t  *oldixa;        /* Already attached to conn_t */
+        ip_xmit_attr_t  *ixa;           /* New one, which we return. */
 
+        /*
+         * NOTE: If the marked-below common case isn't, move the
+         * kmem_alloc() up here and put a free in what was marked as the
+         * (not really) common case instead.
+         */
+
         mutex_enter(&connp->conn_lock);
-        ixa = connp->conn_ixa;
+        oldixa = connp->conn_ixa;
 
-        /* At least one references for the conn_t */
-        ASSERT(ixa->ixa_refcnt >= 1);
-        if (atomic_inc_32_nv(&ixa->ixa_refcnt) == 2) {
-                /* No other thread using conn_ixa */
+        /* At least one reference for the conn_t */
+        ASSERT3U(oldixa->ixa_refcnt, >=, 1);
+        if (atomic_inc_32_nv(&oldixa->ixa_refcnt) == 2) {
+                /* No other thread using conn_ixa (common case) */
                 mutex_exit(&connp->conn_lock);
-                return (ixa);
+                return (oldixa);
         }
+        /* Do allocation inside-the-conn_lock because it's less common. */
         ixa = kmem_alloc(sizeof (*ixa), kmflag);
         if (ixa == NULL) {
                 mutex_exit(&connp->conn_lock);
-                ixa_refrele(connp->conn_ixa);
+                IXA_REFRELE(oldixa);
                 return (NULL);
         }
-        ixa_safe_copy(connp->conn_ixa, ixa);
+        ixa_safe_copy(oldixa, ixa);
 
         /* Make sure we drop conn_lock before any refrele */
         if (replace) {
                 ixa->ixa_refcnt++;      /* No atomic needed - not visible */
-                oldixa = connp->conn_ixa;
                 connp->conn_ixa = ixa;
                 mutex_exit(&connp->conn_lock);
                 IXA_REFRELE(oldixa);    /* Undo refcnt from conn_t */
         } else {
-                oldixa = connp->conn_ixa;
                 mutex_exit(&connp->conn_lock);
         }
         IXA_REFRELE(oldixa);    /* Undo above atomic_add_32_nv */
 
         return (ixa);

@@ -845,30 +857,25 @@
  * if ip_set_destination needs to be called to re-establish the pointers.
  */
 ip_xmit_attr_t *
 conn_get_ixa_exclusive(conn_t *connp)
 {
+        ip_xmit_attr_t *oldixa;
         ip_xmit_attr_t *ixa;
 
+        ixa = kmem_alloc(sizeof (*ixa), KM_NOSLEEP | KM_NORMALPRI);
+        if (ixa == NULL)
+                return (NULL);
+
         mutex_enter(&connp->conn_lock);
-        ixa = connp->conn_ixa;
 
-        /* At least one references for the conn_t */
-        ASSERT(ixa->ixa_refcnt >= 1);
+        oldixa = connp->conn_ixa;
+        IXA_REFHOLD(oldixa);
 
-        /* Make sure conn_ixa doesn't disappear while we copy it */
-        atomic_inc_32(&ixa->ixa_refcnt);
-
-        ixa = kmem_alloc(sizeof (*ixa), KM_NOSLEEP);
-        if (ixa == NULL) {
+        ixa_safe_copy(oldixa, ixa);
                 mutex_exit(&connp->conn_lock);
-                ixa_refrele(connp->conn_ixa);
-                return (NULL);
-        }
-        ixa_safe_copy(connp->conn_ixa, ixa);
-        mutex_exit(&connp->conn_lock);
-        IXA_REFRELE(connp->conn_ixa);
+        IXA_REFRELE(oldixa);
         return (ixa);
 }
 
 void
 ixa_safe_copy(ip_xmit_attr_t *src, ip_xmit_attr_t *ixa)

@@ -1355,11 +1362,11 @@
                                     conn_t *, connp);
                                 return;
                         }
                 }
                 ixa_cleanup_stale(ixa);
-                ixa_refrele(ixa);
+                IXA_REFRELE(ixa);
         }
 }
 
 /*
  * ixa needs to be an exclusive copy so that no one changes the cookie