From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ferruh Yigit <ferruh.yigit@intel.com>
Subject: Re: [dpdk-stable] [PATCH] net/vmxnet3: fix dereference
 before null check
Date: Fri, 22 Sep 2017 17:39:24 +0100
Message-ID: <0dc033ed-f3e4-2f31-dede-7f5295d92e3c@intel.com>
References: <20170922123906.13308-1-michalx.k.jastrzebski@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: dev@dpdk.org, deepak.k.jain@intel.com,
 Tomasz Kulasek <tomaszx.kulasek@intel.com>, yongwang@vmware.com,
 stable@dpdk.org
To: Michal Jastrzebski <michalx.k.jastrzebski@intel.com>, skhare@vmware.com
Return-path: <dev-bounces@dpdk.org>
In-Reply-To: <20170922123906.13308-1-michalx.k.jastrzebski@intel.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On 9/22/2017 1:39 PM, Michal Jastrzebski wrote:
> From: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> 
> 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 <tomaszx.kulasek@intel.com>
> ---
>  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);
>  }
>