From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 836342C24; Fri, 22 Sep 2017 18:39:28 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Sep 2017 09:39:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,427,1500966000"; d="scan'208";a="902958803" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.57]) ([10.237.220.57]) by FMSMGA003.fm.intel.com with ESMTP; 22 Sep 2017 09:39:25 -0700 To: Michal Jastrzebski , skhare@vmware.com Cc: dev@dpdk.org, deepak.k.jain@intel.com, Tomasz Kulasek , yongwang@vmware.com, stable@dpdk.org References: <20170922123906.13308-1-michalx.k.jastrzebski@intel.com> From: Ferruh Yigit Message-ID: <0dc033ed-f3e4-2f31-dede-7f5295d92e3c@intel.com> Date: Fri, 22 Sep 2017 17:39:24 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170922123906.13308-1-michalx.k.jastrzebski@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-stable] [PATCH] net/vmxnet3: fix dereference before null check X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Sep 2017 16:39:29 -0000 On 9/22/2017 1:39 PM, Michal Jastrzebski wrote: > From: Tomasz Kulasek > > Coverity error: > > check_after_deref: Null-checking rq suggests that it may be null, but it > has already been dereferenced on all paths leading to > the check. > > This patch moves NULL checking of "rq" at the very beginning of the path > before any dereference. > > Coverity issue: 143468 > Fixes: 5aecdc17a97d ("vmxnet3: fix stop/restart") > Cc: yongwang@vmware.com > Cc: stable@dpdk.org > > Signed-off-by: Tomasz Kulasek > --- > drivers/net/vmxnet3/vmxnet3_rxtx.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c > index d9cf437..4fcceb4 100644 > --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c > +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c > @@ -259,17 +259,16 @@ > { > int i; > vmxnet3_rx_queue_t *rq = rxq; > - struct vmxnet3_hw *hw = rq->hw; > struct vmxnet3_cmd_ring *ring0, *ring1; > struct vmxnet3_comp_ring *comp_ring; > - struct vmxnet3_rx_data_ring *data_ring = &rq->data_ring; > int size; > > - if (rq != NULL) { vmxnet3_dev_rx_queue_reset() is static function and only called from single function [1], which already checks if the parameter is NULL. What do you think just removing this check and keep rest same? [1] vmxnet3_dev_clear_queues() > - /* Release both the cmd_rings mbufs */ > - for (i = 0; i < VMXNET3_RX_CMDRING_SIZE; i++) > - vmxnet3_rx_cmd_ring_release_mbufs(&rq->cmd_ring[i]); > - } > + if (rq == NULL) > + return; > + > + /* Release both the cmd_rings mbufs */ > + for (i = 0; i < VMXNET3_RX_CMDRING_SIZE; i++) > + vmxnet3_rx_cmd_ring_release_mbufs(&rq->cmd_ring[i]); > > ring0 = &rq->cmd_ring[0]; > ring1 = &rq->cmd_ring[1]; > @@ -287,8 +286,8 @@ > > size = sizeof(struct Vmxnet3_RxDesc) * (ring0->size + ring1->size); > size += sizeof(struct Vmxnet3_RxCompDesc) * comp_ring->size; > - if (VMXNET3_VERSION_GE_3(hw) && rq->data_desc_size) > - size += rq->data_desc_size * data_ring->size; > + if (VMXNET3_VERSION_GE_3(rq->hw) && rq->data_desc_size) > + size += rq->data_desc_size * rq->data_ring.size; > > memset(ring0->base, 0, size); > } >