Re: [PATCHv4 2/3] kernel: add support for live patching

[Miroslav Benes]

On Wed, 26 Nov 2014, Josh Poimboeuf wrote:

> Hi Miroslav,
> 
> Just addressing one of your comments below.  I'll let Seth respond to
> the others :-)
> 
> On Wed, Nov 26, 2014 at 03:19:17PM +0100, Miroslav Benes wrote:
> > > +/**
> > > + * struct klp_func - function structure for live patching
> > > + * @old_name:	name of the function to be patched
> > > + * @new_func:	pointer to the patched function code
> > > + * @old_addr:	a hint conveying at what address the old function
> > > + *		can be found (optional, vmlinux patches only)
> > > + */
> > > +struct klp_func {
> > > +	/* external */
> > > +	const char *old_name;
> > > +	void *new_func;
> > > +	/*
> > > +	 * The old_addr field is optional and can be used to resolve
> > > +	 * duplicate symbol names in the vmlinux object.  If this
> > > +	 * information is not present, the symbol is located by name
> > > +	 * with kallsyms. If the name is not unique and old_addr is
> > > +	 * not provided, the patch application fails as there is no
> > > +	 * way to resolve the ambiguity.
> > > +	 */
> > > +	unsigned long old_addr;
> > 
> > I wonder if we really need old_addr as an external field. I assume that 
> > userspace tool in kpatch use it as a "hint" for kernel part and thus 
> > kallsyms is not needed there (and it solves ambiguity problem as well). 
> > But I am not sure if it is gonna be the same in upstream. When kernel is 
> > randomized (CONFIG_RANDOMIZE_BASE is set to 'y', though default is 'n') 
> > old_addr is not usable (and we throw it away in the code). Without 
> > old_addr being set by the user we could spare some of code (calls to 
> > klp_verify_vmlinux_symbol and such). 
> 
> Even with CONFIG_RANDOMIZE_BASE, the function offsets will be the same
> regardless of the base address.  So we could still use old_addr to
> determine the offset.
> 
> > So the question is whether future userspace tool in upstream would need it 
> > and would use it. Please note that I do not mean it as a kpatch or kgraft 
> > way to do things, I'm just not sure about old_addr being "public" and want 
> > do discuss the options.
> > 
> > The ambiguity of symbols was discussed in some other thread in lkml in 
> > october (I guess) with no conclusion IIRC...
> 
> We need to resolve ambiguity somehow, and old_addr is a way to do that.
> Do you have any other ideas?

Unfortunately I don't.

But similarly we don't deal with ambiguity in modules either. And it is 
(at least theoretically) possible. Two static functions of the same name 
in two different .c files which the final module is linked from. You have 
to use kallsyms and it would get confused. Maybe this sounds odd but it 
could happen. 

Thus the old_addr value is not general protection (as modules are still 
affected) and it is questionable whether the user should use it.

I do not have strong opinion on this and if no one else shares my 
thoughts, I am not against.

Mira
--
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/