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... ook.com/?url=https*3A*2F*2Flore.kernel.org*2Flkml*2FCA*2B55aFw yNTLuZgOWMTRuabWobF27ygskuxvFd-P0n- 3UNT*3D0Og*40mail.gmail.com*2F&data=04*7C01*7Cchristian.k oenig*40amd.com*7C19f53e2a2d1843b65adc08d962463b78*7C3dd896 1fe4884e608e11a82d994e183d*7C0*7C0*7C637648876076613233*7CU nknown*7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL CJBTiI6Ik1haWwiLCJXVCI6Mn0*3D*7C1000&sdata=ajyBnjePRak3 o7ObpBAuJNd08HgkANM9C*2BgzOAeHrMk*3D&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 useAnyone 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... ook.com/?url=https*3A*2F*2Flists.linaro.org*2Fmailman*2Flistinfo*2 Flinaro-mm- sig&data=04*7C01*7Cchristian.koenig*40amd.com*7C19f53e2a2 d1843b65adc08d962463b78*7C3dd8961fe4884e608e11a82d994e183d* 7C0*7C0*7C637648876076613233*7CUnknown*7CTWFpbGZsb3d8eyJ WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 *3D*7C1000&sdata=0E5L4Kid5ZPeKT8Uxx7K61fBXmI4TOsz*2F5IL sFpLB*2Fo*3D&reserved=0__;JSUlJSUlJSUlJSUlJSUlJSUlJSUl!!A3N i8CS0y2Y!qiDegx4svPUMZrvnzUoX7VKvvFpDcedH9gYbRCiWfe_N3fw4z pmA54oQstzSNA$
linaro-mm-sig@lists.linaro.org
-
Christian König