Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1361863 > unrolled thread

Re: [PATCH v5 4/6] livepatch: reuse module loader code to write relocations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 4/6] livepatch: reuse module loader code to write  relocations Miroslav Benes <mbenes@suse.cz> - 2016-03-21 15:00 +0100
    Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2016-03-21 20:20 +0100
      Re: livepatch: reuse module loader code to write relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-21 20:30 +0100
      Re: livepatch: reuse module loader code to write relocations Jiri Kosina <jikos@kernel.org> - 2016-03-21 22:20 +0100
        Re: livepatch: reuse module loader code to write relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-21 22:40 +0100
          Re: livepatch: reuse module loader code to write relocations Jiri Kosina <jikos@kernel.org> - 2016-03-21 23:10 +0100
            Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2016-03-22 20:10 +0100

#1361863 — Re: [PATCH v5 4/6] livepatch: reuse module loader code to write relocations

On Wed, 16 Mar 2016, Jessica Yu wrote:

[...]

> +struct klp_buf {
> +	char symname[KSYM_SYMBOL_LEN];

I think it is better to make this KSYM_NAME_LEN. KSYM_SYMBOL_LEN looks 
like something different and KSYM_NAME_LEN is 128 which you reference 
below.

> +	char objname[MODULE_NAME_LEN];
> +};

[...]

> +static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
> +{
> +	int i, cnt, vmlinux, ret;
> +	struct klp_buf bufs = {0};
> +	Elf_Rela *relas;
> +	Elf_Sym *sym;
> +	char *symname;
> +	unsigned long sympos;
> +
> +	relas = (Elf_Rela *) relasec->sh_addr;
> +	/* For each rela in this klp relocation section */
> +	for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
> +		sym = pmod->core_kallsyms.symtab + ELF_R_SYM(relas[i].r_info);
> +		if (sym->st_shndx != SHN_LIVEPATCH)
> +			return -EINVAL;
> +
> +		klp_clear_buf(&bufs);
> +
> +		/* Format: .klp.sym.objname.symbol_name,sympos */
> +		symname = pmod->core_kallsyms.strtab + sym->st_name;
> +		cnt = sscanf(symname, ".klp.sym.%64[^.].%128[^,],%lu",
> +			     bufs.objname, bufs.symname, &sympos);

It would be really nice to change actual values for their macro 
definitions, but this would be a mess which is not worth it. Anyway 
shouldn't those width modifiers be %63 and %127 to make a room for \0?

> +		if (cnt != 3)
> +			return -EINVAL;
> +
> +		/* klp_find_object_symbol() treats a NULL objname as vmlinux */
> +		vmlinux = !strcmp(bufs.objname, "vmlinux");
> +		ret = klp_find_object_symbol(vmlinux ? NULL : bufs.objname,
> +					     bufs.symname, sympos,
> +					     (unsigned long *) &sym->st_value);
> +		if (ret)
> +			return ret;
>  	}
> -	preempt_enable();
>  
> -	/*
> -	 * Check if it's in another .o within the patch module. This also
> -	 * checks that the external symbol is unique.
> -	 */
> -	return klp_find_object_symbol(pmod->name, name, 0, addr);
> +	return 0;
>  }
>  
>  static int klp_write_object_relocations(struct module *pmod,
>  					struct klp_object *obj)
>  {
> -	int ret = 0;
> -	unsigned long val;
> -	struct klp_reloc *reloc;
> +	int i, cnt, ret = 0;
> +	const char *objname, *secname;
> +	struct klp_buf bufs = {0};
> +	Elf_Shdr *sec;
>  
>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>  		return -EINVAL;
>  
> -	if (WARN_ON(!obj->relocs))
> -		return -EINVAL;
> +	objname = klp_is_module(obj) ? obj->name : "vmlinux";
>  
>  	module_disable_ro(pmod);
> +	/* For each klp relocation section */
> +	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> +		sec = pmod->klp_info->sechdrs + i;
> +		if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
> +			continue;
>  
> -	for (reloc = obj->relocs; reloc->name; reloc++) {
> -		/* discover the address of the referenced symbol */
> -		if (reloc->external) {
> -			if (reloc->sympos > 0) {
> -				pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
> -				       reloc->name);
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -			ret = klp_find_external_symbol(pmod, reloc->name, &val);
> -		} else
> -			ret = klp_find_object_symbol(obj->name,
> -						     reloc->name,
> -						     reloc->sympos,
> -						     &val);
> -		if (ret)
> -			goto out;
> +		klp_clear_buf(&bufs);
>  
> -		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
> -					     val + reloc->addend);
> -		if (ret) {
> -			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
> -			       reloc->name, val, ret);
> -			goto out;
> +		/* Check if this klp relocation section belongs to obj */
> +		secname = pmod->klp_info->secstrings + sec->sh_name;
> +		cnt = sscanf(secname, ".klp.rela.%64[^.]", bufs.objname);

Same here.

Otherwise it looks really good (which applies for the whole series), so 
after fixing these nits you can add my

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

Cheers,
Miroslav

[toc] | [next] | [standalone]

#1362073 — Re: livepatch: reuse module loader code to write relocations

+++ Miroslav Benes [21/03/16 14:55 +0100]:
>On Wed, 16 Mar 2016, Jessica Yu wrote:
>
>[...]
>
>> +struct klp_buf {
>> +	char symname[KSYM_SYMBOL_LEN];
>
>I think it is better to make this KSYM_NAME_LEN. KSYM_SYMBOL_LEN looks
>like something different and KSYM_NAME_LEN is 128 which you reference
>below.
>

Ack, I did mean to use KSYM_NAME_LEN, thanks.

>> +	char objname[MODULE_NAME_LEN];
>> +};
>
>[...]
>
>> +static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
>> +{
>> +	int i, cnt, vmlinux, ret;
>> +	struct klp_buf bufs = {0};
>> +	Elf_Rela *relas;
>> +	Elf_Sym *sym;
>> +	char *symname;
>> +	unsigned long sympos;
>> +
>> +	relas = (Elf_Rela *) relasec->sh_addr;
>> +	/* For each rela in this klp relocation section */
>> +	for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
>> +		sym = pmod->core_kallsyms.symtab + ELF_R_SYM(relas[i].r_info);
>> +		if (sym->st_shndx != SHN_LIVEPATCH)
>> +			return -EINVAL;
>> +
>> +		klp_clear_buf(&bufs);
>> +
>> +		/* Format: .klp.sym.objname.symbol_name,sympos */
>> +		symname = pmod->core_kallsyms.strtab + sym->st_name;
>> +		cnt = sscanf(symname, ".klp.sym.%64[^.].%128[^,],%lu",
>> +			     bufs.objname, bufs.symname, &sympos);
>
>It would be really nice to change actual values for their macro
>definitions, but this would be a mess which is not worth it. Anyway
>shouldn't those width modifiers be %63 and %127 to make a room for \0?
>

Yes, this is a concern and I'm not sure what the best way to fix it
is. If both MODULE_NAME_LEN and KSYM_NAME_LEN were straight up
constants, then I think Josh's stringify approach would have worked
perfectly. However since MODULE_NAME_LEN translates to an expression
(64 - sizeof(unsigned long)), which the preprocessor cannot evaluate,
we will need another approach. Building the format strings at run time
might be messier than we'd like. Alternatively we could just go the
simple route and simply be a bit more aggressive on the upper bound
for the format width; though the size of long varies on different
architectures, afaik the max size it could ever be on any arch is 8
bytes, so perhaps 64 - 8 = 56 (then - 1 to make room for \0) might be
an appropriate field width. This would deserve a comment as well.

>> +		if (cnt != 3)
>> +			return -EINVAL;
>> +
>> +		/* klp_find_object_symbol() treats a NULL objname as vmlinux */
>> +		vmlinux = !strcmp(bufs.objname, "vmlinux");
>> +		ret = klp_find_object_symbol(vmlinux ? NULL : bufs.objname,
>> +					     bufs.symname, sympos,
>> +					     (unsigned long *) &sym->st_value);
>> +		if (ret)
>> +			return ret;
>>  	}
>> -	preempt_enable();
>>
>> -	/*
>> -	 * Check if it's in another .o within the patch module. This also
>> -	 * checks that the external symbol is unique.
>> -	 */
>> -	return klp_find_object_symbol(pmod->name, name, 0, addr);
>> +	return 0;
>>  }
>>
>>  static int klp_write_object_relocations(struct module *pmod,
>>  					struct klp_object *obj)
>>  {
>> -	int ret = 0;
>> -	unsigned long val;
>> -	struct klp_reloc *reloc;
>> +	int i, cnt, ret = 0;
>> +	const char *objname, *secname;
>> +	struct klp_buf bufs = {0};
>> +	Elf_Shdr *sec;
>>
>>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>>  		return -EINVAL;
>>
>> -	if (WARN_ON(!obj->relocs))
>> -		return -EINVAL;
>> +	objname = klp_is_module(obj) ? obj->name : "vmlinux";
>>
>>  	module_disable_ro(pmod);
>> +	/* For each klp relocation section */
>> +	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
>> +		sec = pmod->klp_info->sechdrs + i;
>> +		if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
>> +			continue;
>>
>> -	for (reloc = obj->relocs; reloc->name; reloc++) {
>> -		/* discover the address of the referenced symbol */
>> -		if (reloc->external) {
>> -			if (reloc->sympos > 0) {
>> -				pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
>> -				       reloc->name);
>> -				ret = -EINVAL;
>> -				goto out;
>> -			}
>> -			ret = klp_find_external_symbol(pmod, reloc->name, &val);
>> -		} else
>> -			ret = klp_find_object_symbol(obj->name,
>> -						     reloc->name,
>> -						     reloc->sympos,
>> -						     &val);
>> -		if (ret)
>> -			goto out;
>> +		klp_clear_buf(&bufs);
>>
>> -		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
>> -					     val + reloc->addend);
>> -		if (ret) {
>> -			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
>> -			       reloc->name, val, ret);
>> -			goto out;
>> +		/* Check if this klp relocation section belongs to obj */
>> +		secname = pmod->klp_info->secstrings + sec->sh_name;
>> +		cnt = sscanf(secname, ".klp.rela.%64[^.]", bufs.objname);
>
>Same here.
>
>Otherwise it looks really good (which applies for the whole series), so
>after fixing these nits you can add my
>
>Reviewed-by: Miroslav Benes <mbenes@suse.cz>
>
>Cheers,
>Miroslav

[toc] | [prev] | [next] | [standalone]

#1362079 — Re: livepatch: reuse module loader code to write relocations

On Mon, Mar 21, 2016 at 03:18:32PM -0400, Jessica Yu wrote:
> +++ Miroslav Benes [21/03/16 14:55 +0100]:
> >On Wed, 16 Mar 2016, Jessica Yu wrote:
> >
> >[...]
> >
> >>+struct klp_buf {
> >>+	char symname[KSYM_SYMBOL_LEN];
> >
> >I think it is better to make this KSYM_NAME_LEN. KSYM_SYMBOL_LEN looks
> >like something different and KSYM_NAME_LEN is 128 which you reference
> >below.
> >
> 
> Ack, I did mean to use KSYM_NAME_LEN, thanks.
> 
> >>+	char objname[MODULE_NAME_LEN];
> >>+};
> >
> >[...]
> >
> >>+static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
> >>+{
> >>+	int i, cnt, vmlinux, ret;
> >>+	struct klp_buf bufs = {0};
> >>+	Elf_Rela *relas;
> >>+	Elf_Sym *sym;
> >>+	char *symname;
> >>+	unsigned long sympos;
> >>+
> >>+	relas = (Elf_Rela *) relasec->sh_addr;
> >>+	/* For each rela in this klp relocation section */
> >>+	for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
> >>+		sym = pmod->core_kallsyms.symtab + ELF_R_SYM(relas[i].r_info);
> >>+		if (sym->st_shndx != SHN_LIVEPATCH)
> >>+			return -EINVAL;
> >>+
> >>+		klp_clear_buf(&bufs);
> >>+
> >>+		/* Format: .klp.sym.objname.symbol_name,sympos */
> >>+		symname = pmod->core_kallsyms.strtab + sym->st_name;
> >>+		cnt = sscanf(symname, ".klp.sym.%64[^.].%128[^,],%lu",
> >>+			     bufs.objname, bufs.symname, &sympos);
> >
> >It would be really nice to change actual values for their macro
> >definitions, but this would be a mess which is not worth it. Anyway
> >shouldn't those width modifiers be %63 and %127 to make a room for \0?
> >
> 
> Yes, this is a concern and I'm not sure what the best way to fix it
> is. If both MODULE_NAME_LEN and KSYM_NAME_LEN were straight up
> constants, then I think Josh's stringify approach would have worked
> perfectly. However since MODULE_NAME_LEN translates to an expression
> (64 - sizeof(unsigned long)), which the preprocessor cannot evaluate,
> we will need another approach. Building the format strings at run time
> might be messier than we'd like. Alternatively we could just go the
> simple route and simply be a bit more aggressive on the upper bound
> for the format width; though the size of long varies on different
> architectures, afaik the max size it could ever be on any arch is 8
> bytes, so perhaps 64 - 8 = 56 (then - 1 to make room for \0) might be
> an appropriate field width. This would deserve a comment as well.

I think something like that would be good, along with:

  BUILD_BUG_ON(MODULE_NAME_LEN < 56);

and a comment explaining why.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1362155 — Re: livepatch: reuse module loader code to write relocations

On Mon, 21 Mar 2016, Jessica Yu wrote:

> Yes, this is a concern and I'm not sure what the best way to fix it
> is. If both MODULE_NAME_LEN and KSYM_NAME_LEN were straight up
> constants, then I think Josh's stringify approach would have worked
> perfectly. However since MODULE_NAME_LEN translates to an expression
> (64 - sizeof(unsigned long)), which the preprocessor cannot evaluate,
> we will need another approach. Building the format strings at run time
> might be messier than we'd like. Alternatively we could just go the
> simple route and simply be a bit more aggressive on the upper bound
> for the format width; though the size of long varies on different
> architectures, afaik the max size it could ever be on any arch is 8
> bytes, so perhaps 64 - 8 = 56 (then - 1 to make room for \0) might be
> an appropriate field width. This would deserve a comment as well.

So how about actually modifying MAX_PARAM_PREFIX_LEN so 
that it's actually properly evaluable at preprocessing time, 
i.e. something along the lines of

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 52666d9..954dae9 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -14,7 +14,7 @@
 #endif
 
 /* Chosen so that structs with an unsigned long line up. */
-#define MAX_PARAM_PREFIX_LEN (64 - sizeof(unsigned long))
+#define MAX_PARAM_PREFIX_LEN (64 - __SIZEOF_LONG__)
 
 #ifdef MODULE
 #define __MODULE_INFO(tag, name, info)					  \

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1362166 — Re: livepatch: reuse module loader code to write relocations

On Mon, Mar 21, 2016 at 10:16:17PM +0100, Jiri Kosina wrote:
> On Mon, 21 Mar 2016, Jessica Yu wrote:
> 
> > Yes, this is a concern and I'm not sure what the best way to fix it
> > is. If both MODULE_NAME_LEN and KSYM_NAME_LEN were straight up
> > constants, then I think Josh's stringify approach would have worked
> > perfectly. However since MODULE_NAME_LEN translates to an expression
> > (64 - sizeof(unsigned long)), which the preprocessor cannot evaluate,
> > we will need another approach. Building the format strings at run time
> > might be messier than we'd like. Alternatively we could just go the
> > simple route and simply be a bit more aggressive on the upper bound
> > for the format width; though the size of long varies on different
> > architectures, afaik the max size it could ever be on any arch is 8
> > bytes, so perhaps 64 - 8 = 56 (then - 1 to make room for \0) might be
> > an appropriate field width. This would deserve a comment as well.
> 
> So how about actually modifying MAX_PARAM_PREFIX_LEN so 
> that it's actually properly evaluable at preprocessing time, 
> i.e. something along the lines of
> 
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 52666d9..954dae9 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -14,7 +14,7 @@
>  #endif
>  
>  /* Chosen so that structs with an unsigned long line up. */
> -#define MAX_PARAM_PREFIX_LEN (64 - sizeof(unsigned long))
> +#define MAX_PARAM_PREFIX_LEN (64 - __SIZEOF_LONG__)
>  
>  #ifdef MODULE
>  #define __MODULE_INFO(tag, name, info)					  \

According to my test that still results in the literal value of
"(64 - 8)".

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1362174 — Re: livepatch: reuse module loader code to write relocations

On Mon, 21 Mar 2016, Josh Poimboeuf wrote:

> According to my test that still results in the literal value of
> "(64 - 8)".

Alright. But we should be able to special-case it with a two #if checks on 
the __SIZEOF_LONG__ value and BUILD_BUG_ON() when __SIZEOF_LONG__ is not 
of one of the ususal sizes.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1362917 — Re: livepatch: reuse module loader code to write relocations

+++ Jiri Kosina [21/03/16 23:02 +0100]:
>On Mon, 21 Mar 2016, Josh Poimboeuf wrote:
>
>> According to my test that still results in the literal value of
>> "(64 - 8)".
>
>Alright. But we should be able to special-case it with a two #if checks on
>the __SIZEOF_LONG__ value and BUILD_BUG_ON() when __SIZEOF_LONG__ is not
>of one of the ususal sizes.
>

And while we're at it, we might as well add a BUILD_BUG_ON check for
KSYM_NAME_LEN too, since we are also hard coding that field width, and
we'd like to be alerted if that value ever changes.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web