[PATCH] module: make symbol_put_addr() work for all exported symbols

[PATCH] module: make symbol_put_addr() work for all exported symbols

[Jiri Kosina]

symbol_put_addr() works only for exported function names (symbols present 
in text section). This for example means that

         symbol_put_addr(__symbol_get("any_exported_variable_name"))

triggers a BUG, which really seems wrong.

This patch introduces generic lookup_symbol_address(), which performs 
lookup on symbol tables of all modules according to the address, and makes 
symbol_put_addr() use this interface to find the module owner instead.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>

diff --git a/kernel/module.c b/kernel/module.c
index 5f80478..77ff23e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -302,6 +302,64 @@ static unsigned long find_symbol(const char *name,
  	return -ENOENT;
  }

+/* Lookup module symbol by address */
+const static struct kernel_symbol *lookup_symbol_address(unsigned long addr,
+		struct module **owner)
+{
+	struct module *mod;
+	const struct kernel_symbol *s;
+	unsigned int i;
+
+	/* Core kernel first */
+	struct {
+		const struct kernel_symbol *s_start;
+		const struct kernel_symbol *s_end;
+	} kern_syms[] = {
+		{ __start___ksymtab, __stop___ksymtab },
+		{ __start___ksymtab_gpl, __stop___ksymtab_gpl },
+		{ __start___ksymtab_gpl_future, __stop___ksymtab_gpl_future },
+		{ __start___ksymtab_unused, __stop___ksymtab_unused },
+		{ __start___ksymtab_unused_gpl, __stop___ksymtab_unused_gpl },
+	};
+
+	for (i = 0; i < ARRAY_SIZE(kern_syms); i++) {
+		for (s = kern_syms[i].s_start; s < kern_syms[i].s_end; s++) {
+			if (s->value == addr) {
+				if (owner)
+					*owner = NULL;
+				return s;
+			}
+		}
+	}
+
+	/* Now try modules */
+	list_for_each_entry(mod, &modules, list) {
+		struct {
+			const struct kernel_symbol *sym;
+			unsigned int num;
+		} arr[] = {
+			{ mod->syms, mod->num_syms },
+			{ mod->gpl_syms, mod->num_gpl_syms },
+			{ mod->gpl_future_syms, mod->num_gpl_future_syms },
+			{ mod->unused_syms, mod->num_unused_syms },
+			{ mod->unused_gpl_syms, mod->num_unused_gpl_syms },
+		};
+
+		for (i = 0; i < ARRAY_SIZE(arr); i++) {
+			for (s = arr[i].sym; s < arr[i].sym + arr[i].num; s++) {
+				if (s->value == addr) {
+					if (owner)
+						*owner = mod;
+					return s;
+				}
+			}
+		}
+	}
+	return NULL;
+}
+
+
+
  /* Search for module by name: must hold module_mutex. */
  static struct module *find_module(const char *name)
  {
@@ -799,13 +857,12 @@ EXPORT_SYMBOL(__symbol_put);
  void symbol_put_addr(void *addr)
  {
  	struct module *modaddr;
-
-	if (core_kernel_text((unsigned long)addr))
-		return;
-
-	if (!(modaddr = module_text_address((unsigned long)addr)))
+
+	if (!lookup_symbol_address((unsigned long)addr, &modaddr))
  		BUG();
-	module_put(modaddr);
+
+	if(modaddr)
+		module_put(modaddr);
  }
  EXPORT_SYMBOL_GPL(symbol_put_addr);

Re: [PATCH] module: make symbol_put_addr() work for all exported symbols

[Rusty Russell]

On Thursday 19 June 2008 23:11:54 Jiri Kosina wrote:
> symbol_put_addr() works only for exported function names (symbols present
> in text section). This for example means that
>
>          symbol_put_addr(__symbol_get("any_exported_variable_name"))
>
> triggers a BUG, which really seems wrong.

Hi Jiri,

    You're right, good catch!

> This patch introduces generic lookup_symbol_address(), which performs
> lookup on symbol tables of all modules according to the address, and makes
> symbol_put_addr() use this interface to find the module owner instead.
>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>

    It might be better to centralize all these iterators, and create a proper 
iterator function.  Any chance you could rewrite it on top of this patch? 
(Lightly tested)

Thanks!
Rusty.

module: generic each_symbol iterator function

Introduce an each_symbol() iterator to avoid duplicating the knowledge
about the 5 different sections containing symbols.  Currently only
used by find_symbol(), but will be used by symbol_put_addr() too.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r 909eac180d25 kernel/module.c
--- a/kernel/module.c	Fri Jun 20 15:56:43 2008 +1000
+++ b/kernel/module.c	Mon Jun 23 13:40:43 2008 +1000
@@ -152,6 +152,167 @@ extern const unsigned long __start___kcr
 #define symversion(base, idx) ((base != NULL) ? ((base) + (idx)) : NULL)
 #endif
 
+struct symsearch {
+	const struct kernel_symbol *start, *stop;
+	const unsigned long *crcs;
+	enum {
+		NOT_GPL_ONLY,
+		GPL_ONLY,
+		WILL_BE_GPL_ONLY,
+	} licence;
+	bool unused;
+};
+
+static bool each_symbol_in_section(const struct symsearch *arr,
+				   unsigned int arrsize,
+				   struct module *owner,
+				   bool (*fn)(const struct symsearch *syms,
+					      struct module *owner,
+					      unsigned int symnum, void *data),
+				   void *data)
+{
+	unsigned int i, j;
+
+	for (j = 0; j < arrsize; j++) {
+		for (i = 0; i < arr[j].stop - arr[j].start; i++)
+			if (fn(&arr[j], owner, i, data))
+				return true;
+	}
+
+	return false;
+}
+
+/* Returns true as soon as fn returns true, otherwise false. */
+static bool each_symbol(bool (*fn)(const struct symsearch *arr,
+				   struct module *owner,
+				   unsigned int symnum, void *data),
+			void *data)
+{
+	struct module *mod;
+	const struct symsearch arr[] = {
+		{ __start___ksymtab, __stop___ksymtab, __start___kcrctab,
+		  NOT_GPL_ONLY, false },
+		{ __start___ksymtab_gpl, __stop___ksymtab_gpl,
+		  __start___kcrctab_gpl,
+		  GPL_ONLY, false },
+		{ __start___ksymtab_gpl_future, __stop___ksymtab_gpl_future,
+		  __start___kcrctab_gpl_future, 
+		  WILL_BE_GPL_ONLY, false },
+		{ __start___ksymtab_unused, __stop___ksymtab_unused,
+		  __start___kcrctab_unused, 
+		  NOT_GPL_ONLY, true },
+		{ __start___ksymtab_unused_gpl, __stop___ksymtab_unused_gpl,
+		  __start___kcrctab_unused_gpl,
+		  GPL_ONLY, true },
+	};
+
+	if (each_symbol_in_section(arr, ARRAY_SIZE(arr), NULL, fn, data))
+		return true;
+
+	list_for_each_entry(mod, &modules, list) {
+		struct symsearch arr[] = {
+			{ mod->syms, mod->syms + mod->num_syms, mod->crcs,
+			  NOT_GPL_ONLY, false },
+			{ mod->gpl_syms, mod->gpl_syms + mod->num_gpl_syms,
+			  mod->gpl_crcs,
+			  GPL_ONLY, false },
+			{ mod->gpl_future_syms,
+			  mod->gpl_future_syms + mod->num_gpl_future_syms,
+			  mod->gpl_future_crcs,
+			  WILL_BE_GPL_ONLY, false },
+			{ mod->unused_syms,
+			  mod->unused_syms + mod->num_unused_syms,
+			  mod->unused_crcs,
+			  NOT_GPL_ONLY, true },
+			{ mod->unused_gpl_syms,
+			  mod->unused_gpl_syms + mod->num_unused_gpl_syms,
+			  mod->unused_gpl_crcs,
+			  GPL_ONLY, true },
+		};
+
+		if (each_symbol_in_section(arr, ARRAY_SIZE(arr), mod, fn, data))
+			return true;
+	}
+	return false;
+}
+
+struct find_symbol_arg
+{
+	/* Input */
+	const char *name;
+	bool gplok;
+	bool warn;
+
+	/* Output */
+	struct module *owner;
+	const unsigned long *crc;
+	unsigned long value;
+};
+
+static bool find_symbol_in_section(const struct symsearch *syms,
+				   struct module *owner,
+				   unsigned int symnum, void *data)
+{
+	struct find_symbol_arg *fsa = data;
+
+	if (strcmp(syms->start[symnum].name, fsa->name) != 0)
+		return false;
+
+	if (!fsa->gplok) {
+		if (syms->licence == GPL_ONLY)
+			return false;
+		if (syms->licence == WILL_BE_GPL_ONLY && fsa->warn) {
+			printk(KERN_WARNING "Symbol %s is being used "
+			       "by a non-GPL module, which will not "
+			       "be allowed in the future\n", fsa->name);
+			printk(KERN_WARNING "Please see the file "
+			       "Documentation/feature-removal-schedule.txt "
+			       "in the kernel source tree for more details.\n");
+		}
+	}
+
+	if (syms->unused && fsa->warn) {
+		printk(KERN_WARNING "Symbol %s is marked as UNUSED, "
+		       "however this module is using it.\n", fsa->name);
+		printk(KERN_WARNING
+		       "This symbol will go away in the future.\n");
+		printk(KERN_WARNING
+		       "Please evalute if this is the right api to use and if "
+		       "it really is, submit a report the linux kernel "
+		       "mailinglist together with submitting your code for "
+		       "inclusion.\n");
+	}
+	
+	fsa->owner = owner;
+	fsa->crc = symversion(syms->crcs, symnum);
+	fsa->value = syms->start[symnum].value;
+	return true;
+}
+
+/* Find a symbol, return value, (optional) crc and (optional) module
+ * which owns it */
+static unsigned long find_symbol(const char *name,
+				 struct module **owner,
+				 const unsigned long **crc,
+				 bool gplok,
+				 bool warn)
+{
+	struct find_symbol_arg fsa;
+
+	fsa.name = name;
+	fsa.gplok = gplok;
+	fsa.warn = warn;
+
+	if (each_symbol(find_symbol_in_section, &fsa)) {
+		*owner = fsa.owner;
+		*crc = fsa.crc;
+		return fsa.value;
+	}
+
+	DEBUGP("Failed to find symbol %s\n", name);
+	return -ENOENT;
+}
+
 /* lookup symbol in given range of kernel_symbols */
 static const struct kernel_symbol *lookup_symbol(const char *name,
 	const struct kernel_symbol *start,
@@ -162,144 +323,6 @@ static const struct kernel_symbol *looku
 		if (strcmp(ks->name, name) == 0)
 			return ks;
 	return NULL;
-}
-
-static bool always_ok(bool gplok, bool warn, const char *name)
-{
-	return true;
-}
-
-static bool printk_unused_warning(bool gplok, bool warn, const char *name)
-{
-	if (warn) {
-		printk(KERN_WARNING "Symbol %s is marked as UNUSED, "
-		       "however this module is using it.\n", name);
-		printk(KERN_WARNING
-		       "This symbol will go away in the future.\n");
-		printk(KERN_WARNING
-		       "Please evalute if this is the right api to use and if "
-		       "it really is, submit a report the linux kernel "
-		       "mailinglist together with submitting your code for "
-		       "inclusion.\n");
-	}
-	return true;
-}
-
-static bool gpl_only_unused_warning(bool gplok, bool warn, const char *name)
-{
-	if (!gplok)
-		return false;
-	return printk_unused_warning(gplok, warn, name);
-}
-
-static bool gpl_only(bool gplok, bool warn, const char *name)
-{
-	return gplok;
-}
-
-static bool warn_if_not_gpl(bool gplok, bool warn, const char *name)
-{
-	if (!gplok && warn) {
-		printk(KERN_WARNING "Symbol %s is being used "
-		       "by a non-GPL module, which will not "
-		       "be allowed in the future\n", name);
-		printk(KERN_WARNING "Please see the file "
-		       "Documentation/feature-removal-schedule.txt "
-		       "in the kernel source tree for more details.\n");
-	}
-	return true;
-}
-
-struct symsearch {
-	const struct kernel_symbol *start, *stop;
-	const unsigned long *crcs;
-	bool (*check)(bool gplok, bool warn, const char *name);
-};
-
-/* Look through this array of symbol tables for a symbol match which
- * passes the check function. */
-static const struct kernel_symbol *search_symarrays(const struct symsearch *arr,
-						    unsigned int num,
-						    const char *name,
-						    bool gplok,
-						    bool warn,
-						    const unsigned long **crc)
-{
-	unsigned int i;
-	const struct kernel_symbol *ks;
-
-	for (i = 0; i < num; i++) {
-		ks = lookup_symbol(name, arr[i].start, arr[i].stop);
-		if (!ks || !arr[i].check(gplok, warn, name))
-			continue;
-
-		if (crc)
-			*crc = symversion(arr[i].crcs, ks - arr[i].start);
-		return ks;
-	}
-	return NULL;
-}
-
-/* Find a symbol, return value, (optional) crc and (optional) module
- * which owns it */
-static unsigned long find_symbol(const char *name,
-				 struct module **owner,
-				 const unsigned long **crc,
-				 bool gplok,
-				 bool warn)
-{
-	struct module *mod;
-	const struct kernel_symbol *ks;
-	const struct symsearch arr[] = {
-		{ __start___ksymtab, __stop___ksymtab, __start___kcrctab,
-		  always_ok },
-		{ __start___ksymtab_gpl, __stop___ksymtab_gpl,
-		  __start___kcrctab_gpl, gpl_only },
-		{ __start___ksymtab_gpl_future, __stop___ksymtab_gpl_future,
-		  __start___kcrctab_gpl_future, warn_if_not_gpl },
-		{ __start___ksymtab_unused, __stop___ksymtab_unused,
-		  __start___kcrctab_unused, printk_unused_warning },
-		{ __start___ksymtab_unused_gpl, __stop___ksymtab_unused_gpl,
-		  __start___kcrctab_unused_gpl, gpl_only_unused_warning },
-	};
-
-	/* Core kernel first. */
-	ks = search_symarrays(arr, ARRAY_SIZE(arr), name, gplok, warn, crc);
-	if (ks) {
-		if (owner)
-			*owner = NULL;
-		return ks->value;
-	}
-
-	/* Now try modules. */
-	list_for_each_entry(mod, &modules, list) {
-		struct symsearch arr[] = {
-			{ mod->syms, mod->syms + mod->num_syms, mod->crcs,
-			  always_ok },
-			{ mod->gpl_syms, mod->gpl_syms + mod->num_gpl_syms,
-			  mod->gpl_crcs, gpl_only },
-			{ mod->gpl_future_syms,
-			  mod->gpl_future_syms + mod->num_gpl_future_syms,
-			  mod->gpl_future_crcs, warn_if_not_gpl },
-			{ mod->unused_syms,
-			  mod->unused_syms + mod->num_unused_syms,
-			  mod->unused_crcs, printk_unused_warning },
-			{ mod->unused_gpl_syms,
-			  mod->unused_gpl_syms + mod->num_unused_gpl_syms,
-			  mod->unused_gpl_crcs, gpl_only_unused_warning },
-		};
-
-		ks = search_symarrays(arr, ARRAY_SIZE(arr),
-				      name, gplok, warn, crc);
-		if (ks) {
-			if (owner)
-				*owner = mod;
-			return ks->value;
-		}
-	}
-
-	DEBUGP("Failed to find symbol %s\n", name);
-	return -ENOENT;
 }
 
 /* Search for module by name: must hold module_mutex. */

Re: [PATCH] module: make symbol_put_addr() work for all exported symbols

[Jiri Kosina]

On Mon, 23 Jun 2008, Rusty Russell wrote:

>    It might be better to centralize all these iterators, and create a 
> proper iterator function.  Any chance you could rewrite it on top of 
> this patch? (Lightly tested)

Hi Rusty,

yes, this looks like better approach. I will respin that patch.

> +struct find_symbol_arg
> +{
> +	/* Input */
> +	const char *name;
> +	bool gplok;
> +	bool warn;
> +
> +	/* Output */
> +	struct module *owner;
> +	const unsigned long *crc;
> +	unsigned long value;
> +};

I see two options how to do this -- either I can make 'value' input 
parameter too, so that find_symbol_in_section() could perform lookup 
either according to address or according to name (whatever is specified in 
the passed struct find_symbol_arg), or I can do a completely new lookup 
function for address-wise lookups. What would you prefer? I don't have any 
strong preference either way.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] module: make symbol_put_addr() work for all exported symbols

[Rusty Russell]

On Wednesday 25 June 2008 18:37:37 Jiri Kosina wrote:
> On Mon, 23 Jun 2008, Rusty Russell wrote:
> >    It might be better to centralize all these iterators, and create a
> > proper iterator function.  Any chance you could rewrite it on top of
> > this patch? (Lightly tested)
>
> Hi Rusty,

Hi Jiri,

> I see two options how to do this -- either I can make 'value' input
> parameter too, so that find_symbol_in_section() could perform lookup
> either according to address or according to name (whatever is specified in
> the passed struct find_symbol_arg), or I can do a completely new lookup
> function for address-wise lookups. What would you prefer? I don't have any
> strong preference either way.

A new lookup function (and hence a new structure) I think.  It'll be clearer.

Cheers,
Rusty.