linux-kernel - Re: [PATCH] drm/tegra: submit: No need for Null pointer check before kfree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <280170a7-de12-f362-cda3-11208ead0a88@kapsi.fi>
Date:   Wed, 28 Dec 2022 15:17:59 +0200
From:   Mikko Perttunen <cyndis@...si.fi>
To:     Deepak R Varma <drv@...lo.com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Jonathan Hunter <jonathanh@...dia.com>,
        dri-devel@...ts.freedesktop.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Saurabh Singh Sengar <ssengar@...rosoft.com>,
        Praveen Kumar <kumarpraveen@...ux.microsoft.com>
Subject: Re: [PATCH] drm/tegra: submit: No need for Null pointer check before
 kfree

On 12/28/22 15:08, Deepak R Varma wrote:
> On Wed, Dec 28, 2022 at 02:28:54PM +0200, Mikko Perttunen wrote:
>> On 12/27/22 19:14, Deepak R Varma wrote:
>>> kfree() & vfree() internally perform NULL check on the pointer handed
>>> to it and take no action if it indeed is NULL. Hence there is no need
>>> for a pre-check of the memory pointer before handing it to
>>> kfree()/vfree().
>>>
>>> Issue reported by ifnullfree.cocci Coccinelle semantic patch script.
>>>
>>> Signed-off-by: Deepak R Varma <drv@...lo.com>
>>> ---
>>>    drivers/gpu/drm/tegra/submit.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/submit.c b/drivers/gpu/drm/tegra/submit.c
>>> index 066f88564169..06f836db99d0 100644
>>> --- a/drivers/gpu/drm/tegra/submit.c
>>> +++ b/drivers/gpu/drm/tegra/submit.c
>>> @@ -680,8 +680,8 @@ int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
>>>    		kfree(job_data->used_mappings);
>>>    	}
>>>
>>> -	if (job_data)
>>> -		kfree(job_data);
>>> +	kfree(job_data);
>>> +
>>>    put_bo:
>>>    	gather_bo_put(&bo->base);
>>>    unlock:
>>> --
>>> 2.34.1
>>>
>>>
>>>
>>
>> It continues to be the case that I think this transform is bad. Same applies
>> to the host1x patch.
> 
> Hello Mikko,
> Thank you for responding to the patch proposal. Could you please explain why is
> this bad?
> 
> Regards,
> ./drv
> 
>>
>> Mikko
> 
> 

Hi,

it gets rid of visual hints on code paths indicating the possible 
liveness of pointer variables. I.e., after the change, whether the 
pointer can be NULL or not is more difficult to reason about locally, 
instead requiring more global reasoning which is mentally more taxing.

Since C's type system doesn't help with tracking these kinds of things, 
I believe it is important to have these kinds of local contextual cues 
to help the programmer.

Mikko