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

Groups > linux.kernel > #1260841 > unrolled thread

[PATCH] livepatch: old_name.number scheme in livepatch sysfs directory

Started byChris J Arges <chris.j.arges@canonical.com>
First post2015-11-02 19:00 +0100
Last post2015-11-10 10:10 +0100
Articles 3 on this page of 43 — 6 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] livepatch: old_name.number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-02 19:00 +0100
    Re: livepatch: old_name.number scheme in livepatch sysfs directory Jessica Yu <jeyu@redhat.com> - 2015-11-02 20:20 +0100
    Re: [PATCH] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-02 21:00 +0100
      Re: [PATCH] livepatch: old_name.number scheme in livepatch sysfs  directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-02 21:20 +0100
        Re: [PATCH] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-02 21:40 +0100
          [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-03 00:10 +0100
            Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-03 11:00 +0100
              Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 16:10 +0100
            Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-03 12:00 +0100
              Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Petr Mladek <pmladek@suse.com> - 2015-11-03 13:50 +0100
                Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 16:10 +0100
                Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Jiri Kosina <jikos@kernel.org> - 2015-11-03 21:00 +0100
                  Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 21:10 +0100
                    [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp_func Chris J Arges <chris.j.arges@canonical.com> - 2015-11-11 17:30 +0100
                      Re: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field  to klp_func Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-11 18:50 +0100
                      Re: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field  to klp_func Miroslav Benes <mbenes@suse.cz> - 2015-11-12 11:30 +0100
                    [PATCH 3/3 v4] livepatch: old_name,number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-11 17:40 +0100
                      Re: [PATCH 3/3 v4] livepatch: old_name,number scheme in livepatch  sysfs directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-11 19:10 +0100
                    Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Chris J Arges <chris.j.arges@canonical.com> - 2015-11-11 17:40 +0100
                    [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field to klp_reloc Chris J Arges <chris.j.arges@canonical.com> - 2015-11-11 17:40 +0100
                      Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-11 19:00 +0100
                        Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Petr Mladek <pmladek@suse.com> - 2015-11-12 15:40 +0100
                          Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 20:20 +0100
                            Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Petr Mladek <pmladek@suse.com> - 2015-11-13 15:00 +0100
                              Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-13 18:00 +0100
                      Re: [PATCH 2/3 v4] livepatch: add old_sympos as disambiguator field  to klp_reloc Miroslav Benes <mbenes@suse.cz> - 2015-11-12 11:30 +0100
              Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 16:00 +0100
                Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-03 17:20 +0100
                  Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-03 18:00 +0100
                    Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-03 21:50 +0100
                    Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-04 11:00 +0100
                      Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-04 17:10 +0100
                        Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-04 17:20 +0100
                        Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-05 16:20 +0100
                          Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-05 17:00 +0100
                            Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs  directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-05 17:10 +0100
                            [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-09 17:20 +0100
                              Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-09 22:00 +0100
                                Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs  directory Chris J Arges <chris.j.arges@canonical.com> - 2015-11-10 00:10 +0100
                                  Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-10 06:00 +0100
                                    Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-10 09:50 +0100
                                      Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs  directory Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-10 14:50 +0100
                              Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs  directory Miroslav Benes <mbenes@suse.cz> - 2015-11-10 10:10 +0100

Page 3 of 3 — ← Prev page 1 2 [3]

#1266330 — Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory

FromMiroslav Benes <mbenes@suse.cz>
Date2015-11-10 09:50 +0100
SubjectRe: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory
Message-ID<qtcMO-2IH-13@gated-at.bofh.it>
In reply to#1266233
On Mon, 9 Nov 2015, Josh Poimboeuf wrote:

> On Mon, Nov 09, 2015 at 05:01:18PM -0600, Chris J Arges wrote:
> > On 11/09/2015 02:56 PM, Josh Poimboeuf wrote:
> > > I'd recommend splitting this up into two separate patches:
> > > 
> > > 1. introduce old_sympos
> > > 2. change the sysfs interface
> > > 
> > > On Mon, Nov 09, 2015 at 10:16:05AM -0600, Chris J Arges wrote:
> > >> In cases of duplicate symbols in vmlinux, old_sympos will be used to
> > >> disambiguate instead of old_addr. Normally old_sympos will be 0, and
> > >> default to only returning the first found instance of that symbol. If an
> > >> incorrect symbol position is specified then livepatching will fail.
> > > 
> > > In the case of old_sympos == 0, instead of just returning the first
> > > symbol it finds, I think it should ensure that the symbol is unique.  As
> > > Miroslav suggested:
> > > 
> > >   0 - default, preserve more or less current behaviour. If the symbol is 
> > >       unique there is no problem. If it is not the patching would fail.
> > >   1, 2, ... - occurrence of the symbol in kallsyms.
> > >   
> > >   The advantage is that if the user does not care and is certain that the 
> > >   symbol is unique he doesn't have to do anything. If the symbol is not 
> > >   unique he still has means how to solve it.
> > > 
> > 
> > So one part that will be confusing here is as follows.
> > 
> > If '0' is specified for old_sympos, should the symbol be 'func_name,0'
> > or 'func_name,1' provided we have a unique symbol? We could also default
> > to 'what the user provides', but this seems odd.
> 
> I don't feel strongly either way, but I think using the same number the
> user provides is fine, since it makes the sysfs interface consistent
> with the old_sympos usage.

I think it should be func_name,1 even if '0' is specified and the symbol 
is unique. Because if we say that 1, 2, ... is the occurrence of the 
symbol in kallsyms it should stay that way everywhere. Hence for 
old_sympos == 0 it is func_name,1 in sysfs; for 1 it is still func_name,1; 
for 2 it is func_name,2 and so on.

And I'd add this to sysfs documentation.

> > Another option would be to use no postfix when 0 is given, and only
> > introduce the ',n' postfix if old_sympos is > 0.
> 
> IMO always having a suffix is good, as it makes parsing less surprising
> and less error-prone.

Agreed.

> > >>  static int klp_write_object_relocations(struct module *pmod,
> > >> @@ -307,7 +318,7 @@ static int klp_write_object_relocations(struct module *pmod,
> > >>  			else
> > >>  				ret = klp_find_object_symbol(obj->mod->name,
> > >>  							     reloc->name,
> > >> -							     &reloc->val);
> > >> +							     &reloc->val, 0);
> > > 
> > > I think it would be a good idea to also add old_sympos to klp_reloc so
> > > the relocation code is consistent with the klp_func symbol addressing.
> > >
> > 
> > So you are thinking as an optional external field as well? I'll have to
> > look at this a bit more but makes sense to me.
> 
> Yeah, the semantics would be the same as klp_func.old_sympos.  We could
> add a new klp_reloc.sympos and make klp_reloc.val a private field.

Agreed as well.

Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1266513 — Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2015-11-10 14:50 +0100
SubjectRe: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory
Message-ID<qtht9-5IV-29@gated-at.bofh.it>
In reply to#1266330
On Tue, Nov 10, 2015 at 09:49:09AM +0100, Miroslav Benes wrote:
> On Mon, 9 Nov 2015, Josh Poimboeuf wrote:
> 
> > On Mon, Nov 09, 2015 at 05:01:18PM -0600, Chris J Arges wrote:
> > > On 11/09/2015 02:56 PM, Josh Poimboeuf wrote:
> > > > I'd recommend splitting this up into two separate patches:
> > > > 
> > > > 1. introduce old_sympos
> > > > 2. change the sysfs interface
> > > > 
> > > > On Mon, Nov 09, 2015 at 10:16:05AM -0600, Chris J Arges wrote:
> > > >> In cases of duplicate symbols in vmlinux, old_sympos will be used to
> > > >> disambiguate instead of old_addr. Normally old_sympos will be 0, and
> > > >> default to only returning the first found instance of that symbol. If an
> > > >> incorrect symbol position is specified then livepatching will fail.
> > > > 
> > > > In the case of old_sympos == 0, instead of just returning the first
> > > > symbol it finds, I think it should ensure that the symbol is unique.  As
> > > > Miroslav suggested:
> > > > 
> > > >   0 - default, preserve more or less current behaviour. If the symbol is 
> > > >       unique there is no problem. If it is not the patching would fail.
> > > >   1, 2, ... - occurrence of the symbol in kallsyms.
> > > >   
> > > >   The advantage is that if the user does not care and is certain that the 
> > > >   symbol is unique he doesn't have to do anything. If the symbol is not 
> > > >   unique he still has means how to solve it.
> > > > 
> > > 
> > > So one part that will be confusing here is as follows.
> > > 
> > > If '0' is specified for old_sympos, should the symbol be 'func_name,0'
> > > or 'func_name,1' provided we have a unique symbol? We could also default
> > > to 'what the user provides', but this seems odd.
> > 
> > I don't feel strongly either way, but I think using the same number the
> > user provides is fine, since it makes the sysfs interface consistent
> > with the old_sympos usage.
> 
> I think it should be func_name,1 even if '0' is specified and the symbol 
> is unique. Because if we say that 1, 2, ... is the occurrence of the 
> symbol in kallsyms it should stay that way everywhere. Hence for 
> old_sympos == 0 it is func_name,1 in sysfs; for 1 it is still func_name,1; 
> for 2 it is func_name,2 and so on.
> 
> And I'd add this to sysfs documentation.

That makes sense, sounds fine to me.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1266339 — Re: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory

FromMiroslav Benes <mbenes@suse.cz>
Date2015-11-10 10:10 +0100
SubjectRe: [PATCH v3] livepatch: old_name,number scheme in livepatch sysfs directory
Message-ID<qtd69-35q-5@gated-at.bofh.it>
In reply to#1265788
On Mon, 9 Nov 2015, Chris J Arges wrote:

> In cases of duplicate symbols in vmlinux, old_sympos will be used to
> disambiguate instead of old_addr. Normally old_sympos will be 0, and
> default to only returning the first found instance of that symbol. If an
> incorrect symbol position is specified then livepatching will fail.
> Finally, old_addr is now an internal structure element and not to be
> specified by the user.

Hi,

Josh has already mentioned it, but in this case '0' would be same as '1'. 
'0' should fail if the symbol is ambiguous.

Few more things follow, but Josh pointed out the issues.

[...]

> @@ -239,20 +251,19 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
>  static int klp_find_verify_func_addr(struct klp_object *obj,
>  				     struct klp_func *func)
>  {
> +	int sympos = 0;
>  	int ret;
>  
> -#if defined(CONFIG_RANDOMIZE_BASE)
> -	/* If KASLR has been enabled, adjust old_addr accordingly */
> -	if (kaslr_enabled() && func->old_addr)
> -		func->old_addr += kaslr_offset();
> -#endif
> +	if (func->old_sympos && !klp_is_module(obj))
> +		sympos = func->old_sympos;

We need to deal with ambiguity in modules as well. 

Also, maybe the function could be renamed, because there would be no 
verification here in the future.

>  static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> @@ -732,8 +743,7 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>  	INIT_LIST_HEAD(&func->stack_node);
>  	func->state = KLP_DISABLED;
>  
> -	return kobject_init_and_add(&func->kobj, &klp_ktype_func,
> -				    &obj->kobj, "%s", func->old_name);
> +	return 0;
>  }
>  
>  /* parts of the initialization that is done only when the object is loaded */
> @@ -755,6 +765,18 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  			return ret;
>  	}
>  
> +	/*
> +	 * for each function initialize and add, old_sympos will be already
> +	 * verified at this point
> +	 */
> +	klp_for_each_func(obj, func) {
> +		ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
> +				    &obj->kobj, "%s,%lu", func->old_name,
> +				    func->old_sympos ? func->old_sympos : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }

There is a problem with error handling in klp_init_object() after the 
change. 

free:
        klp_free_funcs_limited(obj, func);
        kobject_put(&obj->kobj);
        return ret;

This snippet ensures that all already created sysfs func entries are 
destroyed. 'func' is the function which klp_init_func() failed for (or 
'{}' if nothing failed). When you move kobject_init_and_add() with the 
loop to klp_init_object_loaded(), we do not know where the exact problem 
was in klp_init_object(). So I agree with Josh that it can stay in 
klp_init_func(). old_sympos is defined and if the following 
klp_find_verify_func_addr() fails (for example when old_sympos is '0' and 
the symbol is not unique) we deal with it here in klp_init_object() 
correctly.

Thanks,
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 3 of 3 — ← Prev page 1 2 [3]

Back to top | Article view | linux.kernel

csiph-web