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

On Tue, 3 Nov 2015, Josh Poimboeuf wrote:

On Tue, Nov 03, 2015 at 05:09:48PM +0100, Miroslav Benes wrote:

quoted

On Tue, 3 Nov 2015, Josh Poimboeuf wrote:

quoted

On Tue, Nov 03, 2015 at 11:52:08AM +0100, Miroslav Benes wrote:

quoted

There is a problem which I missed before. klp_init_func() is called before 
klp_find_verify_func_addr() in klp_init_object(). This means that 
func->old_addr is either not verified yet or worse it is still 0. This 
means that klp_get_func_pos_callback() never returns 1 and is thus called 
on each symbol. So if you for example patched cmdline_proc_show the 
resulting directory in sysfs would be called cmdline_proc_show,1 because 
addr is never matched. Had old_addr been specified the name would have 
been probably correct, but not for sure.

This should be fixed as well.

Even worse, klp_init_func() can be called even if the object hasn't been
loaded.  In that case there's no way to know what the value of n is, and
therefore no way to reliably create the sysfs entry.

Ah, right.

quoted

Should we create "func,n" in klp_init_object_loaded() instead of
klp_init_func()?

So that the function entries in sysfs would be created only when the 
object is loaded? Well, why not, but in that case it could easily confuse 
the user.

Maybe, but I think it would be fine if we document it.  It should only
be relied on by tools, anyway.

Agreed.

quoted

Object entry would be empty for not loaded object. I would not 
dare to propose to remove such object entries. It would make things worse. 

Why would removing an empty object entry make things worse?

I think it all comes down to a question whether the sysfs entries say what 
a patch is capable to patch or what this patch is currently patching in 
the system. I am inclined to the former so the removal would make me 
nervous. But I am not against the second approach. We are still in testing 
mode as far as sysfs is concerned so we can try even harsh changes and see 
how it's gonna go.

quoted

So maybe we could introduce an attribute in sysfs object entry which would 
say if the object is loaded or not. Or something like that.

Hm, I'm not sure I see how this would help.

Hopefully I cleared this up with the above.

Miroslav