linux-kernel - Re: [PATCH] drm/radeon: cleanup coding style a bit

Open Source and information security mailing list archives

Message-ID: <81368fdb-87c5-d963-68c2-e02d0c100363@gmail.com>
Date:   Thu, 30 Apr 2020 15:32:52 +0200
From:   Christian König <ckoenig.leichtzumerken@...il.com>
To:     Bernard <bernard@...o.com>, Joe Perches <joe@...ches.com>
Cc:     "David (ChunMing) Zhou" <David1.Zhou@....com>,
        opensource.kernel@...o.com, David Airlie <airlied@...ux.ie>,
        linux-kernel@...r.kernel.org, amd-gfx@...ts.freedesktop.org,
        dri-devel@...ts.freedesktop.org, Daniel Vetter <daniel@...ll.ch>,
        Alex Deucher <alexander.deucher@....com>,
        Christian König <christian.koenig@....com>
Subject: Re: [PATCH] drm/radeon: cleanup coding style a bit

Am 30.04.20 um 13:00 schrieb Bernard:
>
> 发件人：Joe Perches <joe@...ches.com>
> 发送日期：2020-04-27 01:53:06
> 收件人："Christian König" <christian.koenig@....com>,Bernard Zhao <bernard@...o.com>,Alex Deucher <alexander.deucher@....com>,"David (ChunMing) Zhou" <David1.Zhou@....com>,David Airlie <airlied@...ux.ie>,Daniel Vetter <daniel@...ll.ch>,amd-gfx@...ts.freedesktop.org,dri-devel@...ts.freedesktop.org,linux-kernel@...r.kernel.org
> 抄送人：opensource.kernel@...o.com
> 主题：Re: [PATCH] drm/radeon: cleanup coding style a bit>On Sun, 2020-04-26 at 15:18 +0200, Christian König wrote:
>>> Am 26.04.20 um 15:12 schrieb Bernard Zhao:
>>>> Maybe no need to check ws before kmalloc, kmalloc will check
>>>> itself, kmalloc`s logic is if ptr is NULL, kmalloc will just
>>>> return
>>>>
>>>> Signed-off-by: Bernard Zhao <bernard@...o.com>
>>> Reviewed-by: Christian König <christian.koenig@....com>
>>>
>>> I'm wondering why the automated scripts haven't found that one before.
>> because this pattern is
>>
>> 	if (foo)
>> 		kfree(bar);
>>
>> and the pattern looked for is:
>>
>> 	if (foo)
>> 		kfree(foo);
>>
>>>> diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c
>> []
>>>> @@ -1211,8 +1211,7 @@ static int atom_execute_table_locked(struct atom_context *ctx, int index, uint32
>>>>    	SDEBUG("<<\n");
>>>>    
>>>>    free:
>>>> -	if (ws)
>>>> -		kfree(ectx.ws);
>>>> +	kfree(ectx.ws);
>>>>    	return ret;
>>>>    }
>> I'm wondering if this removal is correct as the function
>> is named _locked and it may be recursive or called under
>> some external lock.
>>
> Hi
> I am a little confused about this. I understand that the caller guarantees the lock protection
> that we will not release the wrong pointer. And the NULL check is the same with the first check in kfree?
> Maybe we do not need check twich.

I don't understand the comment either. When you look at the function you 
see that code is freeing up the temporary allocated buffer which is to 
large for the stack.

In other words we kcalloc() this buffer a few lines above and free it 
here again. So I think the patch is perfectly valid.

What we could do is to update the coci pattern to catch this as well, 
but this case is so rare that it is probably not worth it.

Regards,
Christian.

>
> Regards,
> Bernard
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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