Re: [Linaro-mm-sig] [PATCH] dma-buf: return -EINVAL if dmabuf object is NULL

["Christian König" <christian.koenig@amd.com>]

Am 18.08.21 um 15:13 schrieb Sa, Nuno:
>> From: Christian König <christian.koenig@amd.com>
>> Sent: Wednesday, August 18, 2021 2:58 PM
>> To: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Sa, Nuno <Nuno.Sa@analog.com>; linaro-mm-sig@lists.linaro.org;
>> dri-devel@lists.freedesktop.org; linux-media@vger.kernel.org; Rob
>> Clark <rob@ti.com>
>> Subject: Re: [Linaro-mm-sig] [PATCH] dma-buf: return -EINVAL if
>> dmabuf object is NULL
>>
>> [External]
>>
>> Am 18.08.21 um 14:46 schrieb Daniel Vetter:
>>> On Wed, Aug 18, 2021 at 02:31:34PM +0200, Christian König wrote:
>>>> Am 18.08.21 um 14:17 schrieb Sa, Nuno:
>>>>>> From: Christian König <christian.koenig@amd.com>
>>>>>> Sent: Wednesday, August 18, 2021 2:10 PM
>>>>>> To: Sa, Nuno <Nuno.Sa@analog.com>; linaro-mm-
>> sig@lists.linaro.org;
>>>>>> dri-devel@lists.freedesktop.org; linux-media@vger.kernel.org
>>>>>> Cc: Rob Clark <rob@ti.com>; Sumit Semwal
>>>>>> <sumit.semwal@linaro.org>
>>>>>> Subject: Re: [PATCH] dma-buf: return -EINVAL if dmabuf object
>> is
>>>>>> NULL
>>>>>>
>>>>>> [External]
>>>>>>
>>>>>> To be honest I think the if(WARN_ON(!dmabuf)) return -EINVAL
>>>>>> handling
>>>>>> here is misleading in the first place.
>>>>>>
>>>>>> Returning -EINVAL on a hard coding error is not good practice and
>>>>>> should
>>>>>> probably be removed from the DMA-buf subsystem in general.
>>>>> Would you say to just return 0 then? I don't think that having the
>>>>> dereference is also good..
>>>> No, just run into the dereference.
>>>>
>>>> Passing NULL as the core object you are working on is a hard coding
>> error
>>>> and not something we should bubble up as recoverable error.
>>>>
>>>>> I used -EINVAL to be coherent with the rest of the code.
>>>> I rather suggest to remove the check elsewhere as well.
>>> It's a lot more complicated, and WARN_ON + bail out is rather
>>> well-established code-pattern. There's been plenty of discussions in
>> the
>>> past that a BUG_ON is harmful since it makes debugging a major
>> pain, e.g.
>>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Furldefense.com%2Fv3%2F__https%3A%2F%2Fnam11.safelinks.protection.outl&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C6355660e526b4da23fa408d9624a0160%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637648892261202104%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=pkZg9vDm4RTgmAD6vtugsLmUuL0fG9ExgTWedxOxCW8%3D&amp;reserved=0
>> ook.com/?url=https*3A*2F*2Flore.kernel.org*2Flkml*2FCA*2B55aFw
>> yNTLuZgOWMTRuabWobF27ygskuxvFd-P0n-
>> 3UNT*3D0Og*40mail.gmail.com*2F&amp;data=04*7C01*7Cchristian.k
>> oenig*40amd.com*7C19f53e2a2d1843b65adc08d962463b78*7C3dd896
>> 1fe4884e608e11a82d994e183d*7C0*7C0*7C637648876076613233*7CU
>> nknown*7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
>> CJBTiI6Ik1haWwiLCJXVCI6Mn0*3D*7C1000&amp;sdata=ajyBnjePRak3
>> o7ObpBAuJNd08HgkANM9C*2BgzOAeHrMk*3D&amp;reserved=0__;J
>> SUlJSUlJSUlJSUlJSUlJSUlJSUlJSU!!A3Ni8CS0y2Y!qiDegx4svPUMZrvnzUo
>> X7VKvvFpDcedH9gYbRCiWfe_N3fw4zpmA54qxefvMiQ$
>>> There's also a checkpatch check for this.
>>>
>>> commit 9d3e3c705eb395528fd8f17208c87581b134da48
>>> Author: Joe Perches <joe@perches.com>
>>> Date:   Wed Sep 9 15:37:27 2015 -0700
>>>
>>>       checkpatch: add warning on BUG/BUG_ON use
>>>
>>> Anyone who is paranoid about security crashes their machine on any
>> WARNING
>>> anyway (like syzkaller does).
>>>
>>> My rule of thumb is that if the WARN_ON + bail-out code is just an if
>>> (WARN_ON()) return; then it's fine, if it's more then BUG_ON is the
>> better
>>> choice perhaps.
>>>
>>> I think the worst choice is just removing all these checks, because a
>> few
>>> code reorgs later you might not Oops immediately afterwards
>> anymore, and
>>> then we'll merge potentially very busted new code. Which is no
>> good.
>>
>> Well BUG_ON(some_codition) is a different problem which I agree on
>> with
>> Linus that this is problematic.
>>
>> But "if (WARN_ON(!dmabuf)) return -EINVAL;" is really bad coding
>> style
>> as well since it hides real problems which are hard errors behind
>> warnings.
> I agree that doing these kind of checks in the core object of an API is
> abusing parameter "validation". I guess a good pattern is having the
> warning and let the code flow. But since these checks are already all
> over the place I'm not sure the way to go. I'm very new to dma-buf
> and I was just checking the code and saw this was not be coherent with
> the rest of the API so I thought it would be a straight easy patch... Well,
> I could not be more wrong :)

Well that existing stuff might actually depend on this is a really good 
argument to keep it for now or at least until we have a consent on what 
to do.

> Anyways, depending on what's decided, I can send another patch trying
> to make these stuff more coherent. At this point, my feeling is that this
> patch is to be dropped...

At least for now I would hold it back.

Thanks,
Christian.

>
> - Nuno Sá
>
>> Returning -EINVAL indicates a recoverable error which is usually caused
>> by userspace giving invalid parameters and should never be abused to
>> indicate a driver coding error.
>>
>> Functions are either intended to take NULL as valid parameter, e.g. like
>> kfree(NULL). Or they are intended to work on an object which is
>> mandatory to provide.
>>
>> Christian.
>>
>>> -Daniel
>>>
>>>
>>>
>>>> Christian.
>>>>
>>>>> - Nuno Sá
>>>>>
>>>>>> Christian.
>>>>>>
>>>>>> Am 18.08.21 um 13:58 schrieb Nuno Sá:
>>>>>>> On top of warning about a NULL object, we also want to return
>> with a
>>>>>>> proper error code (as done in 'dma_buf_begin_cpu_access()').
>>>>>> Otherwise,
>>>>>>> we will get a NULL pointer dereference.
>>>>>>>
>>>>>>> Fixes: fc13020e086b ("dma-buf: add support for kernel cpu
>> access")
>>>>>>> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
>>>>>>> ---
>>>>>>>      drivers/dma-buf/dma-buf.c | 3 ++-
>>>>>>>      1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-
>> buf/dma-
>>>>>> buf.c
>>>>>>> index 63d32261b63f..8ec7876dd523 100644
>>>>>>> --- a/drivers/dma-buf/dma-buf.c
>>>>>>> +++ b/drivers/dma-buf/dma-buf.c
>>>>>>> @@ -1231,7 +1231,8 @@ int dma_buf_end_cpu_access(struct
>>>>>> dma_buf *dmabuf,
>>>>>>>      {
>>>>>>>      	int ret = 0;
>>>>>>>
>>>>>>> -	WARN_ON(!dmabuf);
>>>>>>> +	if (WARN_ON(!dmabuf))
>>>>>>> +		return -EINVAL;
>>>>>>>
>>>>>>>      	might_lock(&dmabuf->resv->lock.base);
>>>>>>>
>>>> _______________________________________________
>>>> Linaro-mm-sig mailing list
>>>> Linaro-mm-sig@lists.linaro.org
>>>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Furldefense.com%2Fv3%2F__https%3A%2F%2Fnam11.safelinks.protection.outl&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C6355660e526b4da23fa408d9624a0160%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637648892261212099%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=%2B7ORg3ZL932Gf%2FQzZdgcJTb02dm5dIL0YaAR6mtAQ2c%3D&amp;reserved=0
>> ook.com/?url=https*3A*2F*2Flists.linaro.org*2Fmailman*2Flistinfo*2
>> Flinaro-mm-
>> sig&amp;data=04*7C01*7Cchristian.koenig*40amd.com*7C19f53e2a2
>> d1843b65adc08d962463b78*7C3dd8961fe4884e608e11a82d994e183d*
>> 7C0*7C0*7C637648876076613233*7CUnknown*7CTWFpbGZsb3d8eyJ
>> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
>> *3D*7C1000&amp;sdata=0E5L4Kid5ZPeKT8Uxx7K61fBXmI4TOsz*2F5IL
>> sFpLB*2Fo*3D&amp;reserved=0__;JSUlJSUlJSUlJSUlJSUlJSUlJSUl!!A3N
>> i8CS0y2Y!qiDegx4svPUMZrvnzUoX7VKvvFpDcedH9gYbRCiWfe_N3fw4z
>> pmA54oQstzSNA$