From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [PATCH v2] enic: fix seg fault when releasing queues Date: Thu, 9 Jun 2016 17:08:08 +0100 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 Cc: dev@dpdk.org To: John Daley 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) Content-Disposition: inline In-Reply-To: <1464230700-13341-1-git-send-email-johndale@cisco.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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