Skip to contentSkip to conversation

consolidate all within() implementations

PATCH In review Peter Oberparleiter 5 participants 9 messages May 19, 2008 – May 21, 2008

Subsystems (2)
x86 x86 MM
lore
Share & tools
lore

Peter Oberparleiter

author
#0 lore raw

PATCH 1/1 consolidate all within() implementations

From: Peter Oberparleiter <[email protected]>

This patch consolidates a number of different implementations of the within() function which checks whether an address is within a specified address range. Apart from parameter typing, existing implementations can be classified in two categories which differ in the way the range is specified:

1) by start and end address
2) by start and size

These categories are covered by the within() macro (case 1) and the within_len() macro (case 2). Both macros can be used with any pointer or pointer-equivalent type as parameter.

Signed-off-by Peter Oberparleiter <[email protected]>
---
 arch/x86/mm/pageattr.c |    7 +------
 include/linux/kernel.h |   24 ++++++++++++++++++++++++
 kernel/lockdep.c       |   10 +++-------
 kernel/module.c        |   25 ++++++++++---------------
 4 files changed, 38 insertions(+), 28 deletions(-)
include/linux/kernel.h +24 -0
Index: linux-2.6.26-rc3/include/linux/kernel.h
===================================================================
@@ -434,6 +434,30 @@ static inline char *pack_hex_byte(char *
 	__val > __max ? __max: __val; })
 
 /**
+ * within - check whether address is within a start-and-end address range
+ * @val: address
+ * @start: start address (included in range)
+ * @end: end address (excluded from range)
+ */
+#define within(val, start, end) ({			\
+	unsigned long __val = (unsigned long) (val);	\
+	unsigned long __start = (unsigned long) (start);	\
+	unsigned long __end = (unsigned long) (end);	\
+	(__val >= __start) && (__val < __end); })
+
+/**
+ * within_len - check whether address is within a start-and-length address range
+ * @val: address
+ * @start: start of range
+ * @len: number of bytes in range
+ */
+#define within_len(val, start, len) ({				\
+	unsigned long __val = (unsigned long) (val);		\
+	unsigned long __start = (unsigned long) (start);	\
+	unsigned long __end = __start + (unsigned long) (len);	\
+	(__val >= __start) && (__val < __end); })
+
+/**
  * container_of - cast a member of a structure out to the containing structure
  * @ptr:	the pointer to the member.
  * @type:	the type of the container struct this is embedded in.
kernel/lockdep.c +3 -7
Index: linux-2.6.26-rc3/kernel/lockdep.c
===================================================================
@@ -25,6 +25,7 @@
  * Thanks to Arjan van de Ven for coming up with the initial idea of
  * mapping lock dependencies runtime.
  */
+#include <linux/kernel.h>
 #include <linux/mutex.h>
 #include <linux/sched.h>
 #include <linux/delay.h>

@@ -2932,11 +2933,6 @@ static void zap_class(struct lock_class 
 
 }
 
-static inline int within(const void *addr, void *start, unsigned long size)
-{
-	return addr >= start && addr < start + size;
-}
-
 void lockdep_free_key_range(void *start, unsigned long size)
 {
 	struct lock_class *class, *next;

@@ -2956,9 +2952,9 @@ void lockdep_free_key_range(void *start,
 		if (list_empty(head))
 			continue;
 		list_for_each_entry_safe(class, next, head, hash_entry) {
-			if (within(class->key, start, size))
+			if (within_len(class->key, start, size))
 				zap_class(class);
-			else if (within(class->name, start, size))
+			else if (within_len(class->name, start, size))
 				zap_class(class);
 		}
 	}
kernel/module.c +10 -15
Index: linux-2.6.26-rc3/kernel/module.c
===================================================================
@@ -2262,11 +2262,6 @@ sys_init_module(void __user *umod,
 	return 0;
 }
 
-static inline int within(unsigned long addr, void *start, unsigned long size)
-{
-	return ((void *)addr >= start && (void *)addr < start + size);
-}
-
 #ifdef CONFIG_KALLSYMS
 /*
  * This ignores the intensely annoying "mapping symbols" found

@@ -2287,7 +2282,7 @@ static const char *get_ksymbol(struct mo
 	unsigned long nextval;
 
 	/* At worse, next value is at end of module */
-	if (within(addr, mod->module_init, mod->init_size))
+	if (within_len(addr, mod->module_init, mod->init_size))
 		nextval = (unsigned long)mod->module_init+mod->init_text_size;
 	else
 		nextval = (unsigned long)mod->module_core+mod->core_text_size;

@@ -2335,8 +2330,8 @@ const char *module_address_lookup(unsign
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size)
-		    || within(addr, mod->module_core, mod->core_size)) {
+		if (within_len(addr, mod->module_init, mod->init_size)
+		    || within_len(addr, mod->module_core, mod->core_size)) {
 			if (modname)
 				*modname = mod->name;
 			ret = get_ksymbol(mod, addr, size, offset);

@@ -2358,8 +2353,8 @@ int lookup_module_symbol_name(unsigned l
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size) ||
-		    within(addr, mod->module_core, mod->core_size)) {
+		if (within_len(addr, mod->module_init, mod->init_size) ||
+		    within_len(addr, mod->module_core, mod->core_size)) {
 			const char *sym;
 
 			sym = get_ksymbol(mod, addr, NULL, NULL);

@@ -2382,8 +2377,8 @@ int lookup_module_symbol_attrs(unsigned 
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size) ||
-		    within(addr, mod->module_core, mod->core_size)) {
+		if (within_len(addr, mod->module_init, mod->init_size) ||
+		    within_len(addr, mod->module_core, mod->core_size)) {
 			const char *sym;
 
 			sym = get_ksymbol(mod, addr, size, offset);

@@ -2579,7 +2574,7 @@ int is_module_address(unsigned long addr
 	preempt_disable();
 
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_core, mod->core_size)) {
+		if (within_len(addr, mod->module_core, mod->core_size)) {
 			preempt_enable();
 			return 1;
 		}

@@ -2597,8 +2592,8 @@ struct module *__module_text_address(uns
 	struct module *mod;
 
 	list_for_each_entry(mod, &modules, list)
-		if (within(addr, mod->module_init, mod->init_text_size)
-		    || within(addr, mod->module_core, mod->core_text_size))
+		if (within_len(addr, mod->module_init, mod->init_text_size)
+		    || within_len(addr, mod->module_core, mod->core_text_size))
 			return mod;
 	return NULL;
 }
arch/x86/mm/pageattr.c +1 -6
Index: linux-2.6.26-rc3/arch/x86/mm/pageattr.c
===================================================================
@@ -2,6 +2,7 @@
  * Copyright 2002 Andi Kleen, SuSE Labs.
  * Thanks to Ben LaHaise for precious feedback.
  */
+#include <linux/kernel.h>
 #include <linux/highmem.h>
 #include <linux/bootmem.h>
 #include <linux/module.h>

@@ -54,12 +55,6 @@ static inline unsigned long highmap_end_
 # define debug_pagealloc 0
 #endif
 
-static inline int
-within(unsigned long addr, unsigned long start, unsigned long end)
-{
-	return addr >= start && addr < end;
-}
-
 /*
  * Flushing functions
  */
#1 lore raw
Quoted 8 lines · Peter Oberparleiter

From: Peter Oberparleiter <[email protected]>

This patch consolidates a number of different implementations of the within() function which checks whether an address is within a specified address range. Apart from parameter typing, existing implementations can be classified in two categories which differ in the way the range is specified:

Quoted 6 lines

1) by start and end address
2) by start and size

These categories are covered by the within() macro (case 1) and the within_len() macro (case 2). Both macros can be used with any pointer or pointer-equivalent type as parameter.

Would it be that hard to just make them static inlines taking unsigned longs?

Quoted 7 lines
Signed-off-by Peter Oberparleiter <[email protected]>

---

/**
+ * within - check whether address is within a start-and-end address range
+ * @val: address

@addr perhaps

Quoted 4 lines

+ * @start: start address (included in range)
+ * @end: end address (excluded from range)
+ */
+#define within(val, start, end) ({ \

How about:
static inline int addr_within(unsigned long addr, unsigned long start,
unsigned long end)

include/linux/kernel.h static inline char *pack_hex_byte(char * · line 450

Quoted 8 lines
include/linux/kernel.h +8 -0
+	unsigned long __val = (unsigned long) (val);	\
+	unsigned long __start = (unsigned long) (start);	\
+	unsigned long __end = (unsigned long) (end);	\
+	(__val >= __start) && (__val < __end); })
+
+/**
+ * within_len - check whether address is within a start-and-length address range
+ * @val: address

@addr

Quoted 4 lines

+ * @start: start of range
+ * @len: number of bytes in range
+ */
+#define within_len(val, start, len) ({ \

static inline int addr_within_len(unsigned long addr, unsigned long start,
unsigned long len)

Just a thought.

Harvey

#2 lore raw
Quoted 9 lines · Harvey Harrison
Quoted 5 lines · Peter Oberparleiter

From: Peter Oberparleiter <[email protected]>

This patch consolidates a number of different implementations of the within() function which checks whether an address is within a specified address range.

Would it be that hard to just make them static inlines taking unsigned longs?

I was hoping to get by without the spray of unsigned long casts that entails the enforcement of this specific parameter type, seeing that each implementation had it's own combination of longs and void *.

On the other hand, an inline function would of course be the cleaner approach, so if the code owners agree, here goes take #2:

--

From: Peter Oberparleiter <[email protected]>

This patch consolidates a number of different implementations of the within() function which checks whether an address is within a specified address range. Apart from parameter typing, existing implementations can be classified in two categories which differ in the way the range is specified:

1) by start and end address
2) by start and size

Case 1) is covered by addr_within() while 2) is covered by addr_within_len().

Signed-off-by Peter Oberparleiter <[email protected]>
---
 arch/x86/mm/pageattr.c |   20 ++++++++------------
 include/linux/kernel.h |   24 ++++++++++++++++++++++++
 kernel/lockdep.c       |   12 +++++-------
 kernel/module.c        |   35 ++++++++++++++++++++---------------
 4 files changed, 57 insertions(+), 34 deletions(-)
include/linux/kernel.h +24 -0
Index: linux-2.6.26-rc3/include/linux/kernel.h
===================================================================
@@ -434,6 +434,30 @@ static inline char *pack_hex_byte(char *
 	__val > __max ? __max: __val; })
 
 /**
+ * addr_within - check whether address is in start-and-end address range
+ * @addr: address
+ * @start: start address (included in range)
+ * @end: end address (excluded from range)
+ */
+static inline int addr_within(unsigned long addr, unsigned long start,
+			      unsigned long end)
+{
+	return (addr >= start) && (addr < end);
+}
+
+/**
+ * addr_within_len - check whether address is in start-and-length address range
+ * @addr: address
+ * @start: start of range
+ * @len: number of bytes in range
+ */
+static inline int addr_within_len(unsigned long addr, unsigned long start,
+				  unsigned long len)
+{
+	return (addr >= start) && (addr < (start + len));
+}
+
+/**
  * container_of - cast a member of a structure out to the containing structure
  * @ptr:	the pointer to the member.
  * @type:	the type of the container struct this is embedded in.
kernel/lockdep.c +5 -7
Index: linux-2.6.26-rc3/kernel/lockdep.c
===================================================================
@@ -25,6 +25,7 @@
  * Thanks to Arjan van de Ven for coming up with the initial idea of
  * mapping lock dependencies runtime.
  */
+#include <linux/kernel.h>
 #include <linux/mutex.h>
 #include <linux/sched.h>
 #include <linux/delay.h>

@@ -2932,11 +2933,6 @@ static void zap_class(struct lock_class 
 
 }
 
-static inline int within(const void *addr, void *start, unsigned long size)
-{
-	return addr >= start && addr < start + size;
-}
-
 void lockdep_free_key_range(void *start, unsigned long size)
 {
 	struct lock_class *class, *next;

@@ -2956,9 +2952,11 @@ void lockdep_free_key_range(void *start,
 		if (list_empty(head))
 			continue;
 		list_for_each_entry_safe(class, next, head, hash_entry) {
-			if (within(class->key, start, size))
+			if (addr_within_len((unsigned long) class->key,
+					    (unsigned long) start, size))
 				zap_class(class);
-			else if (within(class->name, start, size))
+			else if (addr_within_len((unsigned long) class->name,
+						 (unsigned long) start, size))
 				zap_class(class);
 		}
 	}
kernel/module.c +20 -15
Index: linux-2.6.26-rc3/kernel/module.c
===================================================================
@@ -2262,11 +2262,6 @@ sys_init_module(void __user *umod,
 	return 0;
 }
 
-static inline int within(unsigned long addr, void *start, unsigned long size)
-{
-	return ((void *)addr >= start && (void *)addr < start + size);
-}
-
 #ifdef CONFIG_KALLSYMS
 /*
  * This ignores the intensely annoying "mapping symbols" found

@@ -2287,7 +2282,8 @@ static const char *get_ksymbol(struct mo
 	unsigned long nextval;
 
 	/* At worse, next value is at end of module */
-	if (within(addr, mod->module_init, mod->init_size))
+	if (addr_within_len(addr, (unsigned long) mod->module_init,
+			    mod->init_size))
 		nextval = (unsigned long)mod->module_init+mod->init_text_size;
 	else
 		nextval = (unsigned long)mod->module_core+mod->core_text_size;

@@ -2335,8 +2331,10 @@ const char *module_address_lookup(unsign
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size)
-		    || within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len(addr, (unsigned long) mod->module_init,
+				    mod->init_size)
+		    || addr_within_len(addr, (unsigned long) mod->module_core,
+				       mod->core_size)) {
 			if (modname)
 				*modname = mod->name;
 			ret = get_ksymbol(mod, addr, size, offset);

@@ -2358,8 +2356,10 @@ int lookup_module_symbol_name(unsigned l
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size) ||
-		    within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len(addr, (unsigned long) mod->module_init,
+				    mod->init_size) ||
+		    addr_within_len(addr, (unsigned long) mod->module_core,
+				    mod->core_size)) {
 			const char *sym;
 
 			sym = get_ksymbol(mod, addr, NULL, NULL);

@@ -2382,8 +2382,10 @@ int lookup_module_symbol_attrs(unsigned 
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size) ||
-		    within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len(addr, (unsigned long) mod->module_init,
+				    mod->init_size) ||
+		    addr_within_len(addr, (unsigned long) mod->module_core,
+				    mod->core_size)) {
 			const char *sym;
 
 			sym = get_ksymbol(mod, addr, size, offset);

@@ -2579,7 +2581,8 @@ int is_module_address(unsigned long addr
 	preempt_disable();
 
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len(addr, (unsigned long) mod->module_core,
+				    mod->core_size)) {
 			preempt_enable();
 			return 1;
 		}

@@ -2597,8 +2600,10 @@ struct module *__module_text_address(uns
 	struct module *mod;
 
 	list_for_each_entry(mod, &modules, list)
-		if (within(addr, mod->module_init, mod->init_text_size)
-		    || within(addr, mod->module_core, mod->core_text_size))
+		if (addr_within_len(addr, (unsigned long) mod->module_init,
+				    mod->init_text_size)
+		    || addr_within_len(addr, (unsigned long) mod->module_core,
+				       mod->core_text_size))
 			return mod;
 	return NULL;
 }
arch/x86/mm/pageattr.c +8 -12
Index: linux-2.6.26-rc3/arch/x86/mm/pageattr.c
===================================================================
@@ -2,6 +2,7 @@
  * Copyright 2002 Andi Kleen, SuSE Labs.
  * Thanks to Ben LaHaise for precious feedback.
  */
+#include <linux/kernel.h>
 #include <linux/highmem.h>
 #include <linux/bootmem.h>
 #include <linux/module.h>

@@ -54,12 +55,6 @@ static inline unsigned long highmap_end_
 # define debug_pagealloc 0
 #endif
 
-static inline int
-within(unsigned long addr, unsigned long start, unsigned long end)
-{
-	return addr >= start && addr < end;
-}
-
 /*
  * Flushing functions
  */

@@ -164,7 +159,7 @@ static inline pgprot_t static_protection
 	 * The BIOS area between 640k and 1Mb needs to be executable for
 	 * PCI BIOS based config access (CONFIG_PCI_GOBIOS) support.
 	 */
-	if (within(pfn, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >> PAGE_SHIFT))
+	if (addr_within(pfn, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >> PAGE_SHIFT))
 		pgprot_val(forbidden) |= _PAGE_NX;
 
 	/*

@@ -172,14 +167,14 @@ static inline pgprot_t static_protection
 	 * Does not cover __inittext since that is gone later on. On
 	 * 64bit we do not enforce !NX on the low mapping
 	 */
-	if (within(address, (unsigned long)_text, (unsigned long)_etext))
+	if (addr_within(address, (unsigned long)_text, (unsigned long)_etext))
 		pgprot_val(forbidden) |= _PAGE_NX;
 
 	/*
 	 * The .rodata section needs to be read-only. Using the pfn
 	 * catches all aliases.
 	 */
-	if (within(pfn, __pa((unsigned long)__start_rodata) >> PAGE_SHIFT,
+	if (addr_within(pfn, __pa((unsigned long)__start_rodata) >> PAGE_SHIFT,
 		   __pa((unsigned long)__end_rodata) >> PAGE_SHIFT))
 		pgprot_val(forbidden) |= _PAGE_RW;
 

@@ -620,7 +615,7 @@ static int cpa_process_alias(struct cpa_
 	 * No need to redo, when the primary call touched the direct
 	 * mapping already:
 	 */
-	if (!within(cpa->vaddr, PAGE_OFFSET,
+	if (!addr_within(cpa->vaddr, PAGE_OFFSET,
 		    PAGE_OFFSET + (max_pfn_mapped << PAGE_SHIFT))) {
 
 		alias_cpa = *cpa;

@@ -636,14 +631,15 @@ static int cpa_process_alias(struct cpa_
 	 * No need to redo, when the primary call touched the high
 	 * mapping already:
 	 */
-	if (within(cpa->vaddr, (unsigned long) _text, (unsigned long) _end))
+	if (addr_within(cpa->vaddr, (unsigned long) _text,
+			(unsigned long) _end))
 		return 0;
 
 	/*
 	 * If the physical address is inside the kernel map, we need
 	 * to touch the high mapped kernel as well:
 	 */
-	if (!within(cpa->pfn, highmap_start_pfn(), highmap_end_pfn()))
+	if (!addr_within(cpa->pfn, highmap_start_pfn(), highmap_end_pfn()))
 		return 0;
 
 	alias_cpa = *cpa;
#3 lore raw
Quoted 62 lines · On Tue, 20 May 2008 10:08:32 +0200 Peter Oberparleiter…
Quoted 9 lines · Harvey Harrison
Quoted 5 lines · Peter Oberparleiter

From: Peter Oberparleiter <[email protected]>

This patch consolidates a number of different implementations of the within() function which checks whether an address is within a specified address range.

Would it be that hard to just make them static inlines taking unsigned longs?

I was hoping to get by without the spray of unsigned long casts that entails the enforcement of this specific parameter type, seeing that each implementation had it's own combination of longs and void *.

On the other hand, an inline function would of course be the cleaner approach, so if the code owners agree, here goes take #2:

--

From: Peter Oberparleiter <[email protected]>

This patch consolidates a number of different implementations of the within() function which checks whether an address is within a specified address range. Apart from parameter typing, existing implementations can be classified in two categories which differ in the way the range is specified:

1) by start and end address
2) by start and size

Case 1) is covered by addr_within() while 2) is covered by addr_within_len().

Signed-off-by Peter Oberparleiter <[email protected]>
---
 arch/x86/mm/pageattr.c |   20 ++++++++------------
 include/linux/kernel.h |   24 ++++++++++++++++++++++++
 kernel/lockdep.c       |   12 +++++-------
 kernel/module.c        |   35 ++++++++++++++++++++---------------
 4 files changed, 57 insertions(+), 34 deletions(-)

Index: linux-2.6.26-rc3/include/linux/kernel.h
===================================================================

include/linux/kernel.h +12 -0
@@ -434,6 +434,30 @@ static inline char *pack_hex_byte(char *
 	__val > __max ? __max: __val; })
 
 /**
+ * addr_within - check whether address is in start-and-end address range
+ * @addr: address
+ * @start: start address (included in range)
+ * @end: end address (excluded from range)
+ */
+static inline int addr_within(unsigned long addr, unsigned long start,
+			      unsigned long end)
+{
+	return (addr >= start) && (addr < end);
+}
+
+/**

include/linux/kernel.h static inline char *pack_hex_byte(char * · line 458

Quoted 10 lines
include/linux/kernel.h +10 -0
+ * addr_within_len - check whether address is in start-and-length address range
+ * @addr: address
+ * @start: start of range
+ * @len: number of bytes in range
+ */
+static inline int addr_within_len(unsigned long addr, unsigned long start,
+				  unsigned long len)
+{
+	return (addr >= start) && (addr < (start + len));
+}

The kernel's use of unsigned long to represent pointers sometimes makes sense, but often gets us into a mess. It's OK in bootup code which fiddles with memory map layout because there is no reason why such code will ever dereference any of the addresses.

But I don't think we can assume this usage pattern when creating a kernel-wide facility in kernel.h.

So yes, I do think that an address-comparison tool like this should operate on void*'s. (They will need to be const void*'s).

Quoted 4 lines
include/linux/kernel.h +4 -0
+			if (addr_within_len((unsigned long) class->key,
+					    (unsigned long) start, size))
+			else if (addr_within_len((unsigned long) class->name,
+						 (unsigned long) start, size))

include/linux/kernel.h static inline char *pack_hex_byte(char *

Quoted 10 lines
include/linux/kernel.h +10 -0
+	if (addr_within_len(addr, (unsigned long) mod->module_init,
+		if (addr_within_len(addr, (unsigned long) mod->module_init,
+		    || addr_within_len(addr, (unsigned long) mod->module_core,
+		if (addr_within_len(addr, (unsigned long) mod->module_init,
+		    addr_within_len(addr, (unsigned long) mod->module_core,
+		if (addr_within_len(addr, (unsigned long) mod->module_init,
+		    addr_within_len(addr, (unsigned long) mod->module_core,
+		if (addr_within_len(addr, (unsigned long) mod->module_core,
+		if (addr_within_len(addr, (unsigned long) mod->module_init,
+		    || addr_within_len(addr, (unsigned long) mod->module_core,

And you've had to add a great pile of casts anwyay?

author
#4 lore raw
Quoted 18 lines · Andrew Morton

Review

Quoted 11 lines · On Tue, 20 May 2008 10:08:32 +0200 Peter Oberparleiter…
(quoted diff) +11 -0
+/**
+ * addr_within_len - check whether address is in start-and-length address range
+ * @addr: address
+ * @start: start of range
+ * @len: number of bytes in range
+ */
+static inline int addr_within_len(unsigned long addr, unsigned long start,
+				  unsigned long len)
+{
+	return (addr >= start) && (addr < (start + len));
+}

The kernel's use of unsigned long to represent pointers sometimes makes sense, but often gets us into a mess. It's OK in bootup code which fiddles with memory map layout because there is no reason why such code will ever dereference any of the addresses.

Quoted 5 lines

But I don't think we can assume this usage pattern when creating a kernel-wide facility in kernel.h.

So yes, I do think that an address-comparison tool like this should operate on void*'s. (They will need to be const void*'s).

Now that every line using a within() has to be changed anyway, it could as well be done right, yes.

Quoted 1 line

And you've had to add a great pile of casts anwyay?

Well, between the unsigned long and the void * version, there's not that much of a difference in the number of casts. So here goes #3:

--

From: Peter Oberparleiter <[email protected]>

This patch consolidates a number of different implementations of the within() function which checks whether an address is within a specified address range. Apart from parameter typing, existing implementations can be classified in two categories which differ in the way the range is specified:

1) by start and end address
2) by start and size

Case 1) is covered by addr_within() while 2) is covered by addr_within_len().

Signed-off-by Peter Oberparleiter <[email protected]>
---
 arch/x86/mm/pageattr.c |   26 ++++++++++++--------------
 include/linux/kernel.h |   26 ++++++++++++++++++++++++++
 kernel/lockdep.c       |   10 +++-------
 kernel/module.c        |   34 +++++++++++++++++++---------------
 4 files changed, 60 insertions(+), 36 deletions(-)
include/linux/kernel.h +26 -0
Index: linux-2.6.26-rc3/include/linux/kernel.h
===================================================================
@@ -434,6 +434,32 @@ static inline char *pack_hex_byte(char *
 	__val > __max ? __max: __val; })
 
 /**
+ * addr_within - check whether address is in start-and-end address range
+ * @addr: address
+ * @start: start address (included in range)
+ * @end: end address (excluded from range)
+ */
+static inline int addr_within(const void *addr, const void *start,
+			      const void *end)
+{
+	return ((unsigned long) addr >= (unsigned long) start) &&
+	       ((unsigned long) addr < (unsigned long) end);
+}
+
+/**
+ * addr_within_len - check whether address is in start-and-length address range
+ * @addr: address
+ * @start: start of range
+ * @len: number of bytes in range
+ */
+static inline int addr_within_len(const void *addr, const void *start,
+				  size_t len)
+{
+	return ((unsigned long) addr >= (unsigned long) start) &&
+	       ((unsigned long) addr < ((unsigned long) start + len));
+}
+
+/**
  * container_of - cast a member of a structure out to the containing structure
  * @ptr:	the pointer to the member.
  * @type:	the type of the container struct this is embedded in.
kernel/lockdep.c +3 -7
Index: linux-2.6.26-rc3/kernel/lockdep.c
===================================================================
@@ -25,6 +25,7 @@
  * Thanks to Arjan van de Ven for coming up with the initial idea of
  * mapping lock dependencies runtime.
  */
+#include <linux/kernel.h>
 #include <linux/mutex.h>
 #include <linux/sched.h>
 #include <linux/delay.h>

@@ -2932,11 +2933,6 @@ static void zap_class(struct lock_class 
 
 }
 
-static inline int within(const void *addr, void *start, unsigned long size)
-{
-	return addr >= start && addr < start + size;
-}
-
 void lockdep_free_key_range(void *start, unsigned long size)
 {
 	struct lock_class *class, *next;

@@ -2956,9 +2952,9 @@ void lockdep_free_key_range(void *start,
 		if (list_empty(head))
 			continue;
 		list_for_each_entry_safe(class, next, head, hash_entry) {
-			if (within(class->key, start, size))
+			if (addr_within_len(class->key, start, size))
 				zap_class(class);
-			else if (within(class->name, start, size))
+			else if (addr_within_len(class->name, start, size))
 				zap_class(class);
 		}
 	}
kernel/module.c +19 -15
Index: linux-2.6.26-rc3/kernel/module.c
===================================================================
@@ -2262,11 +2262,6 @@ sys_init_module(void __user *umod,
 	return 0;
 }
 
-static inline int within(unsigned long addr, void *start, unsigned long size)
-{
-	return ((void *)addr >= start && (void *)addr < start + size);
-}
-
 #ifdef CONFIG_KALLSYMS
 /*
  * This ignores the intensely annoying "mapping symbols" found

@@ -2287,7 +2282,7 @@ static const char *get_ksymbol(struct mo
 	unsigned long nextval;
 
 	/* At worse, next value is at end of module */
-	if (within(addr, mod->module_init, mod->init_size))
+	if (addr_within_len((void *) addr, mod->module_init, mod->init_size))
 		nextval = (unsigned long)mod->module_init+mod->init_text_size;
 	else
 		nextval = (unsigned long)mod->module_core+mod->core_text_size;

@@ -2335,8 +2330,10 @@ const char *module_address_lookup(unsign
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size)
-		    || within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len((void *) addr, mod->module_init,
+				    mod->init_size)
+		    || addr_within_len((void *) addr, mod->module_core,
+				       mod->core_size)) {
 			if (modname)
 				*modname = mod->name;
 			ret = get_ksymbol(mod, addr, size, offset);

@@ -2358,8 +2355,10 @@ int lookup_module_symbol_name(unsigned l
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size) ||
-		    within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len((void *) addr, mod->module_init,
+				    mod->init_size) ||
+		    addr_within_len((void *) addr, mod->module_core,
+				    mod->core_size)) {
 			const char *sym;
 
 			sym = get_ksymbol(mod, addr, NULL, NULL);

@@ -2382,8 +2381,10 @@ int lookup_module_symbol_attrs(unsigned 
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size) ||
-		    within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len((void *) addr, mod->module_init,
+				    mod->init_size) ||
+		    addr_within_len((void *) addr, mod->module_core,
+				    mod->core_size)) {
 			const char *sym;
 
 			sym = get_ksymbol(mod, addr, size, offset);

@@ -2579,7 +2580,8 @@ int is_module_address(unsigned long addr
 	preempt_disable();
 
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len((void *) addr, mod->module_core,
+				    mod->core_size)) {
 			preempt_enable();
 			return 1;
 		}

@@ -2597,8 +2599,10 @@ struct module *__module_text_address(uns
 	struct module *mod;
 
 	list_for_each_entry(mod, &modules, list)
-		if (within(addr, mod->module_init, mod->init_text_size)
-		    || within(addr, mod->module_core, mod->core_text_size))
+		if (addr_within_len((void *) addr, mod->module_init,
+				    mod->init_text_size)
+		    || addr_within_len((void *) addr, mod->module_core,
+				       mod->core_text_size))
 			return mod;
 	return NULL;
 }
arch/x86/mm/pageattr.c +12 -14
Index: linux-2.6.26-rc3/arch/x86/mm/pageattr.c
===================================================================
@@ -2,6 +2,7 @@
  * Copyright 2002 Andi Kleen, SuSE Labs.
  * Thanks to Ben LaHaise for precious feedback.
  */
+#include <linux/kernel.h>
 #include <linux/highmem.h>
 #include <linux/bootmem.h>
 #include <linux/module.h>

@@ -54,12 +55,6 @@ static inline unsigned long highmap_end_
 # define debug_pagealloc 0
 #endif
 
-static inline int
-within(unsigned long addr, unsigned long start, unsigned long end)
-{
-	return addr >= start && addr < end;
-}
-
 /*
  * Flushing functions
  */

@@ -164,7 +159,8 @@ static inline pgprot_t static_protection
 	 * The BIOS area between 640k and 1Mb needs to be executable for
 	 * PCI BIOS based config access (CONFIG_PCI_GOBIOS) support.
 	 */
-	if (within(pfn, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >> PAGE_SHIFT))
+	if (addr_within((void *) pfn, (void *) (BIOS_BEGIN >> PAGE_SHIFT),
+			(void *) (BIOS_END >> PAGE_SHIFT)))
 		pgprot_val(forbidden) |= _PAGE_NX;
 
 	/*

@@ -172,15 +168,16 @@ static inline pgprot_t static_protection
 	 * Does not cover __inittext since that is gone later on. On
 	 * 64bit we do not enforce !NX on the low mapping
 	 */
-	if (within(address, (unsigned long)_text, (unsigned long)_etext))
+	if (addr_within((void *) address, _text, _etext))
 		pgprot_val(forbidden) |= _PAGE_NX;
 
 	/*
 	 * The .rodata section needs to be read-only. Using the pfn
 	 * catches all aliases.
 	 */
-	if (within(pfn, __pa((unsigned long)__start_rodata) >> PAGE_SHIFT,
-		   __pa((unsigned long)__end_rodata) >> PAGE_SHIFT))
+	if (addr_within((void *) pfn,
+			(void *) (__pa(__start_rodata) >> PAGE_SHIFT),
+			(void *) (__pa(__end_rodata) >> PAGE_SHIFT)))
 		pgprot_val(forbidden) |= _PAGE_RW;
 
 	prot = __pgprot(pgprot_val(prot) & ~pgprot_val(forbidden));

@@ -620,8 +617,8 @@ static int cpa_process_alias(struct cpa_
 	 * No need to redo, when the primary call touched the direct
 	 * mapping already:
 	 */
-	if (!within(cpa->vaddr, PAGE_OFFSET,
-		    PAGE_OFFSET + (max_pfn_mapped << PAGE_SHIFT))) {
+	if (!addr_within((void *) cpa->vaddr, (void *) PAGE_OFFSET,
+		    (void *) (PAGE_OFFSET + (max_pfn_mapped << PAGE_SHIFT)))) {
 
 		alias_cpa = *cpa;
 		alias_cpa.vaddr = (unsigned long) __va(cpa->pfn << PAGE_SHIFT);

@@ -636,14 +633,15 @@ static int cpa_process_alias(struct cpa_
 	 * No need to redo, when the primary call touched the high
 	 * mapping already:
 	 */
-	if (within(cpa->vaddr, (unsigned long) _text, (unsigned long) _end))
+	if (addr_within((void *) cpa->vaddr, _text, _end))
 		return 0;
 
 	/*
 	 * If the physical address is inside the kernel map, we need
 	 * to touch the high mapped kernel as well:
 	 */
-	if (!within(cpa->pfn, highmap_start_pfn(), highmap_end_pfn()))
+	if (!addr_within((void *) cpa->pfn, (void *) highmap_start_pfn(),
+			 (void *) highmap_end_pfn()))
 		return 0;
 
 	alias_cpa = *cpa;
#5 lore raw
Quoted 16 lines

/**
+ * addr_within - check whether address is in start-and-end address range
+ * @addr: address
+ * @start: start address (included in range)
+ * @end: end address (excluded from range)
+ */

include/linux/kernel.h +9 -0
+static inline int addr_within(const void *addr, const void *start,
+			      const void *end)
+{
+	return ((unsigned long) addr >= (unsigned long) start) &&
+	       ((unsigned long) addr < (unsigned long) end);
+}
+
+/**
+ * addr_within_len - check whether address is in start-and-length address range

include/linux/kernel.h static inline char *pack_hex_byte(char * · line 460

Quoted 10 lines
include/linux/kernel.h +10 -0
+ * @addr: address
+ * @start: start of range
+ * @len: number of bytes in range
+ */
+static inline int addr_within_len(const void *addr, const void *start,
+				  size_t len)
+{
+	return ((unsigned long) addr >= (unsigned long) start) &&
+	       ((unsigned long) addr < ((unsigned long) start + len));
+}

might be my braindamage, but I'd have written it like:

static inline int
addr_within_len(const void *addr, const void *start, size_t len)

{
	return (unsigned long)addr - (unsigned long)start < len;
}

static inline int
addr_within(const void *add, const void *start, const void *end)

{
	return addr_within_len(addr, start, 
			(unsigned long)end - (unsigned long)start);
}
author
#6 lore raw

Peter Zijlstra <[email protected]> wrote on 21.05.2008 12:04:26:

Quoted 1 line
Quoted 1 line

+static inline int addr_within_len(const void *addr, const void

*start,

Quoted 8 lines

include/linux/kernel.h static inline char *pack_hex_byte(char * · line 460

Quoted 5 lines
include/linux/kernel.h +5 -0
+              size_t len)
+{
+   return ((unsigned long) addr >= (unsigned long) start) &&
+          ((unsigned long) addr < ((unsigned long) start + len));
+}

might be my braindamage, but I'd have written it like:

Quoted 5 lines

static inline int
addr_within_len(const void *addr, const void *start, size_t len)

{
   return (unsigned long)addr - (unsigned long)start < len;
}

Definitely another way to put it. In my opinion the intention of the implementation is more easily understood though when spelling it out as (a>=b) && (a<c).

Quoted 6 lines

static inline int
addr_within(const void *add, const void *start, const void *end) {
return addr_within_len(addr, start,

         (unsigned long)end - (unsigned long)start);
}

For empty ranges (start > end), this produces different (less expected) results than the previous version.

Regards,
Peter

#7 lore raw
Quoted 17 lines · On Wed, 2008-05-21 at 12:33 +0200, Peter 1 Oberparleiter wrote:

Peter Zijlstra <[email protected]> wrote on 21.05.2008 12:04:26:

Quoted 1 line
Quoted 1 line

+static inline int addr_within_len(const void *addr, const void

*start,

Quoted 8 lines

include/linux/kernel.h static inline char *pack_hex_byte(char * · line 460

Quoted 5 lines
include/linux/kernel.h +5 -0
+              size_t len)
+{
+   return ((unsigned long) addr >= (unsigned long) start) &&
+          ((unsigned long) addr < ((unsigned long) start + len));
+}

might be my braindamage, but I'd have written it like:

Quoted 5 lines

static inline int
addr_within_len(const void *addr, const void *start, size_t len)

{
   return (unsigned long)addr - (unsigned long)start < len;
}
Quoted 3 lines

Definitely another way to put it. In my opinion the intention of the implementation is more easily understood though when spelling it out as (a>=b) && (a<c).

peter@lappy:~/tmp$ cat cmp.c

int within_len1(const void *addr, const void *start, unsigned long len)

{
        return (unsigned long)addr - (unsigned long)start < len;
}

int within1(const void *addr, const void *start, const void *end) {
return within_len1(addr, start,
(unsigned long)end - (unsigned long)start);
}

peter@lappy:~/tmp$ cat cmp2.c
int within_len2(const void *addr, const void *start, unsigned long len)
{
        return ((unsigned long) addr >= (unsigned long) start) &&
                ((unsigned long) addr < ((unsigned long) start + len));
}

int within2(const void *addr, const void *start, const void *end)

{
        return ((unsigned long) addr >= (unsigned long) start) &&
                ((unsigned long) addr < ((unsigned long) end));
}
peter@lappy:~/tmp$ gcc -S -Os cmp*.c
peter@lappy:~/tmp$ ls -la cmp*.o
-rw-r--r-- 1 peter peter 752 2008-05-21 12:43 cmp2.o
-rw-r--r-- 1 peter peter 743 2008-05-21 12:43 cmp.o

Also look at the .s output and notice mine doesn't have any additional branches ;-)

Quoted 9 lines
Quoted 6 lines

static inline int
addr_within(const void *add, const void *start, const void *end) {
return addr_within_len(addr, start,

         (unsigned long)end - (unsigned long)start);
}

For empty ranges (start > end), this produces different (less expected) results than the previous version.

agreed, do we care about those?

author
#8 lore raw

Peter Zijlstra <[email protected]> wrote on 21.05.2008 12:48:52:

Review

Quoted 7 lines · On Wed, 2008-05-21 at 12:33 +0200, Peter 1 Oberparleiter wrote:
Quoted 1 line

Peter Zijlstra <[email protected]> wrote on 21.05.2008 12:04:26:

peter@lappy:~/tmp$ gcc -S -Os cmp*.c
peter@lappy:~/tmp$ ls -la cmp*.o
(quoted diff) +0 -2
-rw-r--r-- 1 peter peter 752 2008-05-21 12:43 cmp2.o
-rw-r--r-- 1 peter peter 743 2008-05-21 12:43 cmp.o

Yeah, but!

[oberpar@local cmp]$ gcc -c -O2 cmp*.c [oberpar@local cmp]$ ls -la cmp*.o
-rw-r--r-- 1 oberpar oberpar 1352 May 21 15:40 cmp2.o -rw-r--r-- 1 oberpar oberpar 1408 May 21 15:40 cmp.o

:)

Quoted 2 lines

Also look at the .s output and notice mine doesn't have any additional branches ;-)

It really boils down to the question whether you want to trade a bit of obviousness for a bit of efficiency/clever programming. I vote for keeping the former.

Quoted 8 lines
Quoted 8 lines
Quoted 6 lines

static inline int
addr_within(const void *add, const void *start, const void *end) {
return addr_within_len(addr, start,

         (unsigned long)end - (unsigned long)start);
}

For empty ranges (start > end), this produces different (less

expected)

Quoted 3 lines
Quoted 1 line

results than the previous version.

agreed, do we care about those?

Why not plan for the unexpected when it comes at little cost?

Regards,
Peter