Re: Bug with paravirt ops and livepatches

[Chris J Arges]

On Fri, Apr 01, 2016 at 05:46:52PM +0200, Miroslav Benes wrote:
> On Fri, 1 Apr 2016, Jiri Kosina wrote:
> 
> > On Tue, 29 Mar 2016, Jiri Kosina wrote:
> > 
> > > Agreed; I think we should be safe applying all the alternatives (with 
> > > paravirt being really just a special case of those) to the coming module 
> > > at the very last phase; they really are required only during runtime, 
> > > but nothing else should be depending on them. Right? If anyone is able 
> > > to come up with and counter-example, please speak up :)
> > 
> > So I have quickly gone through all the architectures that actually do 
> > overload __weak module_finalize() by their own implementation, and except 
> > for applying self-modifying code changes and registering unwind tables, 
> > there doesn't seem to be any relevant heavy-lifting, that'd need to be 
> > done before relocations have been written.
> > 
> > Is the (completely untested) sort-of-a-patch below a complete rubbish 
> > (on top of current livepatching.git's for-next)?
> > 
> > 
> > 
> > 
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 5f71aa6..c003648 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3211,7 +3211,7 @@ int __weak module_finalize(const Elf_Ehdr *hdr,
> >  	return 0;
> >  }
> >  
> > -static int post_relocation(struct module *mod, const struct load_info *info)
> > +static void post_relocation(struct module *mod, const struct load_info *info)
> >  {
> >  	/* Sort exception table now relocations are done. */
> >  	sort_extable(mod->extable, mod->extable + mod->num_exentries);
> > @@ -3222,9 +3222,6 @@ static int post_relocation(struct module *mod, const struct load_info *info)
> >  
> >  	/* Setup kallsyms-specific fields. */
> >  	add_kallsyms(mod, info);
> > -
> > -	/* Arch-specific module finalizing. */
> > -	return module_finalize(info->hdr, info->sechdrs, mod);
> >  }
> >  
> >  /* Is this module of this name done loading?  No locks held. */
> > @@ -3562,9 +3559,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >  	if (err < 0)
> >  		goto free_modinfo;
> >  
> > -	err = post_relocation(mod, info);
> > -	if (err < 0)
> > -		goto free_modinfo;
> > +	post_relocation(mod, info);
> >  
> >  	flush_module_icache(mod);
> >  
> > @@ -3589,6 +3584,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >  	if (err)
> >  		goto bug_cleanup;
> >  
> > +	/* Arch-specific module finalizing. */
> > +	err = module_finalize(info->hdr, info->sechdrs, mod);
> > +	if (err)
> > +		goto bug_cleanup;
> 
> goto coming_cleanup;
> 
> Otherwise it looks ok. I'll give it a proper look on Monday though.
> 
> Miroslav

I'll test this out and see if it fixes the original issue.
--chris