On Tue, 12 Oct 2021 14:24:43 +0200 (CEST) Miroslav Benes <mbenes@xxxxxxx> wrote: > > +++ b/kernel/livepatch/patch.c > > @@ -52,11 +52,6 @@ static void notrace klp_ftrace_handler(unsigned long ip, > > bit = ftrace_test_recursion_trylock(ip, parent_ip); > > if (WARN_ON_ONCE(bit < 0)) > > return; > > - /* > > - * A variant of synchronize_rcu() is used to allow patching functions > > - * where RCU is not watching, see klp_synchronize_transition(). > > - */ > > - preempt_disable_notrace(); > > > > func = list_first_or_null_rcu(&ops->func_stack, struct klp_func, > > stack_node); > > @@ -120,7 +115,6 @@ static void notrace klp_ftrace_handler(unsigned long ip, > > klp_arch_set_pc(fregs, (unsigned long)func->new_func); > > > > unlock: > > - preempt_enable_notrace(); > > ftrace_test_recursion_unlock(bit); > > } > > I don't like this change much. We have preempt_disable there not because > of ftrace_test_recursion, but because of RCU. ftrace_test_recursion was > added later. Yes, it would work with the change, but it would also hide > things which should not be hidden in my opinion. Agreed, but I believe the change is fine, but requires a nice comment to explain what you said above. Thus, before the "ftrace_test_recursion_trylock()" we need: /* * The ftrace_test_recursion_trylock() will disable preemption, * which is required for the variant of synchronize_rcu() that is * used to allow patching functions where RCU is not watching. * See klp_synchronize_transition() for more details. */ -- Steve
