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

[Josh Poimboeuf]

On Thu, Nov 05, 2015 at 04:18:12PM +0100, Miroslav Benes wrote:
> On Wed, 4 Nov 2015, Josh Poimboeuf wrote:
> 
> > On Wed, Nov 04, 2015 at 10:52:52AM +0100, Miroslav Benes wrote:
> > > On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> > > > > 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.
> > 
> > I see your point.  This approach only describes what is patched now, but
> > it doesn't describe what *will* be patched.  Ideally we could find a way
> > to describe both.
> > 
> > Speaking of harsh changes, here's an idea.
> 
> Which is the very same you proposed last year when I tried to persuade you 
> to get rid off old_addr and stuff. I called it crazy I remember :D. So 
> here we are again...

Ah, I knew I had entertained the idea before, but I forgot we discussed
it.  It is indeed a little crazy.  But this is live patching after all
;-)

> > What if we require the patch author to supply the value of 'n' instead
> > of supplying the symbol address?  We could get rid of 'old_addr' as an
> > input in klp_func and and replace it with 'old_sympos' which has the
> > value of 'n'.  Or alternatively we could require old_name to be of the
> > format "func,n".
> > 
> > That would uniquely identify each patched function, even _before_ the
> > object is loaded.
> 
> I find it reasonable and we should try it. I think that old_sympos should 
> have this semantics
> 
> 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.
> 
> Does it make sense?

Sounds good!

> > It would also fix another big problem we have today, where there's no
> > way to disambiguate duplicate symbols in modules, for both function
> > addresses and for relocs.
> 
> True.
> 
> > It would simplify the code in other places as well: no special handling
> > for kASLR, no need for klp_verify_vmlinux_symbol() vs
> > klp_find_object_symbol().
> 
> Which would be great.
> 
> > A drawback is that it requires the patch author to do a little more due
> > diligence when filling out klp_func.  But we already require them to be
> > careful.
> 
> Yes, I don't think this should be a problem.
> 
> > Thoughts?
> 
> Yup, we should try it. I suppose that the order of the symbols in kallsyms 
> table is stable for once-built kernel. It is the order of the symbols in 
> the object files, isn't it? And since each livepatch module is built 
> against the specific kernel there should be no issues with this.

The order of the symbols in an object's symbol table does appear to be
the same as the order in kallsyms (per-object).  So yeah, let's try it.

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