[PATCH v3] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Kamalesh Babulal]

On Tuesday 31 October 2017 07:49 PM, Naveen N . Rao wrote:
> Hi Kamalesh,
> Sorry for the late review. Overall, the patch looks good to me. So:
> Acked-by: Naveen N. Rao <naveen.n.rao at linux.vnet.ibm.com>
>
> However, I have a few minor comments which can be addressed in a
> subsequent patch.
>

Thanks for the review.

[...]

>> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
>> index 0b0f896..005aaea 100644
>> --- a/arch/powerpc/kernel/module_64.c
>> +++ b/arch/powerpc/kernel/module_64.c

[...]

>> @@ -239,10 +257,19 @@ static void relaswap(void *_x, void *_y, int size)
>>
>>  /* Get size of potential trampolines required. */
>>  static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
>> -				    const Elf64_Shdr *sechdrs)
>> +				    const Elf64_Shdr *sechdrs,
>> +				    struct module *me)
>>  {
>>  	/* One extra reloc so it's always 0-funcaddr terminated */
>>  	unsigned long relocs = 1;
>
> You might want to get rid of 'relocs' above and simply use the below
> two.
>
>> +	/*
>> +	 * size of livepatch stub is 28 instructions, whereas the
>> +	 * non-livepatch stub requires 7 instructions. Account for
>> +	 * different stub sizes and track the livepatch relocation
>> +	 * count in me->arch.klp_relocs.
>> +	 */
>> +	unsigned long sec_relocs = 0;
>> +	unsigned long klp_relocs = 0;
>
> You should also initialize this to 1 (similar to relocs above) for use
> in the iterators below. Or, update the iterators to use
> me->arch.klp_relocs (1)

relocs is Sum(sec_relocs), whereas per section relocation count is 
assigned to sec_relocs.  If the section is livepatch section, then it
added to klp_relocs or else to relocs. sec_relocs acts like a temp 
variable to hold the current section relocation count.

>
>>  	unsigned i;
>>
>>  	/* Every relocated section... */
>> @@ -262,9 +289,14 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
>>  			     sechdrs[i].sh_size / sizeof(Elf64_Rela),
>>  			     sizeof(Elf64_Rela), relacmp, relaswap);
>>
>> -			relocs += count_relocs((void *)sechdrs[i].sh_addr,
>> +			sec_relocs = count_relocs((void *)sechdrs[i].sh_addr,
>>  					       sechdrs[i].sh_size
>>  					       / sizeof(Elf64_Rela));
>
> How about also capturing 'sec_relocs' in 'struct mod_arch_specific'? (2)
> That will help simplify a lot of the calculations here and elsewhere.
> Note that there are many other places where the number of stubs is
> derived looking at 'sh_size', which is incorrect since we now have klp
> stubs as well (create_ftrace_stub() for instance).
>
>> +			relocs += sec_relocs;
>> +#ifdef CONFIG_LIVEPATCH
>> +			if (sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
>> +				klp_relocs += sec_relocs;
>> +#endif
>
> If a module has SHF_RELA_LIVEPATCH, but the kernel is compiled without
> CONFIG_LIVEPATCH, should we refuse to load the kernel module?

Yes, the module load will fail.

>
> You might want to consider removing the above #ifdef and processing some
> of these flags regardless of CONFIG_LIVEPATCH.

ifdef guard, can be replaced with check for SHF_RELA_LIVEPATCH in 
section flag.

>
>>  		}
>>  	}
>>
>> @@ -273,6 +305,15 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
>>  	relocs++;
>>  #endif
>>
>> +	relocs -= klp_relocs;
>> +#ifdef CONFIG_LIVEPATCH
>> +	me->arch.klp_relocs = klp_relocs;
>> +
>> +	pr_debug("Looks like a total of %lu stubs, (%lu) livepatch stubs, max\n",
> 						   ^^^^^
> 						   (%lu livepatch stubs)
>
> Just wondering why the brackets are the way they are...

Makes it better to use the brackets like you suggested.


>
>> +				relocs, klp_relocs);
>> +	return (relocs * sizeof(struct ppc64_stub_entry) +
>> +		klp_relocs * sizeof(struct ppc64le_klp_stub_entry));
>> +#endif
>>  	pr_debug("Looks like a total of %lu stubs, max\n", relocs);
>>  	return relocs * sizeof(struct ppc64_stub_entry);
>>  }

[...]

>>
>> +#ifdef CONFIG_LIVEPATCH
>> +static unsigned long klp_stub_for_addr(const Elf64_Shdr *sechdrs,
>> +				       unsigned long addr,
>> +				       struct module *me)
>> +{
>> +	struct ppc64le_klp_stub_entry *klp_stubs;
>> +	unsigned int num_klp_stubs = me->arch.klp_relocs;
>> +	unsigned int i, num_stubs;
>> +
>> +	num_stubs = (sechdrs[me->arch.stubs_section].sh_size -
>> +		    (num_klp_stubs * sizeof(*klp_stubs))) /
>> +				sizeof(struct ppc64_stub_entry);
>
> (2) This can be simplified if we have me->arch.sec_relocs.

Having it will make the calculation look simple:
num_stubs = (me->arch.sec_relocs * size(struct ppc64_stub_entry).


>
>> +
>> +	/*
>> +	 * Create livepatch stubs after the regular stubs.
>> +	 */
>> +	klp_stubs = (void *)sechdrs[me->arch.stubs_section].sh_addr +
>> +		    (num_stubs * sizeof(struct ppc64_stub_entry));
>> +	for (i = 0; stub_func_addr(klp_stubs[i].funcdata); i++) {
> 		    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 		    (1) This needs us to allocate one additional stub.
>
>

Good catch, yes there should an extra stub.

-- 
cheers,
Kamalesh.