Print this page
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>

Split Close
Expand all
Collapse all
          --- old/usr/src/uts/common/inet/ip/ip_attr.c
          +++ new/usr/src/uts/common/inet/ip/ip_attr.c
↓ open down ↓ 16 lines elided ↑ open up ↑
  17   17   * information: Portions Copyright [yyyy] [name of copyright owner]
  18   18   *
  19   19   * CDDL HEADER END
  20   20   */
  21   21  
  22   22  /*
  23   23   * Copyright (c) 2009, 2010, Oracle and/or its affiliates. All rights reserved.
  24   24   */
  25   25  /* Copyright (c) 1990 Mentat Inc. */
  26   26  
       27 +/*
       28 + * Copyright 2019 Joyent, Inc.
       29 + */
       30 +
  27   31  #include <sys/types.h>
  28   32  #include <sys/stream.h>
  29   33  #include <sys/strsun.h>
  30   34  #include <sys/zone.h>
  31   35  #include <sys/ddi.h>
  32   36  #include <sys/sunddi.h>
  33   37  #include <sys/cmn_err.h>
  34   38  #include <sys/debug.h>
  35   39  #include <sys/atomic.h>
  36   40  
↓ open down ↓ 53 lines elided ↑ open up ↑
  90   94  #include <inet/sctp/sctp_impl.h>
  91   95  #include <inet/udp_impl.h>
  92   96  #include <sys/sunddi.h>
  93   97  
  94   98  #include <sys/tsol/label.h>
  95   99  #include <sys/tsol/tnet.h>
  96  100  
  97  101  /*
  98  102   * Release a reference on ip_xmit_attr.
  99  103   * The reference is acquired by conn_get_ixa()
      104 + *
      105 + * This macro has a lowercase function-call version for callers outside
      106 + * this file.
 100  107   */
 101  108  #define IXA_REFRELE(ixa)                                        \
 102  109  {                                                               \
 103  110          if (atomic_dec_32_nv(&(ixa)->ixa_refcnt) == 0)  \
 104  111                  ixa_inactive(ixa);                              \
 105  112  }
 106  113  
 107  114  #define IXA_REFHOLD(ixa)                                        \
 108  115  {                                                               \
 109      -        ASSERT((ixa)->ixa_refcnt != 0);                         \
      116 +        ASSERT3U((ixa)->ixa_refcnt, !=, 0);                     \
 110  117          atomic_inc_32(&(ixa)->ixa_refcnt);                      \
 111  118  }
 112  119  
 113  120  /*
 114  121   * When we need to handle a transmit side asynchronous operation, then we need
 115  122   * to save sufficient information so that we can call the fragment and postfrag
 116  123   * functions. That information is captured in an mblk containing this structure.
 117  124   *
 118  125   * Since this is currently only used for IPsec, we include information for
 119  126   * the kernel crypto framework.
↓ open down ↓ 24 lines elided ↑ open up ↑
 144  151          pid_t           ixm_cpid;       /* For getpeerucred */
 145  152  
 146  153          ts_label_t      *ixm_tsl;       /* Refhold if set. */
 147  154  
 148  155          /*
 149  156           * When the pointers below are set they have a refhold on the struct.
 150  157           */
 151  158          ipsec_latch_t           *ixm_ipsec_latch;
 152  159          struct ipsa_s           *ixm_ipsec_ah_sa;       /* SA for AH */
 153  160          struct ipsa_s           *ixm_ipsec_esp_sa;      /* SA for ESP */
 154      -        struct ipsec_policy_s   *ixm_ipsec_policy;      /* why are we here? */
      161 +        struct ipsec_policy_s   *ixm_ipsec_policy;      /* why are we here? */
 155  162          struct ipsec_action_s   *ixm_ipsec_action; /* For reflected packets */
 156  163  
 157  164          ipsa_ref_t              ixm_ipsec_ref[2]; /* Soft reference to SA */
 158  165  
 159  166          /* Need these while waiting for SA */
 160  167          uint16_t ixm_ipsec_src_port;    /* Source port number of d-gram. */
 161  168          uint16_t ixm_ipsec_dst_port;    /* Destination port number of d-gram. */
 162  169          uint8_t  ixm_ipsec_icmp_type;   /* ICMP type of d-gram */
 163  170          uint8_t  ixm_ipsec_icmp_code;   /* ICMP code of d-gram */
 164  171  
↓ open down ↓ 574 lines elided ↑ open up ↑
 739  746          ASSERT(irm->irm_inbound);
 740  747          return (B_TRUE);
 741  748  #else
 742  749          return (DB_TYPE(mp) == M_BREAK);
 743  750  #endif
 744  751  }
 745  752  
 746  753  static ip_xmit_attr_t *
 747  754  conn_get_ixa_impl(conn_t *connp, boolean_t replace, int kmflag)
 748  755  {
 749      -        ip_xmit_attr_t  *ixa;
 750      -        ip_xmit_attr_t  *oldixa;
      756 +        ip_xmit_attr_t  *oldixa;        /* Already attached to conn_t */
      757 +        ip_xmit_attr_t  *ixa;           /* New one, which we return. */
 751  758  
      759 +        /*
      760 +         * NOTE: If the marked-below common case isn't, move the
      761 +         * kmem_alloc() up here and put a free in what was marked as the
      762 +         * (not really) common case instead.
      763 +         */
      764 +
 752  765          mutex_enter(&connp->conn_lock);
 753      -        ixa = connp->conn_ixa;
      766 +        oldixa = connp->conn_ixa;
 754  767  
 755      -        /* At least one references for the conn_t */
 756      -        ASSERT(ixa->ixa_refcnt >= 1);
 757      -        if (atomic_inc_32_nv(&ixa->ixa_refcnt) == 2) {
 758      -                /* No other thread using conn_ixa */
      768 +        /* At least one reference for the conn_t */
      769 +        ASSERT3U(oldixa->ixa_refcnt, >=, 1);
      770 +        if (atomic_inc_32_nv(&oldixa->ixa_refcnt) == 2) {
      771 +                /* No other thread using conn_ixa (common case) */
 759  772                  mutex_exit(&connp->conn_lock);
 760      -                return (ixa);
      773 +                return (oldixa);
 761  774          }
      775 +        /* Do allocation inside-the-conn_lock because it's less common. */
 762  776          ixa = kmem_alloc(sizeof (*ixa), kmflag);
 763  777          if (ixa == NULL) {
 764  778                  mutex_exit(&connp->conn_lock);
 765      -                ixa_refrele(connp->conn_ixa);
      779 +                IXA_REFRELE(oldixa);
 766  780                  return (NULL);
 767  781          }
 768      -        ixa_safe_copy(connp->conn_ixa, ixa);
      782 +        ixa_safe_copy(oldixa, ixa);
 769  783  
 770  784          /* Make sure we drop conn_lock before any refrele */
 771  785          if (replace) {
 772  786                  ixa->ixa_refcnt++;      /* No atomic needed - not visible */
 773      -                oldixa = connp->conn_ixa;
 774  787                  connp->conn_ixa = ixa;
 775  788                  mutex_exit(&connp->conn_lock);
 776  789                  IXA_REFRELE(oldixa);    /* Undo refcnt from conn_t */
 777  790          } else {
 778      -                oldixa = connp->conn_ixa;
 779  791                  mutex_exit(&connp->conn_lock);
 780  792          }
 781  793          IXA_REFRELE(oldixa);    /* Undo above atomic_add_32_nv */
 782  794  
 783  795          return (ixa);
 784  796  }
 785  797  
 786  798  /*
 787  799   * Return an ip_xmit_attr_t to use with a conn_t that ensures that only
 788  800   * the caller can access the ip_xmit_attr_t.
↓ open down ↓ 51 lines elided ↑ open up ↑
 840  852   * separate from conn_ixa.
 841  853   *
 842  854   * This "safe" copy has the pointers set to NULL
 843  855   * (since the pointers might be changed by another thread using
 844  856   * conn_ixa). The caller needs to check for NULL pointers to see
 845  857   * if ip_set_destination needs to be called to re-establish the pointers.
 846  858   */
 847  859  ip_xmit_attr_t *
 848  860  conn_get_ixa_exclusive(conn_t *connp)
 849  861  {
      862 +        ip_xmit_attr_t *oldixa;
 850  863          ip_xmit_attr_t *ixa;
 851  864  
      865 +        ixa = kmem_alloc(sizeof (*ixa), KM_NOSLEEP | KM_NORMALPRI);
      866 +        if (ixa == NULL)
      867 +                return (NULL);
      868 +
 852  869          mutex_enter(&connp->conn_lock);
 853      -        ixa = connp->conn_ixa;
 854  870  
 855      -        /* At least one references for the conn_t */
 856      -        ASSERT(ixa->ixa_refcnt >= 1);
      871 +        oldixa = connp->conn_ixa;
      872 +        IXA_REFHOLD(oldixa);
 857  873  
 858      -        /* Make sure conn_ixa doesn't disappear while we copy it */
 859      -        atomic_inc_32(&ixa->ixa_refcnt);
 860      -
 861      -        ixa = kmem_alloc(sizeof (*ixa), KM_NOSLEEP);
 862      -        if (ixa == NULL) {
 863      -                mutex_exit(&connp->conn_lock);
 864      -                ixa_refrele(connp->conn_ixa);
 865      -                return (NULL);
 866      -        }
 867      -        ixa_safe_copy(connp->conn_ixa, ixa);
      874 +        ixa_safe_copy(oldixa, ixa);
 868  875          mutex_exit(&connp->conn_lock);
 869      -        IXA_REFRELE(connp->conn_ixa);
      876 +        IXA_REFRELE(oldixa);
 870  877          return (ixa);
 871  878  }
 872  879  
 873  880  void
 874  881  ixa_safe_copy(ip_xmit_attr_t *src, ip_xmit_attr_t *ixa)
 875  882  {
 876  883          bcopy(src, ixa, sizeof (*ixa));
 877  884          ixa->ixa_refcnt = 1;
 878  885          /*
 879  886           * Clear any pointers that have references and might be changed
↓ open down ↓ 470 lines elided ↑ open up ↑
1350 1357                                   * Somebody else was using it and kmem_alloc
1351 1358                                   * failed! Next memory reclaim will try to
1352 1359                                   * clean up.
1353 1360                                   */
1354 1361                                  DTRACE_PROBE1(conn__ixa__cleanup__bail,
1355 1362                                      conn_t *, connp);
1356 1363                                  return;
1357 1364                          }
1358 1365                  }
1359 1366                  ixa_cleanup_stale(ixa);
1360      -                ixa_refrele(ixa);
     1367 +                IXA_REFRELE(ixa);
1361 1368          }
1362 1369  }
1363 1370  
1364 1371  /*
1365 1372   * ixa needs to be an exclusive copy so that no one changes the cookie
1366 1373   * or the ixa_nce.
1367 1374   */
1368 1375  boolean_t
1369 1376  ixa_check_drain_insert(conn_t *connp, ip_xmit_attr_t *ixa)
1370 1377  {
↓ open down ↓ 55 lines elided ↑ open up ↑
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX