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
| ||
|
Date: Wed, 13 Oct 2021 00:37:14 +0200 From: Halil Pasic <pasic@...ux.ibm.com> To: Cornelia Huck <cohuck@...hat.com> Cc: Pierre Morel <pmorel@...ux.ibm.com>, Vineeth Vijayan <vneethv@...ux.ibm.com>, Peter Oberparleiter <oberpar@...ux.ibm.com>, Heiko Carstens <hca@...ux.ibm.com>, Vasily Gorbik <gor@...ux.ibm.com>, Christian Borntraeger <borntraeger@...ibm.com>, Michael Mueller <mimu@...ux.ibm.com>, linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org, stable@...r.kernel.org, bfu@...hat.com, Halil Pasic <pasic@...ux.ibm.com> Subject: Re: [RFC PATCH 1/1] s390/cio: make ccw_device_dma_* more robust On Tue, 12 Oct 2021 15:50:48 +0200 Cornelia Huck <cohuck@...hat.com> wrote: > >> If I read cio_gp_dma_zalloc() correctly, we either get NULL or a valid > >> address, so yes. > >> > > > > I don't think the extra care will hurt us too badly. I prefer to keep > > the IS_ERR_OR_NULL() check because it needs less domain specific > > knowledge to be understood, and because it is more robust. > > It feels weird, though -- I'd rather have a comment that tells me This way the change feels simpler and safer to me. I believe I explained the why above. But if you insist I can change it. I double checked the cio_gp_dma_zalloc() code, and more or less the code called by it. So now I don't feel uncomfortable with the simpler check. On the other hand, I'm not very happy doing changes solely based on somebody's feelings. It would feel much more comfortable with a reason based discussion. One reason to change this to a simple NULL check, is that the IS_ERR_OR_NULL() check could upset the reader of the client code, which only checks for NULL. On the other hand I do believe we have some risk of lumping together different errors here. E.g. dma_pool is NULL or dma ops are not set up properly. Currently we would communicate that kind of a problem as -ENOMEM, which wouldn't be a great match. But since dma_alloc_coherent() returns either NULL or a valid pointer, and furthermore this looks like a common thing in all the mm-api, I decided to be inline with that. TLDR; If you insist, I will change this to a simple null pointer check. > exactly what cio_gp_dma_zalloc() is supposed to return; I would have > expected that a _zalloc function always gives me a valid pointer or > NULL. I don't think we have such a comment for dma_alloc_coherent() or even kmalloc(). I agree, it would be nice to have this behavior documented in the apidoc all over the place. But IMHO that is a different issue. Regards, Halil
Powered by blists - more mailing lists