[PATCH] dma-buf: return -EINVAL if dmabuf object is NULL

[PATCH] dma-buf: return -EINVAL if dmabuf object is NULL

[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);
 
-- 
2.32.0

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

[Christian König]

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.

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);
>   

RE: [PATCH] dma-buf: return -EINVAL if dmabuf object is NULL

[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..

I used -EINVAL to be coherent with the rest of the code.

- 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);
> >

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

[Christian König]

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.

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);
>>>

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

[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://lore.kernel.org/lkml/CA+55aFwyNTLuZgOWMTRuabWobF27ygskuxvFd-P0n-3UNT=0Og@mail.gmail.com/

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.
-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://lists.linaro.org/mailman/listinfo/linaro-mm-sig

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Christian König]

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%2Flore.kernel.org%2Flkml%2FCA%2B55aFwyNTLuZgOWMTRuabWobF27ygskuxvFd-P0n-3UNT%3D0Og%40mail.gmail.com%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C19f53e2a2d1843b65adc08d962463b78%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637648876076613233%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ajyBnjePRak3o7ObpBAuJNd08HgkANM9C%2BgzOAeHrMk%3D&amp;reserved=0
>
> 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.

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%2Flists.linaro.org%2Fmailman%2Flistinfo%2Flinaro-mm-sig&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C19f53e2a2d1843b65adc08d962463b78%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637648876076613233%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=0E5L4Kid5ZPeKT8Uxx7K61fBXmI4TOsz%2F5ILsFpLB%2Fo%3D&amp;reserved=0

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

[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://urldefense.com/v3/__https://nam11.safelinks.protection.outl
> 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 :)

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... 

- 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://urldefense.com/v3/__https://nam11.safelinks.protection.outl
> 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$

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

[Christian König]

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$