From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 9E01928BF for ; Thu, 9 Jun 2016 18:08:12 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga104.fm.intel.com with ESMTP; 09 Jun 2016 09:08:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,445,1459839600"; d="scan'208";a="984221514" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.71]) by fmsmga001.fm.intel.com with SMTP; 09 Jun 2016 09:08:09 -0700 Received: by (sSMTP sendmail emulation); Thu, 09 Jun 2016 17:08:09 +0025 Date: Thu, 9 Jun 2016 17:08:08 +0100 From: Bruce Richardson To: John Daley Cc: dev@dpdk.org Message-ID: <20160609160808.GJ12520@bricha3-MOBL3> References: <1464207514-4406-1-git-send-email-johndale@cisco.com> <1464230700-13341-1-git-send-email-johndale@cisco.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1464230700-13341-1-git-send-email-johndale@cisco.com> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v2] enic: fix seg fault when releasing queues X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Jun 2016 16:08:13 -0000 On Wed, May 25, 2016 at 07:45:00PM -0700, John Daley wrote: > If device configuration failed due to a lack of resources, like if > there were more queues requested than available, the queue release > function is called with NULL pointers which were being dereferenced. > > Skip releasing queues if they are NULL pointers. Also, if configuration > fails due to lack of resources, be more specific about which resources > are lacking. The "also", and a a review of the code, indicates that the error message changes should be a separate patch, as it's not related to the NULL check fix. :-) > > Fixes: fefed3d1e62c ("enic: new driver") > Signed-off-by: John Daley > --- > v2: Log an error for all resource deficiencies not just the first one > found. > > drivers/net/enic/enic_main.c | 37 +++++++++++++++++++++++-------------- > 1 file changed, 23 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c > index 996f999..411d23c 100644 > --- a/drivers/net/enic/enic_main.c > +++ b/drivers/net/enic/enic_main.c > @@ -426,14 +426,16 @@ int enic_alloc_intr_resources(struct enic *enic) > > void enic_free_rq(void *rxq) > { > - struct vnic_rq *rq = (struct vnic_rq *)rxq; > - struct enic *enic = vnic_dev_priv(rq->vdev); > + if (rxq != NULL) { > + struct vnic_rq *rq = (struct vnic_rq *)rxq; > + struct enic *enic = vnic_dev_priv(rq->vdev); > > - enic_rxmbuf_queue_release(enic, rq); > - rte_free(rq->mbuf_ring); > - rq->mbuf_ring = NULL; > - vnic_rq_free(rq); > - vnic_cq_free(&enic->cq[rq->index]); > + enic_rxmbuf_queue_release(enic, rq); > + rte_free(rq->mbuf_ring); > + rq->mbuf_ring = NULL; > + vnic_rq_free(rq); > + vnic_cq_free(&enic->cq[rq->index]); > + } > } Is it not just easier to put a check at the top for: if (rq == NULL) return; Rather than putting the whole body of the function inside an if statement? It would certainly be a smaller diff. /Bruce