Re: [PATCH 2/3] livepatch: add atomic replace

[Petr Mladek <pmladek@suse.com>]

On Fri 2017-08-25 11:26:23, Petr Mladek wrote:
> On Wed 2017-07-19 13:18:06, Jason Baron wrote:
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index e63f478..bf353da 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -602,6 +605,118 @@ static void klp_free_patch(struct klp_patch *patch)
> > +
> > +static int klp_init_patch_no_ops(struct klp_patch *patch)
> > +{
> > +	struct klp_object *obj, *prev_obj, *new_obj;
> > +	struct klp_func *prev_func, *func;
> > +	struct klp_func_no_op *new;
> > +	struct klp_patch *prev_patch;
> > +	struct obj_iter o_iter, prev_o_iter;
> > +	struct func_iter prev_f_iter, f_iter;
> > +	bool found, mod;
> > +
> > +	if (patch->list.prev == &klp_patches)
> > +		return 0;
> > +
> > +	prev_patch = list_prev_entry(patch, list);
> > +	klp_for_each_object(prev_patch, prev_obj, &prev_o_iter) {
> > +		if (!klp_is_object_loaded(prev_obj))
> > +			continue;
> > +
> > +		klp_for_each_func(prev_obj, prev_func, &prev_f_iter) {
> > +			found = false;
> > +			klp_for_each_object(patch, obj, &o_iter) {
> > +				klp_for_each_func(obj, func, &f_iter) {
> > +					if ((strcmp(prev_func->old_name,
> > +						    func->old_name) == 0) &&
> > +						(prev_func->old_sympos ==
> > +							func->old_sympos)) {
> > +						found = true;
> > +						break;
> > +					}
> > +				}
> > +				if (found)
> > +					break;
> > +			}
> > +			if (found)
> > +				continue;
> > +
> > +			new = kmalloc(sizeof(*new), GFP_KERNEL);
> > +			if (!new)
> > +				return -ENOMEM;
> > +			new->orig_func = *prev_func;
> > +			new->orig_func.old_name = prev_func->old_name;
> > +			new->orig_func.new_func = NULL;
> 
> This is OK if the operation replaces all older patches. Otherwise,
> you would need to store here the address from the patch down the stack.
> 
> 
> > +			new->orig_func.old_sympos = prev_func->old_sympos;
> > +			new->orig_func.immediate = prev_func->immediate;
> > +			new->orig_func.old_addr = prev_func->old_addr;
> 
> Hmm, this should be
> 
> 			new->orig_func.old_addr = prev_func->new_func;
> 
> klp_check_stack_func() should check the address of the old function
> that is currently running. It is the variant of the function that
> is on top of stack.
>
> I think that we actually have bug in the current livepatch code
> because old_addr always points to the original function!!!
> I am going to look at it.

I take this back. old_addr and old_size points to the original
(unpatched) code. The values are the same in all patches
for the same function. klp_check_stack_func() use this information
only when there is only one patch on the func_stack. Otherwise,
it checks new_func, new_size from the previous patch on the stack.

By other words, you assign old_addr and old_size correctly.
Also the livepatch handle this correctly. Ufff :-)

Best Regards,
Petr