Re: [dpdk-dev] [dpdk-stable] [PATCH] net/vmxnet3: fix dereference before null check

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/25/2017 10:27 AM, Jastrzebski, MichalX K wrote:
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Friday, September 22, 2017 6:39 PM
>> To: Jastrzebski, MichalX K <michalx.k.jastrzebski@intel.com>;
>> skhare@vmware.com
>> Cc: dev@dpdk.org; Jain, Deepak K <deepak.k.jain@intel.com>; Kulasek,
>> TomaszX <tomaszx.kulasek@intel.com>; yongwang@vmware.com;
>> stable@dpdk.org
>> Subject: Re: [dpdk-stable] [PATCH] net/vmxnet3: fix dereference before null
>> check
>>
>> 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()
>>
> 
> Hi Ferruh,
> Ok, we can try to do this as You suggest - we will see 
> what coverity tells about that in the next build.

What I understand is, coverity complains about dereferencing variable
(rq) before NULL check, so removing NULL check should fix the warning.

> As long as vmxnet3_dev_clear_queues() checks if the parameter is NULL,
> We can also classify this issue as False/Positive 
> if our solution in this iteration doesn't help.

Agreed, thanks.

> 
> Michal.
> 

<...>