Skip to content

Commit

Permalink
dma-debug: Fix the overlap() function to be correct and readable
Browse files Browse the repository at this point in the history
Linus noticed how unclean and buggy the overlap() function is:

 - It uses convoluted (and bug-causing) positive checks for
   range overlap - instead of using a more natural negative
   check.

 - Even the positive checks are buggy: a positive intersection
   check has four natural cases while we checked only for three,
   missing the (addr < start && addr2 == end) case for example.

 - The variables are mis-named, making it non-obvious how the
   check was done.

 - It needlessly uses u64 instead of unsigned long. Since these
   are kernel memory pointers and we explicitly exclude highmem
   ranges anyway we cannot ever overflow 32 bits, even if we
   could. (and on 64-bit it doesnt matter anyway)

All in one, this function needs a total revamp. I used Linus's
suggestions minus the paranoid checks (we cannot overflow really
because if we get totally bad DMA ranges passed far more things
break in the systems than just DMA debugging). I also fixed a
few other small details i noticed.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Joerg Roedel <joerg.roedel@amd.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
  • Loading branch information
Ingo Molnar committed Jul 10, 2009
1 parent 2b8777c commit f39d1b9
Showing 1 changed file with 12 additions and 12 deletions.
24 changes: 12 additions & 12 deletions lib/dma-debug.c
Original file line number Diff line number Diff line change
Expand Up @@ -856,22 +856,21 @@ static void check_for_stack(struct device *dev, void *addr)
"stack [addr=%p]\n", addr);
}

static inline bool overlap(void *addr, u64 size, void *start, void *end)
static inline bool overlap(void *addr, unsigned long len, void *start, void *end)
{
void *addr2 = (char *)addr + size;
unsigned long a1 = (unsigned long)addr;
unsigned long b1 = a1 + len;
unsigned long a2 = (unsigned long)start;
unsigned long b2 = (unsigned long)end;

return ((addr >= start && addr < end) ||
(addr2 >= start && addr2 < end) ||
((addr < start) && (addr2 > end)));
return !(b1 <= a2 || a1 >= b2);
}

static void check_for_illegal_area(struct device *dev, void *addr, u64 size)
static void check_for_illegal_area(struct device *dev, void *addr, unsigned long len)
{
if (overlap(addr, size, _text, _etext) ||
overlap(addr, size, __start_rodata, __end_rodata))
err_printk(dev, NULL, "DMA-API: device driver maps "
"memory from kernel text or rodata "
"[addr=%p] [size=%llu]\n", addr, size);
if (overlap(addr, len, _text, _etext) ||
overlap(addr, len, __start_rodata, __end_rodata))
err_printk(dev, NULL, "DMA-API: device driver maps memory from kernel text or rodata [addr=%p] [len=%lu]\n", addr, len);
}

static void check_sync(struct device *dev,
Expand Down Expand Up @@ -969,7 +968,8 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
entry->type = dma_debug_single;

if (!PageHighMem(page)) {
void *addr = ((char *)page_address(page)) + offset;
void *addr = page_address(page) + offset;

check_for_stack(dev, addr);
check_for_illegal_area(dev, addr, size);
}
Expand Down

0 comments on commit f39d1b9

Please sign in to comment.