Re: [RFC PATCH v4 2/6] module: preserve Elf information for livepatch modules

[Petr Mladek]

On Tue 2016-02-09 11:33:07, Jiri Kosina wrote:
> On Tue, 9 Feb 2016, Petr Mladek wrote:
> 
> > > +#ifdef CONFIG_KALLSYMS
> > > +	/* Make symtab and strtab available prior to module init call */
> > > +	mod->num_symtab = mod->core_num_syms;
> > > +	mod->symtab = mod->core_symtab;
> > > +	mod->strtab = mod->core_strtab;
> > > +#endif
> > 
> > This should be done with module_mutex. Otherwise, it looks racy at least 
> > against module_kallsyms_on_each_symbol().
> 
> Hmm, if this is the case, the comment describing the locking rules for 
> module_mutex should be updated.
> 
> > BTW: I wonder why even the original code is not racy for example against 
> > module_get_kallsym. It is called without the mutex. This code sets the 
> > number of entries before the pointer to the entries.
> > 
> > Note that the module is in the list even in the UNFORMED state.
> 
> module_kallsyms_on_each_symbol() gives up in such case though.

The problem is that we set the three values above in the COMMING
state. They were even set in the LIVE state before this patch.

IMHO, we should reorder the writes and add a write barrier.


#ifdef CONFIG_KALLSYMS
	/* Make symtab and strtab available prior to module init call */
	mod->strtab = mod->core_strtab;
	mod->symtab = mod->core_symtab;
	/* Tables must be availabe before the number is set */
	smp_wmb();
	mod->num_symtab = mod->core_num_syms;
#endif


Then we should add the corresponding read barrier to
the read functions that are protected only by
the disabled preeption. For example:


static unsigned long mod_find_symname(struct module *mod, const char *name)
{
	unsigned int i;

	for (i = 0; i < mod->num_symtab; i++)
		/* Make sure that tables are set for the read num_symtab */
		smp_rmb();
		if (strcmp(name, mod->strtab+mod->symtab[i].st_name) == 0 &&
		    mod->symtab[i].st_info != 'U')
			return mod->symtab[i].st_value;
	return 0;
}


Or am I too paranoid, please?

Best Regards,
Petr