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
From: Michal Simek <[email protected]>
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 <[email protected]>
---
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
On Wed, Jan 20, 2010 at 12:08 PM, <[email protected]> wrote:
> From: Michal Simek <[email protected]>
>
> 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),
On Wed, Jan 20, 2010 at 11:08:30AM +0100, [email protected] 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:
On Wed, Jan 20, 2010 at 11:08:31AM +0100, [email protected] wrote:
> From: Michal Simek <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On Wed, Jan 20, 2010 at 11:48, Alexey Dobriyan <[email protected]> wrote:
> On Wed, Jan 20, 2010 at 12:08 PM, <[email protected]> wrote:
>> From: Michal Simek <[email protected]>
>>
>> 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 -- [email protected]
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
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
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: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
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
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!
http://www.digidescorp.com Earthling, return my space modulator!"
#include <standard.disclaimer>
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:
On Wed, 20 Jan 2010 11:08:31 +0100
[email protected] wrote:
> From: Michal Simek <[email protected]>
>
> 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.