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>
*** 22,31 ****
--- 22,35 ----
/*
* 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,114 ****
#include <sys/tsol/tnet.h>
/*
* Release a reference on ip_xmit_attr.
* The reference is acquired by conn_get_ixa()
*/
#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); \
atomic_inc_32(&(ixa)->ixa_refcnt); \
}
/*
* When we need to handle a transmit side asynchronous operation, then we need
--- 99,121 ----
#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) \
{ \
! 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,783 ****
}
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;
mutex_enter(&connp->conn_lock);
! ixa = 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 */
mutex_exit(&connp->conn_lock);
! return (ixa);
}
ixa = kmem_alloc(sizeof (*ixa), kmflag);
if (ixa == NULL) {
mutex_exit(&connp->conn_lock);
! ixa_refrele(connp->conn_ixa);
return (NULL);
}
! ixa_safe_copy(connp->conn_ixa, 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);
--- 751,795 ----
}
static ip_xmit_attr_t *
conn_get_ixa_impl(conn_t *connp, boolean_t replace, int kmflag)
{
! 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);
! oldixa = connp->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 (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(oldixa);
return (NULL);
}
! ixa_safe_copy(oldixa, ixa);
/* Make sure we drop conn_lock before any refrele */
if (replace) {
ixa->ixa_refcnt++; /* No atomic needed - not visible */
connp->conn_ixa = ixa;
mutex_exit(&connp->conn_lock);
IXA_REFRELE(oldixa); /* Undo refcnt from conn_t */
} else {
mutex_exit(&connp->conn_lock);
}
IXA_REFRELE(oldixa); /* Undo above atomic_add_32_nv */
return (ixa);
*** 845,874 ****
* 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 *ixa;
mutex_enter(&connp->conn_lock);
- ixa = connp->conn_ixa;
! /* At least one references for the conn_t */
! ASSERT(ixa->ixa_refcnt >= 1);
! /* 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) {
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);
return (ixa);
}
void
ixa_safe_copy(ip_xmit_attr_t *src, ip_xmit_attr_t *ixa)
--- 857,881 ----
* 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);
! oldixa = connp->conn_ixa;
! IXA_REFHOLD(oldixa);
! ixa_safe_copy(oldixa, ixa);
mutex_exit(&connp->conn_lock);
! IXA_REFRELE(oldixa);
return (ixa);
}
void
ixa_safe_copy(ip_xmit_attr_t *src, ip_xmit_attr_t *ixa)
*** 1355,1365 ****
conn_t *, connp);
return;
}
}
ixa_cleanup_stale(ixa);
! ixa_refrele(ixa);
}
}
/*
* ixa needs to be an exclusive copy so that no one changes the cookie
--- 1362,1372 ----
conn_t *, connp);
return;
}
}
ixa_cleanup_stale(ixa);
! IXA_REFRELE(ixa);
}
}
/*
* ixa needs to be an exclusive copy so that no one changes the cookie