[PATCH] dma-mapping: remove an unnecessary NULL check

[PATCH] dma-mapping: remove an unnecessary NULL check

[Dan Carpenter]

We already dereferenced "dev" when we called get_dma_ops() so this NULL
check is too late.  We're not supposed to pass NULL "dev" pointers to
dma_alloc_attrs().

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
There are still at least two drivers which do pass a NULL unfortunately.

drivers/staging/comedi/drivers/comedi_isadma.c:195 comedi_isadma_alloc() error: NULL dereference inside function 'dma_alloc_coherent()'
drivers/staging/comedi/drivers/comedi_isadma.c:227 comedi_isadma_free() error: NULL dereference inside function 'dma_free_coherent()'
drivers/tty/synclink.c:3667 mgsl_alloc_buffer_list_memory() error: NULL dereference inside function 'dma_alloc_coherent()'
drivers/tty/synclink.c:3738 mgsl_free_buffer_list_memory() error: NULL dereference inside function 'dma_free_coherent()'
drivers/tty/synclink.c:3777 mgsl_alloc_frame_memory() error: NULL dereference inside function 'dma_alloc_coherent()'
drivers/tty/synclink.c:3811 mgsl_free_frame_memory() error: NULL dereference inside function 'dma_free_coherent()'

If I remember right there are one or two others which sometimes pass a
NULL, but it's harder to grep for those warnings.

 kernel/dma/mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 685a53f2a793..f7afdadb6770 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -244,7 +244,7 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
 	const struct dma_map_ops *ops = get_dma_ops(dev);
 	void *cpu_addr;
 
-	WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
+	WARN_ON_ONCE(!dev->coherent_dma_mask);
 
 	if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
 		return cpu_addr;
-- 
2.18.0

Re: [PATCH] dma-mapping: remove an unnecessary NULL check

[Christoph Hellwig]

On Wed, Apr 24, 2019 at 05:24:37PM +0300, Dan Carpenter wrote:
> We already dereferenced "dev" when we called get_dma_ops() so this NULL
> check is too late.  We're not supposed to pass NULL "dev" pointers to
> dma_alloc_attrs().

Thanks, applied to the dma-mapping for-next tree.

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> There are still at least two drivers which do pass a NULL unfortunately.
> 
> drivers/staging/comedi/drivers/comedi_isadma.c:195 comedi_isadma_alloc() error: NULL dereference inside function 'dma_alloc_coherent()'
> drivers/staging/comedi/drivers/comedi_isadma.c:227 comedi_isadma_free() error: NULL dereference inside function 'dma_free_coherent()'

This is staging code.  Per official decree from Linus we can just
ignore it, and I tend to do so to keep my sanity.

> drivers/tty/synclink.c:3667 mgsl_alloc_buffer_list_memory() error: NULL dereference inside function 'dma_alloc_coherent()'
> drivers/tty/synclink.c:3738 mgsl_free_buffer_list_memory() error: NULL dereference inside function 'dma_free_coherent()'
> drivers/tty/synclink.c:3777 mgsl_alloc_frame_memory() error: NULL dereference inside function 'dma_alloc_coherent()'
> drivers/tty/synclink.c:3811 mgsl_free_frame_memory() error: NULL dereference inside function 'dma_free_coherent()'

The !PCI case there is dead since I removed PCI support a while ago.
Looks like it is still too convoluted for static checkers to notice that,
though.

Re: [PATCH] dma-mapping: remove an unnecessary NULL check

[Ian Abbott]

On 24/04/2019 15:27, Christoph Hellwig wrote:
> On Wed, Apr 24, 2019 at 05:24:37PM +0300, Dan Carpenter wrote:
>> We already dereferenced "dev" when we called get_dma_ops() so this NULL
>> check is too late.  We're not supposed to pass NULL "dev" pointers to
>> dma_alloc_attrs().
> 
> Thanks, applied to the dma-mapping for-next tree.
> 
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> ---
>> There are still at least two drivers which do pass a NULL unfortunately.
>>
>> drivers/staging/comedi/drivers/comedi_isadma.c:195 comedi_isadma_alloc() error: NULL dereference inside function 'dma_alloc_coherent()'
>> drivers/staging/comedi/drivers/comedi_isadma.c:227 comedi_isadma_free() error: NULL dereference inside function 'dma_free_coherent()'
> 
> This is staging code.  Per official decree from Linus we can just
> ignore it, and I tend to do so to keep my sanity.

So for comedi_isadma, we can just replace the NULL with a pointer to a 
static dummy device with a 24-bit coherent mask?

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:    )=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

Re: [PATCH] dma-mapping: remove an unnecessary NULL check

[Christoph Hellwig]

On Thu, Apr 25, 2019 at 03:13:49PM +0100, Ian Abbott wrote:
> So for comedi_isadma, we can just replace the NULL with a pointer to a 
> static dummy device with a 24-bit coherent mask?

It should be converted to a proper isa_driver, so that it gets a real
struct device.

Re: [PATCH] dma-mapping: remove an unnecessary NULL check

[Ian Abbott]

On 25/04/2019 15:18, Christoph Hellwig wrote:
> On Thu, Apr 25, 2019 at 03:13:49PM +0100, Ian Abbott wrote:
>> So for comedi_isadma, we can just replace the NULL with a pointer to a
>> static dummy device with a 24-bit coherent mask?
> 
> It should be converted to a proper isa_driver, so that it gets a real
> struct device.

But it will work as a short term solution?  comedi_isadma isn't a 
driver, but is used by a few comedi drivers for ISA cards.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:    )=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

Re: [PATCH] dma-mapping: remove an unnecessary NULL check

[Christoph Hellwig]

On Thu, Apr 25, 2019 at 03:31:01PM +0100, Ian Abbott wrote:
> On 25/04/2019 15:18, Christoph Hellwig wrote:
>> On Thu, Apr 25, 2019 at 03:13:49PM +0100, Ian Abbott wrote:
>>> So for comedi_isadma, we can just replace the NULL with a pointer to a
>>> static dummy device with a 24-bit coherent mask?
>>
>> It should be converted to a proper isa_driver, so that it gets a real
>> struct device.
>
> But it will work as a short term solution?  comedi_isadma isn't a driver, 
> but is used by a few comedi drivers for ISA cards.

Then convert the drivers properly and pass on the device from them.