[v3,5/6] drm/i915/gvt: Assign NULL to the pointer after memory free.

Submitted by Aleksei Gimbitskii on April 23, 2019, 12:04 p.m.

Details

Message ID 20190423120413.30929-6-aleksei.gimbitskii@intel.com
State New
Headers show
Series "Fix issues reported by klocwork" ( rev: 2 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

Aleksei Gimbitskii April 23, 2019, 12:04 p.m.
The klocwork static code analyzer complains about using pointer after
being freed, because further we pass it to the gvt_vgpu_err() function.
Assign pointer to be NULL intentionaly, to meet requirements of the code
analyzer.

This patch fixed the issue #648 reported as error by klocwork.

Signed-off-by: Aleksei Gimbitskii <aleksei.gimbitskii@intel.com>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/gvt/gtt.c | 1 +
 1 file changed, 1 insertion(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index 061499a38f4d..927753a59a1e 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -1106,6 +1106,7 @@  static struct intel_vgpu_ppgtt_spt *ppgtt_populate_spt_by_guest_entry(
 
 err_free_spt:
 	ppgtt_free_spt(spt);
+	spt = NULL;
 err:
 	gvt_vgpu_err("fail: shadow page %p guest entry 0x%llx type %d\n",
 		     spt, we->val64, we->type);

Comments


On 2019-04-25 11:14, Zhenyu Wang wrote:
> On 2019.04.23 15:04:12 +0300, Aleksei Gimbitskii wrote:
>> The klocwork static code analyzer complains about using pointer after
>> being freed, because further we pass it to the gvt_vgpu_err() function.
>> Assign pointer to be NULL intentionaly, to meet requirements of the code
>> analyzer.
>>
>> This patch fixed the issue #648 reported as error by klocwork.
>>
>> Signed-off-by: Aleksei Gimbitskii <aleksei.gimbitskii@intel.com>
>> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
>> Cc: Zhi Wang <zhi.a.wang@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gvt/gtt.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
>> index 061499a38f4d..927753a59a1e 100644
>> --- a/drivers/gpu/drm/i915/gvt/gtt.c
>> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
>> @@ -1106,6 +1106,7 @@ static struct intel_vgpu_ppgtt_spt *ppgtt_populate_spt_by_guest_entry(
>>   
>>   err_free_spt:
>>   	ppgtt_free_spt(spt);
>> +	spt = NULL;
>>   err:
>>   	gvt_vgpu_err("fail: shadow page %p guest entry 0x%llx type %d\n",
>>   		     spt, we->val64, we->type);
> I think we can remove this error message which doesn't tell the reason
> to fail at all, but have err message in earlier path where we can
> indicate the reason.

Seem like miss my comment in v2.

I would suggest add different msg before the two "goto err_free_spt",

then in this err_free_stp label you could just return without print the

same msg as err. Otherwise you can't tell the reason from the same err.

>
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
Thanks for the reminding and sorry that I forget to mention that.  But we think that if we add this error message here, probably we also need to add the error message in other similar places, which seems a bit over too much. How about we still follow the existing style?

-----Original Message-----
From: Xu, Colin 

Sent: Thursday, April 25, 2019 8:32 AM
To: Gimbitskii, Aleksei <aleksei.gimbitskii@intel.com>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
Subject: Re: [PATCH v3 5/6] drm/i915/gvt: Assign NULL to the pointer after memory free.


On 2019-04-25 11:14, Zhenyu Wang wrote:
> On 2019.04.23 15:04:12 +0300, Aleksei Gimbitskii wrote:

>> The klocwork static code analyzer complains about using pointer after 

>> being freed, because further we pass it to the gvt_vgpu_err() function.

>> Assign pointer to be NULL intentionaly, to meet requirements of the 

>> code analyzer.

>>

>> This patch fixed the issue #648 reported as error by klocwork.

>>

>> Signed-off-by: Aleksei Gimbitskii <aleksei.gimbitskii@intel.com>

>> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>

>> Cc: Zhi Wang <zhi.a.wang@intel.com>

>> ---

>>   drivers/gpu/drm/i915/gvt/gtt.c | 1 +

>>   1 file changed, 1 insertion(+)

>>

>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c 

>> b/drivers/gpu/drm/i915/gvt/gtt.c index 061499a38f4d..927753a59a1e 

>> 100644

>> --- a/drivers/gpu/drm/i915/gvt/gtt.c

>> +++ b/drivers/gpu/drm/i915/gvt/gtt.c

>> @@ -1106,6 +1106,7 @@ static struct intel_vgpu_ppgtt_spt 

>> *ppgtt_populate_spt_by_guest_entry(

>>   

>>   err_free_spt:

>>   	ppgtt_free_spt(spt);

>> +	spt = NULL;

>>   err:

>>   	gvt_vgpu_err("fail: shadow page %p guest entry 0x%llx type %d\n",

>>   		     spt, we->val64, we->type);

> I think we can remove this error message which doesn't tell the reason 

> to fail at all, but have err message in earlier path where we can 

> indicate the reason.


Seem like miss my comment in v2.

I would suggest add different msg before the two "goto err_free_spt",

then in this err_free_stp label you could just return without print the

same msg as err. Otherwise you can't tell the reason from the same err.

>

> _______________________________________________

> intel-gvt-dev mailing list

> intel-gvt-dev@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev


--
Best Regards,
Colin Xu

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
On 2019-04-29 20:22, Gimbitskii, Aleksei wrote:
> Thanks for the reminding and sorry that I forget to mention that.  But we think that if we add this error message here, probably we also need to add the error message in other similar places, which seems a bit over too much. How about we still follow the existing style?

Then it's OK to keep current style. If the err msg can't distinguish the error reasons,
we could craft another patch to cleanup all relative msgs together.

What do you think, Zhi?

>
> -----Original Message-----
> From: Xu, Colin
> Sent: Thursday, April 25, 2019 8:32 AM
> To: Gimbitskii, Aleksei <aleksei.gimbitskii@intel.com>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
> Subject: Re: [PATCH v3 5/6] drm/i915/gvt: Assign NULL to the pointer after memory free.
>
>
> On 2019-04-25 11:14, Zhenyu Wang wrote:
>> On 2019.04.23 15:04:12 +0300, Aleksei Gimbitskii wrote:
>>> The klocwork static code analyzer complains about using pointer after
>>> being freed, because further we pass it to the gvt_vgpu_err() function.
>>> Assign pointer to be NULL intentionaly, to meet requirements of the
>>> code analyzer.
>>>
>>> This patch fixed the issue #648 reported as error by klocwork.
>>>
>>> Signed-off-by: Aleksei Gimbitskii <aleksei.gimbitskii@intel.com>
>>> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
>>> Cc: Zhi Wang <zhi.a.wang@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gvt/gtt.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c
>>> b/drivers/gpu/drm/i915/gvt/gtt.c index 061499a38f4d..927753a59a1e
>>> 100644
>>> --- a/drivers/gpu/drm/i915/gvt/gtt.c
>>> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
>>> @@ -1106,6 +1106,7 @@ static struct intel_vgpu_ppgtt_spt
>>> *ppgtt_populate_spt_by_guest_entry(
>>>    
>>>    err_free_spt:
>>>    	ppgtt_free_spt(spt);
>>> +	spt = NULL;
>>>    err:
>>>    	gvt_vgpu_err("fail: shadow page %p guest entry 0x%llx type %d\n",
>>>    		     spt, we->val64, we->type);
>> I think we can remove this error message which doesn't tell the reason
>> to fail at all, but have err message in earlier path where we can
>> indicate the reason.
> Seem like miss my comment in v2.
>
> I would suggest add different msg before the two "goto err_free_spt",
>
> then in this err_free_stp label you could just return without print the
>
> same msg as err. Otherwise you can't tell the reason from the same err.
>
>> _______________________________________________
>> intel-gvt-dev mailing list
>> intel-gvt-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> --
> Best Regards,
> Colin Xu
>