Generic DMA - BUG_ON

Generic DMA - BUG_ON

[monstr]

Hi All,

I have this patch in my repo. I just added BUG_ON for dma ops.
The reason for that is if driver not setup ops correctly than
the system do bad access to any memory space without any visible reason.
BUG_ON points to it and helps to solve where the problem is.

Thanks for your review,
Michal

Generic DMA - BUG_ON

[monstr]

Hi All,

I have this patch in my repo. I just added BUG_ON for dma ops.
The reason for that is if driver not setup ops correctly than
the system do bad access to any memory space without any visible reason.
BUG_ON points to it and helps to solve where the problem is.

Thanks for your review,
Michal

[PATCH] asm-generic: dma: Add BUG_ON for uninitialized dma_ops

[monstr]

From: Michal Simek <monstr@monstr.eu>

Check that dma_ops are initialized correctly. Without this
checking you get kernel fault and you don't know where the problem is.

Signed-off-by: Michal Simek <monstr@monstr.eu>
---
 include/asm-generic/dma-mapping-common.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/asm-generic/dma-mapping-common.h b/include/asm-generic/dma-mapping-common.h
index e694263..ca8bc25 100644
--- a/include/asm-generic/dma-mapping-common.h
+++ b/include/asm-generic/dma-mapping-common.h
@@ -15,6 +15,7 @@ static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
 	dma_addr_t addr;
 
 	kmemcheck_mark_initialized(ptr, size);
+	BUG_ON(!ops);
 	BUG_ON(!valid_dma_direction(dir));
 	addr = ops->map_page(dev, virt_to_page(ptr),
 			     (unsigned long)ptr & ~PAGE_MASK, size,
@@ -32,6 +33,7 @@ static inline void dma_unmap_single_attrs(struct device *dev, dma_addr_t addr,
 {
 	struct dma_map_ops *ops = get_dma_ops(dev);
 
+	BUG_ON(!ops);
 	BUG_ON(!valid_dma_direction(dir));
 	if (ops->unmap_page)
 		ops->unmap_page(dev, addr, size, dir, attrs);
@@ -48,6 +50,7 @@ static inline int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
 
 	for_each_sg(sg, s, nents, i)
 		kmemcheck_mark_initialized(sg_virt(s), s->length);
+	BUG_ON(!ops);
 	BUG_ON(!valid_dma_direction(dir));
 	ents = ops->map_sg(dev, sg, nents, dir, attrs);
 	debug_dma_map_sg(dev, sg, nents, ents, dir);
@@ -61,6 +64,7 @@ static inline void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg
 {
 	struct dma_map_ops *ops = get_dma_ops(dev);
 
+	BUG_ON(!ops);
 	BUG_ON(!valid_dma_direction(dir));
 	debug_dma_unmap_sg(dev, sg, nents, dir);
 	if (ops->unmap_sg)
@@ -75,6 +79,7 @@ static inline dma_addr_t dma_map_page(struct device *dev, struct page *page,
 	dma_addr_t addr;
 
 	kmemcheck_mark_initialized(page_address(page) + offset, size);
+	BUG_ON(!ops);
 	BUG_ON(!valid_dma_direction(dir));
 	addr = ops->map_page(dev, page, offset, size, dir, NULL);
 	debug_dma_map_page(dev, page, offset, size, dir, addr, false);
@@ -87,6 +92,7 @@ static inline void dma_unmap_page(struct device *dev, dma_addr_t addr,
 {
 	struct dma_map_ops *ops = get_dma_ops(dev);
 
+	BUG_ON(!ops);
 	BUG_ON(!valid_dma_direction(dir));
 	if (ops->unmap_page)
 		ops->unmap_page(dev, addr, size, dir, NULL);
@@ -125,6 +131,7 @@ static inline void dma_sync_single_range_for_cpu(struct device *dev,
 {
 	struct dma_map_ops *ops = get_dma_ops(dev);
 
+	BUG_ON(!ops);
 	BUG_ON(!valid_dma_direction(dir));
 	if (ops->sync_single_range_for_cpu) {
 		ops->sync_single_range_for_cpu(dev, addr, offset, size, dir);
@@ -142,6 +149,7 @@ static inline void dma_sync_single_range_for_device(struct device *dev,
 {
 	struct dma_map_ops *ops = get_dma_ops(dev);
 
+	BUG_ON(!ops);
 	BUG_ON(!valid_dma_direction(dir));
 	if (ops->sync_single_range_for_device) {
 		ops->sync_single_range_for_device(dev, addr, offset, size, dir);
@@ -157,6 +165,7 @@ dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
 {
 	struct dma_map_ops *ops = get_dma_ops(dev);
 
+	BUG_ON(!ops);
 	BUG_ON(!valid_dma_direction(dir));
 	if (ops->sync_sg_for_cpu)
 		ops->sync_sg_for_cpu(dev, sg, nelems, dir);
@@ -169,6 +178,7 @@ dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
 {
 	struct dma_map_ops *ops = get_dma_ops(dev);
 
+	BUG_ON(!ops);
 	BUG_ON(!valid_dma_direction(dir));
 	if (ops->sync_sg_for_device)
 		ops->sync_sg_for_device(dev, sg, nelems, dir);
-- 
1.5.5.1

Re: [PATCH] asm-generic: dma: Add BUG_ON for uninitialized dma_ops

[Alexey Dobriyan]

On Wed, Jan 20, 2010 at 12:08 PM,  <monstr@monstr.eu> wrote:
> From: Michal Simek <monstr@monstr.eu>
>
> Check that dma_ops are initialized correctly. Without this
> checking you get kernel fault and you don't know where the problem is.

Oh, yes you do. PC will be of some small value.

> +       BUG_ON(!ops);
>        BUG_ON(!valid_dma_direction(dir));
>        addr = ops->map_page(dev, virt_to_page(ptr),

Re: [PATCH] asm-generic: dma: Add BUG_ON for uninitialized dma_ops

[Geert Uytterhoeven]

On Wed, Jan 20, 2010 at 11:48, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Wed, Jan 20, 2010 at 12:08 PM,  <monstr@monstr.eu> wrote:
>> From: Michal Simek <monstr@monstr.eu>
>>
>> Check that dma_ops are initialized correctly. Without this
>> checking you get kernel fault and you don't know where the problem is.
>
> Oh, yes you do. PC will be of some small value.

And the backtrace will tell you where to look...

>> +       BUG_ON(!ops);
>>        BUG_ON(!valid_dma_direction(dir));
>>        addr = ops->map_page(dev, virt_to_page(ptr),

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] asm-generic: dma: Add BUG_ON for uninitialized dma_ops

[Joerg Roedel]

On Wed, Jan 20, 2010 at 11:08:31AM +0100, monstr@monstr.eu wrote:
> From: Michal Simek <monstr@monstr.eu>
> 
> Check that dma_ops are initialized correctly. Without this
> checking you get kernel fault and you don't know where the problem is.
> 
> Signed-off-by: Michal Simek <monstr@monstr.eu>
> ---
>  include/asm-generic/dma-mapping-common.h |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/include/asm-generic/dma-mapping-common.h b/include/asm-generic/dma-mapping-common.h
> index e694263..ca8bc25 100644
> --- a/include/asm-generic/dma-mapping-common.h
> +++ b/include/asm-generic/dma-mapping-common.h
> @@ -15,6 +15,7 @@ static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
>  	dma_addr_t addr;
>  
>  	kmemcheck_mark_initialized(ptr, size);
> +	BUG_ON(!ops);
>  	BUG_ON(!valid_dma_direction(dir));
>  	addr = ops->map_page(dev, virt_to_page(ptr),
>  			     (unsigned long)ptr & ~PAGE_MASK, size,

[...]

> @@ -169,6 +178,7 @@ dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
>  {
>  	struct dma_map_ops *ops = get_dma_ops(dev);
>  
> +	BUG_ON(!ops);
>  	BUG_ON(!valid_dma_direction(dir));
>  	if (ops->sync_sg_for_device)
>  		ops->sync_sg_for_device(dev, sg, nelems, dir);

The more logical place for all these checks would be in get_dma_ops. But
I also question the value of the check. Every dma_ops implementation
that has survived a boot test shouldn't have this bug. So I see no point
in adding extra cycles to every dma-api call.

	Joerg

Re: [PATCH] asm-generic: dma: Add BUG_ON for uninitialized dma_ops

[FUJITA Tomonori]

On Wed, 20 Jan 2010 11:08:31 +0100
monstr@monstr.eu wrote:

> From: Michal Simek <monstr@monstr.eu>
> 
> Check that dma_ops are initialized correctly. Without this
> checking you get kernel fault and you don't know where the problem is.

Hmm, dma_ops is initialized in early architecture-specific boot-up
code. We don't change it often. I don't think that we need much time
to find where problems are.

The benefit is insufficient to justify the overhead.

Re: Generic DMA - BUG_ON

[Russell King]

On Wed, Jan 20, 2010 at 11:08:30AM +0100, monstr@monstr.eu wrote:
> Hi All,
> 
> I have this patch in my repo. I just added BUG_ON for dma ops.
> The reason for that is if driver not setup ops correctly than
> the system do bad access to any memory space without any visible reason.
> BUG_ON points to it and helps to solve where the problem is.

I have a question of principle to raise here.

If you have code which does:

	if (ops->foo)
		ops->foo();

and ops is NULL, then this code will oops; you will get a full register
dump and backtrace.  You can use this information along with markup_oops.pl
to find out where the problem is.

If you add a BUG_ON() to this, the only additional information you end
up with is (possibly) a nice friendly message which tells you the file
and line number - but you add additional run-time tests to the code.

The question is: is this worth it?  Is there some problem with the
original oops that makes the problem hard to find?  Should we be using
BUG_ON() to augment normal oopses in this way?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: Generic DMA - BUG_ON

[Russell King]

On Wed, Jan 20, 2010 at 11:08:30AM +0100, monstr@monstr.eu wrote:
> Hi All,
> 
> I have this patch in my repo. I just added BUG_ON for dma ops.
> The reason for that is if driver not setup ops correctly than
> the system do bad access to any memory space without any visible reason.
> BUG_ON points to it and helps to solve where the problem is.

I have a question of principle to raise here.

If you have code which does:

	if (ops->foo)
		ops->foo();

and ops is NULL, then this code will oops; you will get a full register
dump and backtrace.  You can use this information along with markup_oops.pl
to find out where the problem is.

If you add a BUG_ON() to this, the only additional information you end
up with is (possibly) a nice friendly message which tells you the file
and line number - but you add additional run-time tests to the code.

The question is: is this worth it?  Is there some problem with the
original oops that makes the problem hard to find?  Should we be using
BUG_ON() to augment normal oopses in this way?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: Generic DMA - BUG_ON

[Joerg Roedel]

On Wed, Jan 20, 2010 at 10:53:50AM +0000, Russell King wrote:
> and ops is NULL, then this code will oops; you will get a full register
> dump and backtrace.  You can use this information along with markup_oops.pl
> to find out where the problem is.

You can't rely on the oops if the code runs in process context. The
process may have address 0 mapped which would result in a security hole.
We had two of these bugs last year.

But I don't see any point in checking for dma_ops != NULL too. Any
developer would mention such a bug long before init is started.

	Joerg

Re: Generic DMA - BUG_ON

[Joerg Roedel]

On Wed, Jan 20, 2010 at 10:53:50AM +0000, Russell King wrote:
> and ops is NULL, then this code will oops; you will get a full register
> dump and backtrace.  You can use this information along with markup_oops.pl
> to find out where the problem is.

You can't rely on the oops if the code runs in process context. The
process may have address 0 mapped which would result in a security hole.
We had two of these bugs last year.

But I don't see any point in checking for dma_ops != NULL too. Any
developer would mention such a bug long before init is started.

	Joerg

Re: Generic DMA - BUG_ON

[Michal Simek]

Joerg Roedel wrote:
> On Wed, Jan 20, 2010 at 10:53:50AM +0000, Russell King wrote:
>> and ops is NULL, then this code will oops; you will get a full register
>> dump and backtrace.  You can use this information along with markup_oops.pl
>> to find out where the problem is.
> 
> You can't rely on the oops if the code runs in process context. The
> process may have address 0 mapped which would result in a security hole.
> We had two of these bugs last year.

That's the same problem which I had some days ago and Microblaze misses 
valuable backtrace (because we don't have FP or constant frame size).

> 
> But I don't see any point in checking for dma_ops != NULL too. Any
> developer would mention such a bug long before init is started.

I agree that checking adds extra cycles to every dma-api call.

I like as wrote Russel to check if ops exists or not.

Let's look at code which is there.
dma_map_single_attrs - calls ops->map_page without any checking
The same for dma_map_sg_attrs, dma_map_page

The rest of functions is ok. If any checking will be there, I don't have 
any problem with it.

Michal




> 
> 	Joerg
> 
> 


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Re: Generic DMA - BUG_ON

[Arnd Bergmann]

On Wednesday 20 January 2010, Michal Simek wrote:
> Joerg Roedel wrote:
> > On Wed, Jan 20, 2010 at 10:53:50AM +0000, Russell King wrote:
> >> and ops is NULL, then this code will oops; you will get a full register
> >> dump and backtrace.  You can use this information along with markup_oops.pl
> >> to find out where the problem is.
> > 
> > You can't rely on the oops if the code runs in process context. The
> > process may have address 0 mapped which would result in a security hole.
> > We had two of these bugs last year.
> 
> That's the same problem which I had some days ago and Microblaze misses 
> valuable backtrace (because we don't have FP or constant frame size).

You can do what x86 does and just print anything in the stack that looks
like part of a kernel function.

> > But I don't see any point in checking for dma_ops != NULL too. Any
> > developer would mention such a bug long before init is started.
> 
> I agree that checking adds extra cycles to every dma-api call.
> 
> I like as wrote Russel to check if ops exists or not.

If you are worried about the overhead, you could just add the BUG_ON
to the map calls but not to unmap and sync, which are rather unlikely
(and incorrect) to be called before a map.

	Arnd

Re: Generic DMA - BUG_ON

[Steven J. Magnani]

On Wed, 2010-01-20 at 12:00 +0100, Joerg Roedel wrote:
> On Wed, Jan 20, 2010 at 10:53:50AM +0000, Russell King wrote:
> > and ops is NULL, then this code will oops; you will get a full register
> > dump and backtrace.  You can use this information along with markup_oops.pl
> > to find out where the problem is.
> 
> You can't rely on the oops if the code runs in process context. The
> process may have address 0 mapped which would result in a security hole.
> We had two of these bugs last year.

You also can't rely on an oops in a NOMMU environment.

------------------------------------------------------------------------
 Steven J. Magnani               "I claim this network for MARS!
 www.digidescorp.com              Earthling, return my space modulator!"

 #include <standard.disclaimer>

Re: Generic DMA - BUG_ON

[Russell King]

On Thu, Jan 21, 2010 at 09:51:37AM -0600, Steven J. Magnani wrote:
> On Wed, 2010-01-20 at 12:00 +0100, Joerg Roedel wrote:
> > On Wed, Jan 20, 2010 at 10:53:50AM +0000, Russell King wrote:
> > > and ops is NULL, then this code will oops; you will get a full register
> > > dump and backtrace.  You can use this information along with markup_oops.pl
> > > to find out where the problem is.
> > 
> > You can't rely on the oops if the code runs in process context. The
> > process may have address 0 mapped which would result in a security hole.
> > We had two of these bugs last year.
> 
> You also can't rely on an oops in a NOMMU environment.

I don't see why implementations where NULL pointer derefs should be
penalized by having additional NULL checks.

Maybe this needs to be a conditional check which can be optimized away
on architectures where NULL dereference always produces an oops.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: Generic DMA - BUG_ON

[Russell King]

On Thu, Jan 21, 2010 at 09:51:37AM -0600, Steven J. Magnani wrote:
> On Wed, 2010-01-20 at 12:00 +0100, Joerg Roedel wrote:
> > On Wed, Jan 20, 2010 at 10:53:50AM +0000, Russell King wrote:
> > > and ops is NULL, then this code will oops; you will get a full register
> > > dump and backtrace.  You can use this information along with markup_oops.pl
> > > to find out where the problem is.
> > 
> > You can't rely on the oops if the code runs in process context. The
> > process may have address 0 mapped which would result in a security hole.
> > We had two of these bugs last year.
> 
> You also can't rely on an oops in a NOMMU environment.

I don't see why implementations where NULL pointer derefs should be
penalized by having additional NULL checks.

Maybe this needs to be a conditional check which can be optimized away
on architectures where NULL dereference always produces an oops.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of: