Re: New feature: Removal of the exceptions wich belongs to the init section

[Rusty Russell <rusty@rustcorp.com.au>]

In message <20031128155853.33283fec.sapuglha@yahoo.com.br> you write:
> Hello Rusty,

Hi,

> We, Juliano (julianofs@pop.com.br) and I (Tiago Sant' Anna
> (sapuglha@yahoo.com.br) developed this patch.

	I've commented on your patch: I hope you find my feedback
useful.

> diff -Nur linux-2.6.0-test9-vanilla/arch/alpha/mm/extable.c linux-2.6.0-test9-modified/arch/alpha/mm/extable.c
> --- linux-2.6.0-test9-vanilla/arch/alpha/mm/extable.c	2003-10-25 16:42:52.000000000 -0200
> +++ linux-2.6.0-test9-modified/arch/alpha/mm/extable.c	2003-11-28 10:43:18.000000000 -0200
> @@ -27,3 +27,18 @@
>  
>          return NULL;
>  }

It's generally not a good idea to write code for architectures which
you don't have access to, but to leave them for arch maintainers to
sort out.

> +/* Exception_table_entry Comparison. Only the field insn is considered. 
> +   Results:
> +	equal: 0
> +	ex1 less than ex2: -1
> +	ex1 major than ex2: 1
> +*/

This comment is a little verbose: I would simply say:

/* Compare two exception table entries: < 0 if ex1 comes before ex2. */

> +int extable_cmp(const struct exception_table_entry ex1, const struct exception_table_entry ex2) {
> +	
> +	if (ex1.insn < ex2.insn)
> +		return(-1);
> +	else if (ex1.insn > ex2.insn)
> +		return(1);
> +	return(0);
> +}

And it can be implemented as "return (long)ex1.insn - ex2.insn;",
making it a candidate for an inline function in the asm/uaccess.h
header.

> +/* Verify if the addr belongs to the init section */
> +static inline int within_mod_init_section(unsigned long addr, 
> +                                          void *start, unsigned long size)
> +{
> +	    return ((void *)addr >= start && (void *)addr < start + size);
> +}
> +
> +/* Remove exception table entries that point to init area.
> + * It will be used in the unload of init section.
> + */
> +void remove_init_exceptions(struct module *mod) {

This comment is not quite true.  The exception entries themselves are
in the __ex_table section, which is in the module code.  Some of the
"insn" pointers are into the module init section, which is to be
discarded.

This function should be in kernel/extable.c.

> +
> +	static spinlock_t init_ex_remove = SPIN_LOCK_UNLOCKED;

Do not invent your own lock.  You only need to protect against the
exception table walk by other CPUs, for which modlist_lock is
sufficient.

> +
> +	const struct exception_table_entry *local;
> +	unsigned int i;
> +	int num_init_ex=0;
> +
> +
> +	local = mod->extable;
> +	i = 1;
> +
> +	while (i <= mod->num_exentries) {
> +		if (within_mod_init_section((unsigned long) local->insn, mod->module_init, mod->init_size)) {
> +			num_init_ex++;			
> +		}
> +		else break;
> +		local = local+1;
> +		i++;
> +	}
> +
> +	local = mod->extable;

This code is incorrect.  The init section could be before or after the
core section, meaning that all the init-related exceptions will be at
the start, or at the end.  So, the code would look like this:

void remove_init_exceptions(struct module *mod)
{
	int i;

	if (!mod->module_init)
		return;

	spin_lock(&modlist_lock);
	if (mod->module_init > mod->module_core) {
		/* init exception entries at the end.  Trim */
		for (i = mod->num_exentries-1; i >= 0; i--)
			if (within(mod->extable[i].insn,
				   mod->module_init, mod->init_size))
				mod->num_exentries--;
	} else {
		/* init exception entries at the start.  Move ptr. */
		for (i = 0; i < mod->num_exentries; i++) {
			if (!within(mod->extable[i].insn,
				    mod->module_init, mod->init_size))
				break;
		}
		mod->extable += i;
		mod->num_exentries -= i;
	}
	spin_nulock(&modlist_lock);
}
		
> +	/* Unload the init exceptions */
> +	for(i=0; i < num_init_ex; ++i)
> +		kfree(local+i);

This doesn't make sense: these are not pointers, but are part of
module->core!  They will be freed in the normal way.

>  /* Free memory returned from module_alloc */
>  void module_free(struct module *mod, void *module_region)
> -{
> +{	
> +	/* Remove exception entries of the init section */
> +	if (module_region == mod->module_init) {
> +		remove_init_exceptions(mod);
> +	}
> +	

Call this from kernel/module.c directly.

> +++ linux-2.6.0-test9-modified/include/linux/extable.h	2003-11-28 10:4
0:57.000000000 -0200
> @@ -0,0 +1,51 @@
> +#ifndef _EXTABLE_H
> +#define _EXTABLE_H
> +
> +/*
> + * Functions regarding to exception's table.
> + *
> + *	* Written by Juliano Silva and Tiago Silva, 2003
> + *	
> + **/
> +
> +
> +#include <linux/module.h>
> +#include <asm/uaccess.h>
> +#include <asm/sections.h>
> +#include <linux/init.h>
> + 
> +/* Exception_table_entry Comparison. Only the field insn is considered.
> + *   Results:
> + *            equal: 0
> + *            ex1 less than ex2: -1
> + *            ex1 major than ex2: 1
> + *                                                         */
> +extern int extable_cmp(struct exception_table_entry ex1,
> +                       struct exception_table_entry ex2);
> +
> +/* This code sorts an exception table. It is very used with modules code 
> + * void __init_or_module sort_ex_table(struct exception_table_entry *start,
> + * struct exception_table_entry *finish);
> + */
> +void __init_or_module sort_ex_table(struct exception_table_entry *start, struct exception_table_entry *finish)

Never put non-inline functions in a header file.  And never inline a
function this large.

Since this is only called from module.c, perhaps put it in
include/linux/moduleloader.h and the code in extable.c.

Hope that helps,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.