From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756332AbdHYLxP (ORCPT ); Fri, 25 Aug 2017 07:53:15 -0400 Received: from mx2.suse.de ([195.135.220.15]:49037 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754782AbdHYLxO (ORCPT ); Fri, 25 Aug 2017 07:53:14 -0400 Date: Fri, 25 Aug 2017 13:53:12 +0200 From: Petr Mladek To: Jason Baron Cc: linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, jpoimboe@redhat.com, jeyu@kernel.org, jikos@kernel.org, mbenes@suse.cz Subject: Re: [PATCH 2/3] livepatch: add atomic replace Message-ID: <20170825115312.GI30286@pathway.suse.cz> References: <9735365ce9f811a50628fb4eb20d48b76a08baa0.1500482335.git.jbaron@akamai.com> <20170825092623.GG30286@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170825092623.GG30286@pathway.suse.cz> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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