Linus,
Please pull the latest core-fixes-for-linus git tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git core-fixes-for-linus
Thanks,
Ingo
------------------>
Arnd Bergmann (1):
signals: declare sys_rt_tgsigqueueinfo in syscalls.h
Ingo Molnar (1):
dma-debug: Put all hash-chain locks into the same lock class
Joerg Roedel (1):
dma-debug: fix off-by-one error in overlap function
Maynard Johnson (1):
oprofile: reset bt_lost_no_mapping with other stats
Paul E. McKenney (1):
rcu: Mark Hierarchical RCU no longer experimental
Robert Richter (1):
x86/oprofile: rename kernel parameter for architectural perfmon to arch_perfmon
Documentation/kernel-parameters.txt | 4 ++--
arch/x86/oprofile/nmi_int.c | 2 +-
drivers/oprofile/oprofile_stats.c | 1 +
include/linux/syscalls.h | 2 ++
kernel/rcutree.c | 3 +--
lib/dma-debug.c | 4 ++--
6 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 92e1ab8..c59e965 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1728,8 +1728,8 @@ and is between 256 and 4096 characters. It is defined in the file
oprofile.cpu_type= Force an oprofile cpu type
This might be useful if you have an older oprofile
userland or if you want common events.
- Format: { archperfmon }
- archperfmon: [X86] Force use of architectural
+ Format: { arch_perfmon }
+ arch_perfmon: [X86] Force use of architectural
perfmon on Intel CPUs instead of the
CPU specific event set.
diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index b07dd8d..89b9a5c 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -390,7 +390,7 @@ static int __init p4_init(char **cpu_type)
static int force_arch_perfmon;
static int force_cpu_type(const char *str, struct kernel_param *kp)
{
- if (!strcmp(str, "archperfmon")) {
+ if (!strcmp(str, "arch_perfmon")) {
force_arch_perfmon = 1;
printk(KERN_INFO "oprofile: forcing architectural perfmon\n");
}
diff --git a/drivers/oprofile/oprofile_stats.c b/drivers/oprofile/oprofile_stats.c
index e1f6ce0..3c2270a 100644
--- a/drivers/oprofile/oprofile_stats.c
+++ b/drivers/oprofile/oprofile_stats.c
@@ -33,6 +33,7 @@ void oprofile_reset_stats(void)
atomic_set(&oprofile_stats.sample_lost_no_mm, 0);
atomic_set(&oprofile_stats.sample_lost_no_mapping, 0);
atomic_set(&oprofile_stats.event_lost_overflow, 0);
+ atomic_set(&oprofile_stats.bt_lost_no_mapping, 0);
}
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index fa4242c..80de700 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -321,6 +321,8 @@ asmlinkage long sys_rt_sigtimedwait(const sigset_t __user *uthese,
siginfo_t __user *uinfo,
const struct timespec __user *uts,
size_t sigsetsize);
+asmlinkage long sys_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
+ siginfo_t __user *uinfo);
asmlinkage long sys_kill(int pid, int sig);
asmlinkage long sys_tgkill(int tgid, int pid, int sig);
asmlinkage long sys_tkill(int pid, int sig);
diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index 0dccfbb..7717b95 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -1533,7 +1533,7 @@ void __init __rcu_init(void)
int j;
struct rcu_node *rnp;
- printk(KERN_WARNING "Experimental hierarchical RCU implementation.\n");
+ printk(KERN_INFO "Hierarchical RCU implementation.\n");
#ifdef CONFIG_RCU_CPU_STALL_DETECTOR
printk(KERN_INFO "RCU-based detection of stalled CPUs is enabled.\n");
#endif /* #ifdef CONFIG_RCU_CPU_STALL_DETECTOR */
@@ -1546,7 +1546,6 @@ void __init __rcu_init(void)
rcu_cpu_notify(&rcu_nb, CPU_UP_PREPARE, (void *)(long)i);
/* Register notifier for non-boot CPUs */
register_cpu_notifier(&rcu_nb);
- printk(KERN_WARNING "Experimental hierarchical RCU init done.\n");
}
module_param(blimit, int, 0);
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 3b93129..c9187fe 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -716,7 +716,7 @@ void dma_debug_init(u32 num_entries)
for (i = 0; i < HASH_SIZE; ++i) {
INIT_LIST_HEAD(&dma_entry_hash[i].list);
- dma_entry_hash[i].lock = SPIN_LOCK_UNLOCKED;
+ spin_lock_init(&dma_entry_hash[i].lock);
}
if (dma_debug_fs_init() != 0) {
@@ -862,7 +862,7 @@ static inline bool overlap(void *addr, u64 size, void *start, void *end)
return ((addr >= start && addr < end) ||
(addr2 >= start && addr2 < end) ||
- ((addr < start) && (addr2 >= end)));
+ ((addr < start) && (addr2 > end)));
}
static void check_for_illegal_area(struct device *dev, void *addr, u64 size)
On Fri, 10 Jul 2009, Ingo Molnar wrote:
>
> Joerg Roedel (1):
> dma-debug: fix off-by-one error in overlap function
>
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index 3b93129..c9187fe 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -862,7 +862,7 @@ static inline bool overlap(void *addr, u64 size, void *start, void *end)
>
> return ((addr >= start && addr < end) ||
> (addr2 >= start && addr2 < end) ||
> - ((addr < start) && (addr2 >= end)));
> + ((addr < start) && (addr2 > end)));
> }
>
> static void check_for_illegal_area(struct device *dev, void *addr, u64 size)
The above seems like total shit.
If (addr < start && addr2 == end) then the two areas very much overlap.
What am I missing (apart from the fact that all those variables are
horribly badly named)?
Also, the tests make no sense. That's not how you are supposed to check
for overlap to begin with.
Isn't it easier to test for _not_ overlapping?
/* range1 is fully before range2 */
(end1 <= start2 ||
/* range1 is fully after range2 */
start1 >= end2)
possibly together with checking for overflow in the size addition? But I
didn't think that through, so maybe I'm doing something stupid.
Finally, why is 'size' a u64? It will overflow anyway if it's bigger than
a pointer, so it should be just 'unsigned long'. Or it should all be done
in u64 if people care. Or we should care about overflow (which cannot be
done with pointers).
Also, comparing pointers is unsafe to begin with. It's not clear if they
are signed or unsigned comparisons, and gcc has historically had bugs here
(only unsigned comparisons make sense for pointers, but _technically_ a
crazy compiler person could argue that at least in some environments any
valid pointers to the same object - which is the only thing C defines -
must not cross the sign barrier, so they use a buggy signed compare).
IOW, I think this whole function is just total crap, apparently put
together by randomly assembling characters until it compiles. Somebody
should put more effort into looking at it, but I think it should be
something like
static inline int overlap(void *addr, unsigned long len, void *start, void *end)
{
unsigned long a1 = (unsigned long) addr;
unsigned long b1 = a1 + len;
unsigned long a2 = (unsigned long) start;
unsigned long b2 = (unsigned long) end;
#ifdef WE_CARE_DEEPLY
/* Overflow? */
if (b1 < a1)
return 1;
#ifdef AND_ARE_ANAL
if (b2 < a2)
return 1;
#endif
#endif
return !(b1 <= a2 || a1 >= b2);
}
but I really migth have done soemthing wrong there. It's a simple
function, but somebody needs to double-check that I haven't made it worse.
Linus
* Linus Torvalds <[email protected]> wrote:
>
> On Fri, 10 Jul 2009, Ingo Molnar wrote:
> >
> > Joerg Roedel (1):
> > dma-debug: fix off-by-one error in overlap function
> >
> > diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> > index 3b93129..c9187fe 100644
> > --- a/lib/dma-debug.c
> > +++ b/lib/dma-debug.c
> > @@ -862,7 +862,7 @@ static inline bool overlap(void *addr, u64 size, void *start, void *end)
> >
> > return ((addr >= start && addr < end) ||
> > (addr2 >= start && addr2 < end) ||
> > - ((addr < start) && (addr2 >= end)));
> > + ((addr < start) && (addr2 > end)));
> > }
> >
> > static void check_for_illegal_area(struct device *dev, void *addr, u64 size)
>
> The above seems like total shit.
>
> If (addr < start && addr2 == end) then the two areas very much overlap.
>
> What am I missing (apart from the fact that all those variables are
> horribly badly named)?
>
> Also, the tests make no sense. That's not how you are supposed to check
> for overlap to begin with.
>
> Isn't it easier to test for _not_ overlapping?
>
> /* range1 is fully before range2 */
> (end1 <= start2 ||
> /* range1 is fully after range2 */
> start1 >= end2)
>
> possibly together with checking for overflow in the size addition?
> But I didn't think that through, so maybe I'm doing something
> stupid.
>
> Finally, why is 'size' a u64? It will overflow anyway if it's
> bigger than a pointer, so it should be just 'unsigned long'. Or it
> should all be done in u64 if people care. Or we should care about
> overflow (which cannot be done with pointers).
>
> Also, comparing pointers is unsafe to begin with. It's not clear
> if they are signed or unsigned comparisons, and gcc has
> historically had bugs here (only unsigned comparisons make sense
> for pointers, but _technically_ a crazy compiler person could
> argue that at least in some environments any valid pointers to the
> same object - which is the only thing C defines - must not cross
> the sign barrier, so they use a buggy signed compare).
hm, indeed - and i missed that.
[ Even in the pointer space i think this cast is slightly confused
too:
static inline bool overlap(void *addr, u64 size, void *start, void *end)
{
void *addr2 = (char *)addr + size;
as void * has byte granular arithmetics already so 'addr + size'
would suffice. ]
> IOW, I think this whole function is just total crap, apparently
> put together by randomly assembling characters until it compiles.
> Somebody should put more effort into looking at it, but I think it
> should be something like
>
> static inline int overlap(void *addr, unsigned long len, void *start, void *end)
> {
> unsigned long a1 = (unsigned long) addr;
> unsigned long b1 = a1 + len;
> unsigned long a2 = (unsigned long) start;
> unsigned long b2 = (unsigned long) end;
At least some arguments have unsigned long natural types (they come
out of page_address() for example) so the function parameters could
perhaps be changed to unsigned long too as well.
> #ifdef WE_CARE_DEEPLY
> /* Overflow? */
> if (b1 < a1)
> return 1;
> #ifdef AND_ARE_ANAL
> if (b2 < a2)
> return 1;
> #endif
> #endif
> return !(b1 <= a2 || a1 >= b2);
> }
>
> but I really migth have done soemthing wrong there. It's a simple
> function, but somebody needs to double-check that I haven't made
> it worse.
Looks correct to me.
Ingo
* Ingo Molnar <[email protected]> wrote:
> > IOW, I think this whole function is just total crap, apparently
> > put together by randomly assembling characters until it
> > compiles. Somebody should put more effort into looking at it,
> > but I think it should be something like
> >
> > static inline int overlap(void *addr, unsigned long len, void *start, void *end)
> > {
> > unsigned long a1 = (unsigned long) addr;
> > unsigned long b1 = a1 + len;
> > unsigned long a2 = (unsigned long) start;
> > unsigned long b2 = (unsigned long) end;
>
> At least some arguments have unsigned long natural types (they come
> out of page_address() for example) so the function parameters could
> perhaps be changed to unsigned long too as well.
>
> > #ifdef WE_CARE_DEEPLY
> > /* Overflow? */
> > if (b1 < a1)
> > return 1;
> > #ifdef AND_ARE_ANAL
> > if (b2 < a2)
> > return 1;
> > #endif
> > #endif
> > return !(b1 <= a2 || a1 >= b2);
> > }
> >
> > but I really migth have done soemthing wrong there. It's a
> > simple function, but somebody needs to double-check that I
> > haven't made it worse.
>
> Looks correct to me.
How about the patch below? Lightly tested.
Ingo
------------>
>From 35c89da82e969a2fd157478940e7ecde1e19ccc4 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Fri, 10 Jul 2009 21:38:02 +0200
Subject: [PATCH] dma-debug: Fix the overlap() function to be correct and readable
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 <[email protected]>
Cc: Joerg Roedel <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
lib/dma-debug.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index c9187fe..02fed52 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -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,
@@ -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 = (void *)page_address(page) + offset;
+
check_for_stack(dev, addr);
check_for_illegal_area(dev, addr, size);
}
On Fri, 10 Jul 2009, Ingo Molnar wrote:
> >
> > but I really migth have done soemthing wrong there. It's a simple
> > function, but somebody needs to double-check that I haven't made
> > it worse.
>
> Looks correct to me.
Note, I didn't look at how 'end' works, and it really does matter if 'end'
is an "inclusive" or "exclusive" end pointer address. So my replacement
overlap() function was written more as a conceptual patch - I did not
check the exact semantics of the arguments passed in.
If 'end' is exclusive, then 'b1' should be calculated as 'a1+size-1',
because the ranges must have the same rules. And then you should use the
'strict inequality' operators for testing the ranges.
Linus
* Linus Torvalds <[email protected]> wrote:
> On Fri, 10 Jul 2009, Ingo Molnar wrote:
> > >
> > > but I really migth have done soemthing wrong there. It's a
> > > simple function, but somebody needs to double-check that I
> > > haven't made it worse.
> >
> > Looks correct to me.
>
> Note, I didn't look at how 'end' works, and it really does matter
> if 'end' is an "inclusive" or "exclusive" end pointer address. So
> my replacement overlap() function was written more as a conceptual
> patch - I did not check the exact semantics of the arguments
> passed in.
>
> If 'end' is exclusive, then 'b1' should be calculated as
> 'a1+size-1', because the ranges must have the same rules. And then
> you should use the 'strict inequality' operators for testing the
> ranges.
The ranges are inclusive in terms of non-overlap: we can have
adjacent ranges with b1==a2 or b2==a1 that are still considered
non-overlapping. Hence the sharp test you used (which is negated)
looks correct to me.
The end-of-range symbols we use:
if (overlap(addr, len, _text, _etext) ||
overlap(addr, len, __start_rodata, __end_rodata))
Are all at the first byte outside of the to-be-avoided range:
.text : {
_text = .; /* Text */
*(.text)
*(.text.*)
_etext = . ;
}
...
__param : AT(ADDR(__param) - LOAD_OFFSET) { \
VMLINUX_SYMBOL(__start___param) = .; \
*(__param) \
VMLINUX_SYMBOL(__stop___param) = .; \
. = ALIGN((align)); \
VMLINUX_SYMBOL(__end_rodata) = .; \
} \
...
I think ...
Ingo
On Fri, 10 Jul 2009, Ingo Molnar wrote:
>
> How about the patch below? Lightly tested.
>
> if (!PageHighMem(page)) {
> - void *addr = ((char *)page_address(page)) + offset;
> + void *addr = (void *)page_address(page) + offset;
> +
Why is that 'void *' cast there? page_address() is already a void *.
Other than that it obviously looks good to me. But I never see my own
bugs.
Linus
* Linus Torvalds <[email protected]> wrote:
>
>
> On Fri, 10 Jul 2009, Ingo Molnar wrote:
> >
> > How about the patch below? Lightly tested.
> >
> > if (!PageHighMem(page)) {
> > - void *addr = ((char *)page_address(page)) + offset;
> > + void *addr = (void *)page_address(page) + offset;
> > +
>
> Why is that 'void *' cast there? page_address() is already a void *.
>
> Other than that it obviously looks good to me. But I never see my
> own bugs.
hm, indeed. I distinctly remember page_address() having been
unsigned long ten years ago.
And yes, i still have a "highmem-2.2.21-A0" patch proving it:
--- linux/include/linux/pagemap.h.orig Thu Oct 7 14:54:24 1999
+++ linux/include/linux/pagemap.h Wed Oct 13 02:44:03 1999
@@ -11,12 +11,24 @@
#include <linux/mm.h>
#include <linux/fs.h>
+#include <linux/list.h>
-static inline unsigned long page_address(struct page * page)
+extern inline unsigned long FIXME_page_address(struct page * page)
{
+ if (PageHIGHMEM(page))
+ BUG();
return PAGE_OFFSET + ((page - mem_map) << PAGE_SHIFT);
Which, beyond being a rather embarrasing hunk (whose author i wont
name voluntarily - i'll rather take the 5th), also shows that
page_address() started out as an unsigned long.
Which got cleaned up for good once we added page->address, instead
of the above direct calculation.
( Note to self: consider checking the types of core MM facilities
somewhat more frequently than every 10 years. )
Ingo
Linus,
Please pull the v2 core-fixes-for-linus git tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git core-fixes-for-linus-2
This has the overlap() bug(s) fixed. [ You might want to hold off on
pulling this for a few hours, just in case the fresh dma-debug.c fix
has any unexpected (and unprecedented) breakages. ]
Thanks,
Ingo
------------------>
Arnd Bergmann (1):
signals: declare sys_rt_tgsigqueueinfo in syscalls.h
Ingo Molnar (2):
dma-debug: Put all hash-chain locks into the same lock class
dma-debug: Fix the overlap() function to be correct and readable
Joerg Roedel (1):
dma-debug: fix off-by-one error in overlap function
Maynard Johnson (1):
oprofile: reset bt_lost_no_mapping with other stats
Paul E. McKenney (1):
rcu: Mark Hierarchical RCU no longer experimental
Robert Richter (1):
x86/oprofile: rename kernel parameter for architectural perfmon to arch_perfmon
Documentation/kernel-parameters.txt | 4 ++--
arch/x86/oprofile/nmi_int.c | 2 +-
drivers/oprofile/oprofile_stats.c | 1 +
include/linux/syscalls.h | 2 ++
kernel/rcutree.c | 3 +--
lib/dma-debug.c | 26 +++++++++++++-------------
6 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 92e1ab8..c59e965 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1728,8 +1728,8 @@ and is between 256 and 4096 characters. It is defined in the file
oprofile.cpu_type= Force an oprofile cpu type
This might be useful if you have an older oprofile
userland or if you want common events.
- Format: { archperfmon }
- archperfmon: [X86] Force use of architectural
+ Format: { arch_perfmon }
+ arch_perfmon: [X86] Force use of architectural
perfmon on Intel CPUs instead of the
CPU specific event set.
diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index b07dd8d..89b9a5c 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -390,7 +390,7 @@ static int __init p4_init(char **cpu_type)
static int force_arch_perfmon;
static int force_cpu_type(const char *str, struct kernel_param *kp)
{
- if (!strcmp(str, "archperfmon")) {
+ if (!strcmp(str, "arch_perfmon")) {
force_arch_perfmon = 1;
printk(KERN_INFO "oprofile: forcing architectural perfmon\n");
}
diff --git a/drivers/oprofile/oprofile_stats.c b/drivers/oprofile/oprofile_stats.c
index e1f6ce0..3c2270a 100644
--- a/drivers/oprofile/oprofile_stats.c
+++ b/drivers/oprofile/oprofile_stats.c
@@ -33,6 +33,7 @@ void oprofile_reset_stats(void)
atomic_set(&oprofile_stats.sample_lost_no_mm, 0);
atomic_set(&oprofile_stats.sample_lost_no_mapping, 0);
atomic_set(&oprofile_stats.event_lost_overflow, 0);
+ atomic_set(&oprofile_stats.bt_lost_no_mapping, 0);
}
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index fa4242c..80de700 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -321,6 +321,8 @@ asmlinkage long sys_rt_sigtimedwait(const sigset_t __user *uthese,
siginfo_t __user *uinfo,
const struct timespec __user *uts,
size_t sigsetsize);
+asmlinkage long sys_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
+ siginfo_t __user *uinfo);
asmlinkage long sys_kill(int pid, int sig);
asmlinkage long sys_tgkill(int tgid, int pid, int sig);
asmlinkage long sys_tkill(int pid, int sig);
diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index 0dccfbb..7717b95 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -1533,7 +1533,7 @@ void __init __rcu_init(void)
int j;
struct rcu_node *rnp;
- printk(KERN_WARNING "Experimental hierarchical RCU implementation.\n");
+ printk(KERN_INFO "Hierarchical RCU implementation.\n");
#ifdef CONFIG_RCU_CPU_STALL_DETECTOR
printk(KERN_INFO "RCU-based detection of stalled CPUs is enabled.\n");
#endif /* #ifdef CONFIG_RCU_CPU_STALL_DETECTOR */
@@ -1546,7 +1546,6 @@ void __init __rcu_init(void)
rcu_cpu_notify(&rcu_nb, CPU_UP_PREPARE, (void *)(long)i);
/* Register notifier for non-boot CPUs */
register_cpu_notifier(&rcu_nb);
- printk(KERN_WARNING "Experimental hierarchical RCU init done.\n");
}
module_param(blimit, int, 0);
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 3b93129..65b0d99 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -716,7 +716,7 @@ void dma_debug_init(u32 num_entries)
for (i = 0; i < HASH_SIZE; ++i) {
INIT_LIST_HEAD(&dma_entry_hash[i].list);
- dma_entry_hash[i].lock = SPIN_LOCK_UNLOCKED;
+ spin_lock_init(&dma_entry_hash[i].lock);
}
if (dma_debug_fs_init() != 0) {
@@ -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,
@@ -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);
}
On Fri, Jul 10, 2009 at 12:06:23PM -0700, Linus Torvalds wrote:
> What am I missing (apart from the fact that all those variables are
> horribly badly named)?
>
> Also, the tests make no sense. That's not how you are supposed to check
> for overlap to begin with.
The tests made sense in my brain when I wrote that function ;-) My code
checked for possible overlap scenarios. I havn't thought about doing it
much much simpler...
>
> Isn't it easier to test for _not_ overlapping?
>
> /* range1 is fully before range2 */
> (end1 <= start2 ||
> /* range1 is fully after range2 */
> start1 >= end2)
... by checking for non-overlap and negating the result. But now I know
better :-)
Joerg
On Fri, 2009-07-10 at 21:51 +0200, Ingo Molnar wrote:
> * Ingo Molnar <[email protected]> wrote:
>
> > > IOW, I think this whole function is just total crap, apparently
> > > put together by randomly assembling characters until it
> > > compiles. Somebody should put more effort into looking at it,
> > > but I think it should be something like
> > >
> > > static inline int overlap(void *addr, unsigned long len, void *start, void *end)
> > > {
> > > unsigned long a1 = (unsigned long) addr;
> > > unsigned long b1 = a1 + len;
> > > unsigned long a2 = (unsigned long) start;
> > > unsigned long b2 = (unsigned long) end;
> >
> > At least some arguments have unsigned long natural types (they come
> > out of page_address() for example) so the function parameters could
> > perhaps be changed to unsigned long too as well.
> >
> > > #ifdef WE_CARE_DEEPLY
> > > /* Overflow? */
> > > if (b1 < a1)
> > > return 1;
> > > #ifdef AND_ARE_ANAL
> > > if (b2 < a2)
> > > return 1;
> > > #endif
> > > #endif
> > > return !(b1 <= a2 || a1 >= b2);
> > > }
> > >
> > > but I really migth have done soemthing wrong there. It's a
> > > simple function, but somebody needs to double-check that I
> > > haven't made it worse.
> >
> > Looks correct to me.
>
> How about the patch below? Lightly tested.
>
> Ingo
>
> ------------>
> >From 35c89da82e969a2fd157478940e7ecde1e19ccc4 Mon Sep 17 00:00:00 2001
> From: Ingo Molnar <[email protected]>
> Date: Fri, 10 Jul 2009 21:38:02 +0200
> Subject: [PATCH] dma-debug: Fix the overlap() function to be correct and readable
>
> 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 <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Signed-off-by: Ingo Molnar <[email protected]>
> ---
> lib/dma-debug.c | 24 ++++++++++++------------
> 1 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index c9187fe..02fed52 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -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);
> }
>
If b1 = a2 (overlap) then this function will say 0
If a1 = b2 (overlap) then this function will say 0
if b1 > (a2 + infinite) which is not overlap this function will say 1
I think we need to test both edges.
So it should be :
return ((a2 <= b1 && b2 >= a1) || (a1 <= b2 && a2 <= b1));
Thanks,
--
JSR
On Tue, 2009-07-14 at 15:45 +0530, Jaswinder Singh Rajput wrote:
> > >From 35c89da82e969a2fd157478940e7ecde1e19ccc4 Mon Sep 17 00:00:00 2001
> > From: Ingo Molnar <[email protected]>
> > Date: Fri, 10 Jul 2009 21:38:02 +0200
> > Subject: [PATCH] dma-debug: Fix the overlap() function to be correct and readable
> >
> > 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 <[email protected]>
> > Cc: Joerg Roedel <[email protected]>
> > Signed-off-by: Ingo Molnar <[email protected]>
> > ---
> > lib/dma-debug.c | 24 ++++++++++++------------
> > 1 files changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> > index c9187fe..02fed52 100644
> > --- a/lib/dma-debug.c
> > +++ b/lib/dma-debug.c
> > @@ -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);
> > }
> >
>
> If b1 = a2 (overlap) then this function will say 0
> If a1 = b2 (overlap) then this function will say 0
>
> if b1 > (a2 + infinite) which is not overlap this function will say 1
>
> I think we need to test both edges.
>
> So it should be :
>
> return ((a2 <= b1 && b2 >= a1) || (a1 <= b2 && a2 <= b1));
>
We can make it more beautiful like :
return ((a2 <= b1 && b2 >= a1) || (a1 <= b2 && b1 >= a2));
--
JSR
On Tue, 2009-07-14 at 16:07 +0530, Jaswinder Singh Rajput wrote:
> On Tue, 2009-07-14 at 15:45 +0530, Jaswinder Singh Rajput wrote:
> > > >From 35c89da82e969a2fd157478940e7ecde1e19ccc4 Mon Sep 17 00:00:00 2001
> > > From: Ingo Molnar <[email protected]>
> > > Date: Fri, 10 Jul 2009 21:38:02 +0200
> > > Subject: [PATCH] dma-debug: Fix the overlap() function to be correct and readable
> > >
> > > 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 <[email protected]>
> > > Cc: Joerg Roedel <[email protected]>
> > > Signed-off-by: Ingo Molnar <[email protected]>
> > > ---
> > > lib/dma-debug.c | 24 ++++++++++++------------
> > > 1 files changed, 12 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> > > index c9187fe..02fed52 100644
> > > --- a/lib/dma-debug.c
> > > +++ b/lib/dma-debug.c
> > > @@ -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);
> > > }
> > >
> >
> > If b1 = a2 (overlap) then this function will say 0
> > If a1 = b2 (overlap) then this function will say 0
> >
> > if b1 > (a2 + infinite) which is not overlap this function will say 1
> >
> > I think we need to test both edges.
> >
> > So it should be :
> >
> > return ((a2 <= b1 && b2 >= a1) || (a1 <= b2 && a2 <= b1));
> >
>
> We can make it more beautiful like :
>
> return ((a2 <= b1 && b2 >= a1) || (a1 <= b2 && b1 >= a2));
>
In above case I tested overlapping on both side : left and right.
but result is same so (x || x) = x, so in simplified version we can
write :
return a1 <= b2 && b1 >= a2;
Thanks,
--
JSR