Re: [PATCH v2 2/9] livepatch: Apply vmlinux-specific KLP relocations early

[Josh Poimboeuf]

On Mon, Apr 20, 2020 at 01:57:51PM -0400, Joe Lawrence wrote:
> On Fri, Apr 17, 2020 at 09:04:27AM -0500, Josh Poimboeuf wrote:
> > 
> > [ ... snip ... ]
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 40cfac8156fd..5fda3afc0285 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > 
> > [ ... snip ... ]
> > 
> > +int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> > +			  const char *shstrtab, const char *strtab,
> > +			  unsigned int symndx, struct module *pmod,
> > +			  const char *objname)
> >  {
> >  	int i, cnt, ret = 0;
> > -	const char *objname, *secname;
> >  	char sec_objname[MODULE_NAME_LEN];
> >  	Elf_Shdr *sec;
> >  
> > -	if (WARN_ON(!klp_is_object_loaded(obj)))
> > -		return -EINVAL;
> > -
> > -	objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > -
> >  	/* For each klp relocation section */
> > -	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> > -		sec = pmod->klp_info->sechdrs + i;
> > -		secname = pmod->klp_info->secstrings + sec->sh_name;
> > +	for (i = 1; i < ehdr->e_shnum; i++) {
> > +		sec = sechdrs + i;
> 
> Hi Josh, minor bug:
> 
> Note the for loop through the section headers in
> klp_write_relocations(), but its calling function ...
> 
> > [ ... snip ... ]
> > 
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 646f1e2330d2..d36ea8a8c3ec 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
> >  		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
> >  			continue;
> >  
> > -		/* Livepatch relocation sections are applied by livepatch */
> >  		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> > -			continue;
> > -
> > -		if (info->sechdrs[i].sh_type == SHT_REL)
> > +			err = klp_write_relocations(info->hdr, info->sechdrs,
> > +						    info->secstrings,
> > +						    info->strtab,
> > +						    info->index.sym, mod, NULL);
> > +		else if (info->sechdrs[i].sh_type == SHT_REL)
> >  			err = apply_relocate(info->sechdrs, info->strtab,
> >  					     info->index.sym, i, mod);
> >  		else if (info->sechdrs[i].sh_type == SHT_RELA)
> 
> ... apply_relocations() is also iterating over the section headers (the
> diff context doesn't show it here, but i is an incrementing index over
> sechdrs[]).
> 
> So if there is more than one KLP relocation section, we'll process them
> multiple times.  At least the x86 relocation code will detect this and
> fail the module load with an invalid relocation (existing value not
> zero).

Ah, yes, good catch!

-- 
Josh