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

On Mon, Nov 09, 2015 at 05:01:18PM -0600, Chris J Arges wrote:

On 11/09/2015 02:56 PM, Josh Poimboeuf wrote:

quoted

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:

quoted

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.

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.

quoted

quoted

 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.

-- 
Josh