Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755494AbbBLKpP (ORCPT ); Thu, 12 Feb 2015 05:45:15 -0500 Received: from cantor2.suse.de ([195.135.220.15]:51822 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755349AbbBLKpN (ORCPT ); Thu, 12 Feb 2015 05:45:13 -0500 Date: Thu, 12 Feb 2015 11:45:09 +0100 (CET) From: Miroslav Benes To: Josh Poimboeuf cc: Seth Jennings , Jiri Kosina , Vojtech Pavlik , Masami Hiramatsu , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 6/9] livepatch: create per-task consistency model In-Reply-To: <20150211201921.GA4311@treble.redhat.com> Message-ID: References: <2c3d1e685dae5cccc2dfdb1b24c241b2f1c89348.1423499826.git.jpoimboe@redhat.com> <20150211201921.GA4311@treble.redhat.com> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3070 Lines: 88 On Wed, 11 Feb 2015, Josh Poimboeuf wrote: > On Wed, Feb 11, 2015 at 11:21:51AM +0100, Miroslav Benes wrote: > > > > On Mon, 9 Feb 2015, Josh Poimboeuf wrote: > > > > [...] > > > > > @@ -38,14 +39,34 @@ static void notrace klp_ftrace_handler(unsigned long ip, > > > ops = container_of(fops, struct klp_ops, fops); > > > > > > rcu_read_lock(); > > > + > > > func = list_first_or_null_rcu(&ops->func_stack, struct klp_func, > > > stack_node); > > > - rcu_read_unlock(); > > > > > > if (WARN_ON_ONCE(!func)) > > > - return; > > > + goto unlock; > > > + > > > + if (unlikely(func->transition)) { > > > + /* corresponding smp_wmb() is in klp_init_transition() */ > > > + smp_rmb(); > > > + > > > + if (current->klp_universe == KLP_UNIVERSE_OLD) { > > > + /* > > > + * Use the previously patched version of the function. > > > + * If no previous patches exist, use the original > > > + * function. > > > + */ > > > + func = list_entry_rcu(func->stack_node.next, > > > + struct klp_func, stack_node); > > > + > > > + if (&func->stack_node == &ops->func_stack) > > > + goto unlock; > > > + } > > > + } > > > > > > klp_arch_set_pc(regs, (unsigned long)func->new_func); > > > +unlock: > > > + rcu_read_unlock(); > > > } > > > > I decided to understand the code more before answering the email about the > > race and found another problem. I think. > > > > Imagine we patched some function foo() with foo_1() from patch_1 and now > > we'd like to patch it again with foo_2() in patch_2. __klp_enable_patch > > calls klp_init_transition which sets klp_universe for all processes to > > KLP_UNIVERSE_OLD and marks the foo_2() for transition (it is gonna be 1). > > Then __klp_enable_patch adds foo_2() to the RCU-protected list for foo(). > > BUT what if somebody calls foo() right between klp_init_transition and > > the loop in __klp_enable_patch? The ftrace handler first returns the > > first entry in the list which is foo_1() (foo_2() is still not present), > > then it checks for func->transition. It is 1. > > No, actually foo_1()'s func->transition will be 0. Only foo_2()'s > func->transition will be 1. Ah, you're right in both cases. Sorry for the noise. Miroslav > > > It checks for > > current->klp_universe which is KLP_UNIVERSE_OLD and so the next entry is > > retrieved. There is no such and therefore foo() is called. This is > > obviously wrong because foo_1() was expected. > > > > Everything would work fine if one would call foo() before > > klp_start_transition and after the loop in __klp_enable_patch. The > > solution might be to move the setting of func->transition to > > klp_start_transition, but this could break something different. I don't > > know yet. > > > > Am I wrong? > > > > Miroslav > > -- > Josh > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/