[GIT PULL] core kernel fixes

[GIT PULL] core kernel fixes

[Ingo Molnar]

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)

Re: [GIT PULL] core kernel fixes

[Linus Torvalds]

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

Re: [GIT PULL] core kernel fixes

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> 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

[PATCH] dma-debug: Fix the overlap() function to be correct and readable

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> 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 <mingo@elte.hu>
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 <torvalds@linux-foundation.org>
Cc: Joerg Roedel <joerg.roedel@amd.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 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);
 	}

Re: [PATCH] dma-debug: Fix the overlap() function to be correct and readable

[Linus Torvalds]

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

Re: [PATCH] dma-debug: Fix the overlap() function to be correct and readable

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> 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

Re: [PATCH] dma-debug: Fix the overlap() function to be correct and readable

[Jaswinder Singh Rajput]

On Fri, 2009-07-10 at 21:51 +0200, Ingo Molnar wrote:
> * Ingo Molnar <mingo@elte.hu> 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 <mingo@elte.hu>
> 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 <torvalds@linux-foundation.org>
> Cc: Joerg Roedel <joerg.roedel@amd.com>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  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

Re: [PATCH] dma-debug: Fix the overlap() function to be correct and readable

[Jaswinder Singh Rajput]

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 <mingo@elte.hu>
> > 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 <torvalds@linux-foundation.org>
> > Cc: Joerg Roedel <joerg.roedel@amd.com>
> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > ---
> >  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

Re: [PATCH] dma-debug: Fix the overlap() function to be correct and readable

[Jaswinder Singh Rajput]

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 <mingo@elte.hu>
> > > 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 <torvalds@linux-foundation.org>
> > > Cc: Joerg Roedel <joerg.roedel@amd.com>
> > > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > > ---
> > >  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

Re: [GIT PULL] core kernel fixes

[Linus Torvalds]

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

Re: [GIT PULL] core kernel fixes

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> 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

[GIT PULL, v2] core kernel fixes

[Ingo Molnar]

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);
 	}

Re: [GIT PULL] core kernel fixes

[Joerg Roedel]

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