[RFC] kallsyms_lookup always requires buffers

[RFC] kallsyms_lookup always requires buffers

[Franck Bui-Huu]

Hi,

Some uses of kallsyms_lookup() do not need to find out the name of
a symbol and its module's name it belongs. This is specially true
in arch specific code, which needs to unwind the stack to show the
back trace during opps (mips is an example). In this specific case,
we just need to retreive the function's size and the offset of the
active intruction inside it.

This simple patch adds a new entry "kallsyms_lookup_gently()". The
name actually sucks but I can't figure out a coherent name with the
rest of the file. If someone could give me a better idea...
This new entry does exactly the same as kallsyms_lookup() but does
not require any buffers to store any names. It returns 0 if it fails
otherwise 1.

Do you think this can be usefull ?

thanks
		Franck

-- >8 --

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 849043c..30f8d8d 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -12,6 +12,10 @@ #ifdef CONFIG_KALLSYMS
 /* Lookup the address for a symbol. Returns 0 if not found. */
 unsigned long kallsyms_lookup_name(const char *name);
 
+extern int kallsyms_lookup_gently(unsigned long addr,
+				  unsigned long *symbolsize,
+				  unsigned long *offset);
+
 /* Lookup an address.  modname is set to NULL if it's in the kernel. */
 const char *kallsyms_lookup(unsigned long addr,
 			    unsigned long *symbolsize,
@@ -28,6 +32,13 @@ static inline unsigned long kallsyms_loo
 	return 0;
 }
 
+static inline const int kallsyms_lookup_gently(unsigned long addr,
+					       unsigned long *symbolsize,
+					       unsigned long *offset)
+{
+	return 0;
+}
+
 static inline const char *kallsyms_lookup(unsigned long addr,
 					  unsigned long *symbolsize,
 					  unsigned long *offset,
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index ab16a5a..c398753 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -69,6 +69,15 @@ static inline int is_kernel(unsigned lon
 	return in_gate_area_no_task(addr);
 }
 
+static int is_kernel_addr(unsigned long addr)
+{
+	if (all_var)
+		return is_kernel(addr);
+
+	return is_kernel_text(addr) || is_kernel_inittext(addr) ||
+		is_kernel_extratext(addr);
+}
+
 /* expand a compressed symbol data into the resulting uncompressed string,
    given the offset to where the symbol is in the compressed stream */
 static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
@@ -156,6 +165,78 @@ unsigned long kallsyms_lookup_name(const
 }
 EXPORT_SYMBOL_GPL(kallsyms_lookup_name);
 
+static unsigned long get_symbol_pos(unsigned long addr,
+				    unsigned long *symbolsize,
+				    unsigned long *offset)
+{
+	unsigned long symbol_start = 0, symbol_end = 0;
+	unsigned long i, low, high, mid;
+
+	/* This kernel should never had been booted. */
+	BUG_ON(!kallsyms_addresses);
+
+	/* do a binary search on the sorted kallsyms_addresses array */
+	low = 0;
+	high = kallsyms_num_syms;
+
+	while (high - low > 1) {
+		mid = (low + high) / 2;
+		if (kallsyms_addresses[mid] <= addr)
+			low = mid;
+		else
+			high = mid;
+	}
+
+	/*
+	 * search for the first aliased symbol. Aliased
+	 * symbols are symbols with the same address
+	 */
+	while (low && kallsyms_addresses[low-1] == kallsyms_addresses[low])
+		--low;
+
+	symbol_start = kallsyms_addresses[low];
+
+	/* Search for next non-aliased symbol */
+	for (i = low + 1; i < kallsyms_num_syms; i++) {
+		if (kallsyms_addresses[i] > symbol_start) {
+			symbol_end = kallsyms_addresses[i];
+			break;
+		}
+	}
+	
+	/* if we found no next symbol, we use the end of the section */
+	if (!symbol_end) {
+		if (is_kernel_inittext(addr))
+			symbol_end = (unsigned long)_einittext;
+		else if (all_var)
+			symbol_end = (unsigned long)_end;
+		else 
+			symbol_end = (unsigned long)_etext;
+	}
+
+	*symbolsize = symbol_end - symbol_start;
+	*offset = addr - symbol_start;
+
+	return low;
+}
+
+/*
+ * Lookup an address but don't bother to find any names.
+ */
+int kallsyms_lookup_gently(unsigned long addr, unsigned long *symbolsize,
+			   unsigned long *offset)
+{
+	int rv;
+
+	if (is_kernel_addr(addr))
+		rv = !!get_symbol_pos(addr, symbolsize, offset);
+	else
+		rv = !!module_address_lookup(addr, symbolsize, offset, NULL);
+
+	return rv;
+}
+EXPORT_SYMBOL_GPL(kallsyms_lookup_gently);
+
 /*
  * Lookup an address
  * - modname is set to NULL if it's in the kernel
@@ -168,57 +249,18 @@ const char *kallsyms_lookup(unsigned lon
 			    unsigned long *offset,
 			    char **modname, char *namebuf)
 {
-	unsigned long i, low, high, mid;
 	const char *msym;
 
-	/* This kernel should never had been booted. */
-	BUG_ON(!kallsyms_addresses);
-
 	namebuf[KSYM_NAME_LEN] = 0;
 	namebuf[0] = 0;
 
-	if ((all_var && is_kernel(addr)) ||
-	    (!all_var && (is_kernel_text(addr) || is_kernel_inittext(addr) ||
-				is_kernel_extratext(addr)))) {
-		unsigned long symbol_end = 0;
-
-		/* do a binary search on the sorted kallsyms_addresses array */
-		low = 0;
-		high = kallsyms_num_syms;
-
-		while (high-low > 1) {
-			mid = (low + high) / 2;
-			if (kallsyms_addresses[mid] <= addr) low = mid;
-			else high = mid;
-		}
-
-		/* search for the first aliased symbol. Aliased symbols are
-		   symbols with the same address */
-		while (low && kallsyms_addresses[low - 1] == kallsyms_addresses[low])
-			--low;
-
+	if (is_kernel_addr(addr)) {
+		unsigned long pos;
+		
+		pos = get_symbol_pos(addr, symbolsize, offset);
 		/* Grab name */
-		kallsyms_expand_symbol(get_symbol_offset(low), namebuf);
-
-		/* Search for next non-aliased symbol */
-		for (i = low + 1; i < kallsyms_num_syms; i++) {
-			if (kallsyms_addresses[i] > kallsyms_addresses[low]) {
-				symbol_end = kallsyms_addresses[i];
-				break;
-			}
-		}
-
-		/* if we found no next symbol, we use the end of the section */
-		if (!symbol_end) {
-			if (is_kernel_inittext(addr))
-				symbol_end = (unsigned long)_einittext;
-			else
-				symbol_end = all_var ? (unsigned long)_end : (unsigned long)_etext;
-		}
-
-		*symbolsize = symbol_end - kallsyms_addresses[low];
+		kallsyms_expand_symbol(get_symbol_offset(pos), namebuf);
 		*modname = NULL;
-		*offset = addr - kallsyms_addresses[low];
 		return namebuf;
 	}
 
diff --git a/kernel/module.c b/kernel/module.c
index 2a19cd4..0e3e6ab 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2012,7 +2012,8 @@ const char *module_address_lookup(unsign
 	list_for_each_entry(mod, &modules, list) {
 		if (within(addr, mod->module_init, mod->init_size)
 		    || within(addr, mod->module_core, mod->core_size)) {
-			*modname = mod->name;
+			if (modname)
+				*modname = mod->name;
 			return get_ksymbol(mod, addr, size, offset);
 		}
 	}

Re: [RFC] kallsyms_lookup always requires buffers

[Arjan van de Ven]

> +}
> +EXPORT_SYMBOL_GPL(kallsyms_lookup_gently);


Hi,

there don't seem to be modular users so please don't export it since
that export just eats up useless space. (we have way too many of those
already)

(Also I suggest you submit at least one user with your patch but that's
another matter)

Greetings,
   Arjan van de Ven

Re: [RFC] kallsyms_lookup always requires buffers

[Franck Bui-Huu]

Hi

Arjan van de Ven wrote:
>> +}
>> +EXPORT_SYMBOL_GPL(kallsyms_lookup_gently);
> 
> 
> Hi,
> 
> there don't seem to be modular users so please don't export it since
> that export just eats up useless space. (we have way too many of those
> already)
> 

true.

> (Also I suggest you submit at least one user with your patch but that's
> another matter)
> 

mips could be one of the future users, see patch below.

thanks
		Franck

-- >8 --

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 951bf9c..eb80db5 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -344,8 +344,6 @@ static int __init frame_info_init(void)
 {
 	int i;
 #ifdef CONFIG_KALLSYMS
-	char *modname;
-	char namebuf[KSYM_NAME_LEN + 1];
 	unsigned long start, size, ofs;
 	extern char __sched_text_start[], __sched_text_end[];
 	extern char __lock_text_start[], __lock_text_end[];
@@ -354,7 +352,7 @@ #ifdef CONFIG_KALLSYMS
 	for (i = 0; i < ARRAY_SIZE(mfinfo); i++) {
 		if (start == (unsigned long)schedule)
 			schedule_frame = &mfinfo[i];
-		if (!kallsyms_lookup(start, &size, &ofs, &modname, namebuf))
+		if (!kallsyms_lookup_gently(start, &size, &ofs))
 			break;
 		mfinfo[i].func = (void *)(start + ofs);
 		mfinfo[i].func_size = size;
@@ -454,8 +452,6 @@ unsigned long unwind_stack(struct task_s
 {
 	unsigned long stack_page;
 	struct mips_frame_info info;
-	char *modname;
-	char namebuf[KSYM_NAME_LEN + 1];
 	unsigned long size, ofs;
 	int leaf;
 
@@ -463,7 +459,7 @@ unsigned long unwind_stack(struct task_s
 	if (!stack_page)
 		return 0;
 
-	if (!kallsyms_lookup(pc, &size, &ofs, &modname, namebuf))
+	if (!kallsyms_lookup_gently(pc, &size, &ofs))
 		return 0;
 	if (ofs == 0)
 		return 0;

Re: [RFC] kallsyms_lookup always requires buffers

[Paulo Marques]

Franck Bui-Huu wrote:
> Hi,
> 
> Some uses of kallsyms_lookup() do not need to find out the name of
> a symbol and its module's name it belongs. This is specially true
> in arch specific code, which needs to unwind the stack to show the
> back trace during opps (mips is an example). In this specific case,
> we just need to retreive the function's size and the offset of the
> active intruction inside it.
> 
> This simple patch adds a new entry "kallsyms_lookup_gently()". The
> name actually sucks but I can't figure out a coherent name with the
> rest of the file. If someone could give me a better idea...

kallsyms_lookup_size_offset() ?

Granted it is a little bigger, but it tells exactly what it does and 
there are not that many users that we have to write this name all that 
often.

> This new entry does exactly the same as kallsyms_lookup() but does
> not require any buffers to store any names. It returns 0 if it fails
> otherwise 1.
> 
> Do you think this can be usefull ?

You tell me, since you're proposing the change ;)

> diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
> index 849043c..30f8d8d 100644
> --- a/include/linux/kallsyms.h
> +++ b/include/linux/kallsyms.h
> @@ -12,6 +12,10 @@ #ifdef CONFIG_KALLSYMS
>  /* Lookup the address for a symbol. Returns 0 if not found. */
>  unsigned long kallsyms_lookup_name(const char *name);
>  
> +extern int kallsyms_lookup_gently(unsigned long addr,
> +				  unsigned long *symbolsize,
> +				  unsigned long *offset);
> +
>  /* Lookup an address.  modname is set to NULL if it's in the kernel. */
>  const char *kallsyms_lookup(unsigned long addr,
>  			    unsigned long *symbolsize,
> @@ -28,6 +32,13 @@ static inline unsigned long kallsyms_loo
>  	return 0;
>  }
>  
> +static inline const int kallsyms_lookup_gently(unsigned long addr,
> +					       unsigned long *symbolsize,
> +					       unsigned long *offset)
> +{
> +	return 0;
> +}
> +
>  static inline const char *kallsyms_lookup(unsigned long addr,
>  					  unsigned long *symbolsize,
>  					  unsigned long *offset,
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index ab16a5a..c398753 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -69,6 +69,15 @@ static inline int is_kernel(unsigned lon
>  	return in_gate_area_no_task(addr);
>  }
>  
> +static int is_kernel_addr(unsigned long addr)

Maybe change the name to kallsyms_is_kernel_addr, because this function 
does more things than what the generic name implies.

> +{
> +	if (all_var)
> +		return is_kernel(addr);
> +
> +	return is_kernel_text(addr) || is_kernel_inittext(addr) ||
> +		is_kernel_extratext(addr);
> +}
> +

[...]
> +/*
> + * Lookup an address but don't bother to find any names.
> + */
> +int kallsyms_lookup_gently(unsigned long addr, unsigned long *symbolsize,
> +			   unsigned long *offset)
> +{
> +	int rv;
> +
> +	if (is_kernel_addr(addr))
> +		rv = !!get_symbol_pos(addr, symbolsize, offset);
> +	else
> +		rv = !!module_address_lookup(addr, symbolsize, offset, NULL);
> +
> +	return rv;
> +}

<minor nitpick>

Why not just:

 > if (is_kernel_addr(addr))
 >	return !!get_symbol_pos(addr, symbolsize, offset);
 >
 > return !!module_address_lookup(addr, symbolsize, offset, NULL);

and just get rid of "rv" completely?

</minor nitpick>

> +EXPORT_SYMBOL_GPL(kallsyms_lookup_gently);

I agree with Arjan here. If kallsyms_lookup wasn't exported, I don't see 
a reason to export this either.

Everything else seems fine.

-- 
Paulo Marques - www.grupopie.com

"The face of a child can say it all, especially the
mouth part of the face."

Re: [RFC] kallsyms_lookup always requires buffers

[Franck Bui-Huu]

Hi

Paulo Marques wrote:
> Franck Bui-Huu wrote:
>>
>> This simple patch adds a new entry "kallsyms_lookup_gently()". The
>> name actually sucks but I can't figure out a coherent name with the
>> rest of the file. If someone could give me a better idea...
> 
> kallsyms_lookup_size_offset() ?
> 
> Granted it is a little bigger, but it tells exactly what it does and
> there are not that many users that we have to write this name all that
> often.
> 

ok.

>> This new entry does exactly the same as kallsyms_lookup() but does
>> not require any buffers to store any names. It returns 0 if it fails
>> otherwise 1.
>>
>> Do you think this can be usefull ?
> 
> You tell me, since you're proposing the change ;)
> 

:)
actually the question was rather "does this change look sensible to you ?"

>> +static int is_kernel_addr(unsigned long addr)
> 
> Maybe change the name to kallsyms_is_kernel_addr, because this function
> does more things than what the generic name implies.
> 

it sounds like a public function name. Maybe is_ksym_addr() is better ?

>> + * Lookup an address but don't bother to find any names.
>> + */
>> +int kallsyms_lookup_gently(unsigned long addr, unsigned long
>> *symbolsize,
>> +               unsigned long *offset)
>> +{
>> +    int rv;
>> +
>> +    if (is_kernel_addr(addr))
>> +        rv = !!get_symbol_pos(addr, symbolsize, offset);
>> +    else
>> +        rv = !!module_address_lookup(addr, symbolsize, offset, NULL);
>> +
>> +    return rv;
>> +}
> 
> <minor nitpick>
> 
> Why not just:
> 
>> if (is_kernel_addr(addr))
>>    return !!get_symbol_pos(addr, symbolsize, offset);
>>
>> return !!module_address_lookup(addr, symbolsize, offset, NULL);
> 
> and just get rid of "rv" completely?
> 
> </minor nitpick>
> 

No problem.

>> +EXPORT_SYMBOL_GPL(kallsyms_lookup_gently);
> 
> I agree with Arjan here. If kallsyms_lookup wasn't exported, I don't see
> a reason to export this either.
> 

Already removed.

thanks
		Franck