Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1453f93b-f6ad-04d8-c493-6c8d2a3678bd@kapsi.fi>
Date:   Wed, 28 Dec 2022 15:48:05 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH] drm/tegra: submit: No need for Null pointer check before
 kfree
Content-Language: en-US
To:     Deepak R Varma <drv@mailo.com>
Cc:     Thierry Reding <thierry.reding@gmail.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Saurabh Singh Sengar <ssengar@microsoft.com>,
        Praveen Kumar <kumarpraveen@linux.microsoft.com>
References: <Y6sn7XptKyk5cbrA@qemulion>
 <864f2fdd-4289-a178-bbf1-c2a6a579c58c@kapsi.fi> <Y6w/4IzoMFsVnCmu@qemulion>
 <280170a7-de12-f362-cda3-11208ead0a88@kapsi.fi> <Y6xF6q6qj+ggEdgN@qemulion>
From:   Mikko Perttunen <cyndis@kapsi.fi>
In-Reply-To: <Y6xF6q6qj+ggEdgN@qemulion>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/28/22 15:34, Deepak R Varma wrote:
> On Wed, Dec 28, 2022 at 03:17:59PM +0200, Mikko Perttunen wrote:
>> 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@mailo.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.
> 
> Hello Mikko,
> That really helps. Thank you for the detailed explanation. I do have an extended
> question though. In this context, when we are ready to release the memory, how
> is it useful to know if it is NULL or not this late in the flow when the scope
> is about to end?

In the current code it doesn't matter, but if someone went to change 
this code (for example to add another release step), and we just had 
'kfree(job_data)', they would have to remember that kfree works with 
NULL pointers, and would have to go looking elsewhere in the code to see 
if it is in fact possible to assume that job_data cannot be NULL here, 
or not. If they forget about kfree working with NULL pointers, which 
wouldn't be that surprising since it is almost always only called with 
non-NULL pointers, they might instead introduce a bug.

In this particular instance it's probably not that bad since immediately 
above we have another 'if' block that checks if job_data is NULL, which 
serves as a hint to the programmer; however, as a general principle it 
stands that having the NULL check here makes it obvious to any reading 
programmer that they any changes they make have to consider if the 
pointer is NULL or not.

> 
> Thanks again!
> ./drv
> 

Thanks!
Mikko

> 
> 
> 
>>
>> Mikko
> 
>