svn commit: r333254 - head/sys/net

[Stephen Hurd]

Author: shurd
Date: Fri May  4 15:20:34 2018
New Revision: 333254
URL: https://svnweb.freebsd.org/changeset/base/333254

Log:
  iflib: fix invalid free during queue allocation failure
  
  In r301567, code was added to cleanup to prevent memory leaks for the
  Tx and Rx ring structs. This code carefully tracked txq and rxq, and
  made sure to free them properly during cleanup.
  
  Because we assigned the txq and rxq pointers into the ctx->ifc_txqs and
  ctx->ifc_rxqs, we carefully reset these pointers to NULL, so that
  cleanup code would not accidentally free the memory twice.
  
  This was changed by r304021 ("Update iflib to support more NIC designs"),
  which removed this resetting of the pointers to NULL, because it re-used
  the txq and rxq pointers as an index into the queue set array.
  
  Unfortunately, the cleanup code was left alone. Thus, if we fail to
  allocate DMA or fail to configure the queues using the drivers ifdi
  methods, we will attempt to free txq and rxq. These variables would now
  incorrectly point to the wrong location, resulting in a page fault.
  
  There are a number of methods to correct this, but ultimately the root
  cause was that we reuse the txq and rxq pointers for two different
  purposes.
  
  Instead, when allocating, store the returned pointer directly into
  ctx->ifc_txqs and ctx->ifc_rxqs. Then, assign this to txq and rxq as
  index pointers before starting the loop to allocate each queue.
  Drop the cleanup code for txq and rxq, and only use ctx->ifc_txqs and
  ctx->ifc_rxqs.
  
  Thus, we no longer need to free txq or rxq under any error flow, and
  intsead rely solely on the pointers stored in ctx->ifc_txqs and
  ctx->ifc_rxqs. This prevents the invalid free(), and ensures that we
  still properly cleanup after ourselves as before when failing to
  allocate.
  
  Submitted by:	Jacob Keller
  Reviewed by:	gallatin, sbruno
  Sponsored by:	Intel Corporation
  Differential Revision:	https://reviews.freebsd.org/D15285

Modified:
  head/sys/net/iflib.c

Modified: head/sys/net/iflib.c
==============================================================================
--- head/sys/net/iflib.c	Fri May  4 15:11:16 2018	(r333253)
+++ head/sys/net/iflib.c	Fri May  4 15:20:34 2018	(r333254)
@@ -4777,11 +4777,8 @@ iflib_queues_alloc(if_ctx_t ctx)
 	KASSERT(ntxqs > 0, ("number of queues per qset must be at least 1"));
 	KASSERT(nrxqs > 0, ("number of queues per qset must be at least 1"));
 
-	txq = NULL;
-	rxq = NULL;
-
 /* Allocate the TX ring struct memory */
-	if (!(txq =
+	if (!(ctx->ifc_txqs =
 	    (iflib_txq_t) malloc(sizeof(struct iflib_txq) *
 	    ntxqsets, M_IFLIB, M_NOWAIT | M_ZERO))) {
 		device_printf(dev, "Unable to allocate TX ring memory\n");
@@ -4790,7 +4787,7 @@ iflib_queues_alloc(if_ctx_t ctx)
 	}
 
 	/* Now allocate the RX */
-	if (!(rxq =
+	if (!(ctx->ifc_rxqs =
 	    (iflib_rxq_t) malloc(sizeof(struct iflib_rxq) *
 	    nrxqsets, M_IFLIB, M_NOWAIT | M_ZERO))) {
 		device_printf(dev, "Unable to allocate RX ring memory\n");
@@ -4798,8 +4795,8 @@ iflib_queues_alloc(if_ctx_t ctx)
 		goto rx_fail;
 	}
 
-	ctx->ifc_txqs = txq;
-	ctx->ifc_rxqs = rxq;
+	txq = ctx->ifc_txqs;
+	rxq = ctx->ifc_rxqs;
 
 	/*
 	 * XXX handle allocation failure
@@ -4957,17 +4954,13 @@ iflib_queues_alloc(if_ctx_t ctx)
 /* XXX handle allocation failure changes */
 err_rx_desc:
 err_tx_desc:
+rx_fail:
 	if (ctx->ifc_rxqs != NULL)
 		free(ctx->ifc_rxqs, M_IFLIB);
 	ctx->ifc_rxqs = NULL;
 	if (ctx->ifc_txqs != NULL)
 		free(ctx->ifc_txqs, M_IFLIB);
 	ctx->ifc_txqs = NULL;
-rx_fail:
-	if (rxq != NULL)
-		free(rxq, M_IFLIB);
-	if (txq != NULL)
-		free(txq, M_IFLIB);
 fail:
 	return (err);
 }