* static_branch_enable() does not work from a __init function?
@ 2020-12-16 3:54 Dexuan Cui
2020-12-16 9:26 ` Peter Zijlstra
0 siblings, 1 reply; 20+ messages in thread
From: Dexuan Cui @ 2020-12-16 3:54 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Daniel Bristot de Oliveira, kvm; +Cc: linux-kernel
Hi,
The below init_module() prints "foo: false". This is strange since
static_branch_enable() is called before the static_branch_unlikely().
This strange behavior happens to v5.10 and an old v5.4 kernel.
If I remove the "__init" marker from the init_module() function, then
I get the expected output of "foo: true"! I guess here I'm missing
something with Static Keys?
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/jump_label.h>
static DEFINE_STATIC_KEY_FALSE(enable_foo);
int __init init_module(void)
{
static_branch_enable(&enable_foo);
if (static_branch_unlikely(&enable_foo))
printk("foo: true\n");
else
printk("foo: false\n");
return 0;
}
void cleanup_module(void)
{
static_branch_disable(&enable_foo);
}
MODULE_LICENSE("GPL");
PS, I originally found: in arch/x86/kvm/vmx/vmx.c: vmx_init(), it looks
like the line "static_branch_enable(&enable_evmcs);" does not take effect
in a v5.4-based kernel, but does take effect in the v5.10 kernel in the
same x86-64 virtual machine on Hyper-V, so I made the above test module
to test static_branch_enable(), and found that static_branch_enable() in
the test module does not work with both v5.10 and my v5.4 kernel, if the
__init marker is used.
Thanks,
-- Dexuan
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: static_branch_enable() does not work from a __init function? 2020-12-16 3:54 static_branch_enable() does not work from a __init function? Dexuan Cui @ 2020-12-16 9:26 ` Peter Zijlstra 2020-12-16 10:59 ` Peter Zijlstra ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Peter Zijlstra @ 2020-12-16 9:26 UTC (permalink / raw) To: Dexuan Cui Cc: Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, jeyu, Josh Poimboeuf On Wed, Dec 16, 2020 at 03:54:29AM +0000, Dexuan Cui wrote: > Hi, > The below init_module() prints "foo: false". This is strange since > static_branch_enable() is called before the static_branch_unlikely(). > This strange behavior happens to v5.10 and an old v5.4 kernel. > > If I remove the "__init" marker from the init_module() function, then > I get the expected output of "foo: true"! I guess here I'm missing > something with Static Keys? *groan*... I think this is because __init is ran with MODULE_STATE_COMING, we only switch to MODULE_STATE_LIVE later. Let me see if there's a sane way to untangle that. > #include <linux/module.h> > #include <linux/kernel.h> > #include <linux/jump_label.h> > > static DEFINE_STATIC_KEY_FALSE(enable_foo); > > int __init init_module(void) > { > static_branch_enable(&enable_foo); > > if (static_branch_unlikely(&enable_foo)) > printk("foo: true\n"); > else > printk("foo: false\n"); > > return 0; > } > > void cleanup_module(void) > { > static_branch_disable(&enable_foo); > } > > MODULE_LICENSE("GPL"); > > > PS, I originally found: in arch/x86/kvm/vmx/vmx.c: vmx_init(), it looks > like the line "static_branch_enable(&enable_evmcs);" does not take effect > in a v5.4-based kernel, but does take effect in the v5.10 kernel in the > same x86-64 virtual machine on Hyper-V, so I made the above test module > to test static_branch_enable(), and found that static_branch_enable() in > the test module does not work with both v5.10 and my v5.4 kernel, if the > __init marker is used. > > Thanks, > -- Dexuan > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: static_branch_enable() does not work from a __init function? 2020-12-16 9:26 ` Peter Zijlstra @ 2020-12-16 10:59 ` Peter Zijlstra 2020-12-16 13:30 ` [RFC][PATCH] jump_label/static_call: Add MAINTAINERS Peter Zijlstra ` (2 more replies) 2020-12-16 11:55 ` Jessica Yu 2020-12-16 12:38 ` Jessica Yu 2 siblings, 3 replies; 20+ messages in thread From: Peter Zijlstra @ 2020-12-16 10:59 UTC (permalink / raw) To: Dexuan Cui Cc: Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, jeyu, Josh Poimboeuf, ardb On Wed, Dec 16, 2020 at 10:26:49AM +0100, Peter Zijlstra wrote: > On Wed, Dec 16, 2020 at 03:54:29AM +0000, Dexuan Cui wrote: > > Hi, > > The below init_module() prints "foo: false". This is strange since > > static_branch_enable() is called before the static_branch_unlikely(). > > This strange behavior happens to v5.10 and an old v5.4 kernel. > > > > If I remove the "__init" marker from the init_module() function, then > > I get the expected output of "foo: true"! I guess here I'm missing > > something with Static Keys? > > *groan*... I think this is because __init is ran with > MODULE_STATE_COMING, we only switch to MODULE_STATE_LIVE later. > > Let me see if there's a sane way to untangle that. > > > #include <linux/module.h> > > #include <linux/kernel.h> > > #include <linux/jump_label.h> > > > > static DEFINE_STATIC_KEY_FALSE(enable_foo); > > > > int __init init_module(void) > > { > > static_branch_enable(&enable_foo); > > > > if (static_branch_unlikely(&enable_foo)) > > printk("foo: true\n"); > > else > > printk("foo: false\n"); > > > > return 0; > > } > > > > void cleanup_module(void) > > { > > static_branch_disable(&enable_foo); > > } > > > > MODULE_LICENSE("GPL"); > > > > > > PS, I originally found: in arch/x86/kvm/vmx/vmx.c: vmx_init(), it looks > > like the line "static_branch_enable(&enable_evmcs);" does not take effect > > in a v5.4-based kernel, but does take effect in the v5.10 kernel in the > > same x86-64 virtual machine on Hyper-V, so I made the above test module > > to test static_branch_enable(), and found that static_branch_enable() in > > the test module does not work with both v5.10 and my v5.4 kernel, if the > > __init marker is used. So I think the reason your above module doesn't work, while the one in vmx_init() does work (for 5.10) should be fixed by the completely untested below. I've no clue about 5.4 and no desire to investigate. That's what distro people are for. Can you verify? --- diff --git a/kernel/jump_label.c b/kernel/jump_label.c index 015ef903ce8c..c6a39d662935 100644 --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -793,6 +793,7 @@ int jump_label_text_reserved(void *start, void *end) static void jump_label_update(struct static_key *key) { struct jump_entry *stop = __stop___jump_table; + bool init = system_state < SYSTEM_RUNNING; struct jump_entry *entry; #ifdef CONFIG_MODULES struct module *mod; @@ -804,15 +805,16 @@ static void jump_label_update(struct static_key *key) preempt_disable(); mod = __module_address((unsigned long)key); - if (mod) + if (mod) { stop = mod->jump_entries + mod->num_jump_entries; + init = mod->state == MODULE_STATE_COMING; + } preempt_enable(); #endif entry = static_key_entries(key); /* if there are no users, entry can be NULL */ if (entry) - __jump_label_update(key, entry, stop, - system_state < SYSTEM_RUNNING); + __jump_label_update(key, entry, stop, init); } #ifdef CONFIG_STATIC_KEYS_SELFTEST ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [RFC][PATCH] jump_label/static_call: Add MAINTAINERS 2020-12-16 10:59 ` Peter Zijlstra @ 2020-12-16 13:30 ` Peter Zijlstra 2020-12-16 13:42 ` Peter Zijlstra 2020-12-18 16:02 ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra 2020-12-16 13:54 ` [PATCH] jump_label: Fix usage in module __init Peter Zijlstra 2020-12-16 20:45 ` static_branch_enable() does not work from a __init function? Dexuan Cui 2 siblings, 2 replies; 20+ messages in thread From: Peter Zijlstra @ 2020-12-16 13:30 UTC (permalink / raw) To: Dexuan Cui Cc: Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, jeyu, Josh Poimboeuf, ardb, Steven Rostedt, Jason Baron FWIW, I recently noticed we're not being Cc'ed on patches for this stuff, so how about we do something like the below? Anybody holler if they don't agree with the letter assigned, or if they feel they've been left out entirely and want in on the 'fun' :-) --- Subject: jump_label/static_call: Add MAINTAINERS From: Peter Zijlstra <peterz@infradead.org> These files don't appear to have a MAINTAINERS entry and as such patches miss being seen by people who know this code. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- MAINTAINERS | 12 ++++++++++++ 1 file changed, 12 insertions(+) --- a/MAINTAINERS +++ b/MAINTAINERS @@ -16766,6 +16766,18 @@ M: Ion Badulescu <ionut@badula.org> S: Odd Fixes F: drivers/net/ethernet/adaptec/starfire* +STATIC BRANCH/CALL +M: Peter Zijlstra <peterz@infradead.org> +M: Josh Poimboeuf <jpoimboe@redhat.com> +M: Jason Baron <jbaron@akamai.com> +R: Steven Rostedt <rostedt@goodmis.org> +R: Ard Biesheuvel <ardb@kernel.org> +S: Supported +F: include/linux/jump_label*.h +F: include/linux/static_call*.h +F: kernel/jump_label.c +F: kernel/static_call.c + STEC S1220 SKD DRIVER M: Damien Le Moal <Damien.LeMoal@wdc.com> L: linux-block@vger.kernel.org ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] jump_label/static_call: Add MAINTAINERS 2020-12-16 13:30 ` [RFC][PATCH] jump_label/static_call: Add MAINTAINERS Peter Zijlstra @ 2020-12-16 13:42 ` Peter Zijlstra 2020-12-16 14:23 ` Steven Rostedt ` (3 more replies) 2020-12-18 16:02 ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra 1 sibling, 4 replies; 20+ messages in thread From: Peter Zijlstra @ 2020-12-16 13:42 UTC (permalink / raw) To: Dexuan Cui Cc: Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, jeyu, Josh Poimboeuf, ardb, Steven Rostedt, Jason Baron On Wed, Dec 16, 2020 at 02:30:14PM +0100, Peter Zijlstra wrote: > > FWIW, I recently noticed we're not being Cc'ed on patches for this > stuff, so how about we do something like the below? > > Anybody holler if they don't agree with the letter assigned, or if they > feel they've been left out entirely and want in on the 'fun' :-) > > --- > Subject: jump_label/static_call: Add MAINTAINERS > From: Peter Zijlstra <peterz@infradead.org> > > These files don't appear to have a MAINTAINERS entry and as such > patches miss being seen by people who know this code. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > MAINTAINERS | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16766,6 +16766,18 @@ M: Ion Badulescu <ionut@badula.org> > S: Odd Fixes > F: drivers/net/ethernet/adaptec/starfire* > > +STATIC BRANCH/CALL > +M: Peter Zijlstra <peterz@infradead.org> > +M: Josh Poimboeuf <jpoimboe@redhat.com> > +M: Jason Baron <jbaron@akamai.com> > +R: Steven Rostedt <rostedt@goodmis.org> > +R: Ard Biesheuvel <ardb@kernel.org> > +S: Supported F: arch/*/include/asm/jump_label*.h F: arch/*/include/asm/static_call*.h F: arch/*/kernel/jump_label.c F: arch/*/kernel/static_call.c These too? > +F: include/linux/jump_label*.h > +F: include/linux/static_call*.h > +F: kernel/jump_label.c > +F: kernel/static_call.c > + > STEC S1220 SKD DRIVER > M: Damien Le Moal <Damien.LeMoal@wdc.com> > L: linux-block@vger.kernel.org ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] jump_label/static_call: Add MAINTAINERS 2020-12-16 13:42 ` Peter Zijlstra @ 2020-12-16 14:23 ` Steven Rostedt 2020-12-16 16:19 ` Josh Poimboeuf ` (2 subsequent siblings) 3 siblings, 0 replies; 20+ messages in thread From: Steven Rostedt @ 2020-12-16 14:23 UTC (permalink / raw) To: Peter Zijlstra Cc: Dexuan Cui, Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, jeyu, Josh Poimboeuf, ardb, Jason Baron On Wed, 16 Dec 2020 14:42:49 +0100 Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Dec 16, 2020 at 02:30:14PM +0100, Peter Zijlstra wrote: > > > > FWIW, I recently noticed we're not being Cc'ed on patches for this > > stuff, so how about we do something like the below? > > > > Anybody holler if they don't agree with the letter assigned, or if they > > feel they've been left out entirely and want in on the 'fun' :-) > > > > --- > > Subject: jump_label/static_call: Add MAINTAINERS > > From: Peter Zijlstra <peterz@infradead.org> > > > > These files don't appear to have a MAINTAINERS entry and as such > > patches miss being seen by people who know this code. > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > --- > > MAINTAINERS | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -16766,6 +16766,18 @@ M: Ion Badulescu <ionut@badula.org> > > S: Odd Fixes > > F: drivers/net/ethernet/adaptec/starfire* > > > > +STATIC BRANCH/CALL > > +M: Peter Zijlstra <peterz@infradead.org> > > +M: Josh Poimboeuf <jpoimboe@redhat.com> > > +M: Jason Baron <jbaron@akamai.com> > > +R: Steven Rostedt <rostedt@goodmis.org> > > +R: Ard Biesheuvel <ardb@kernel.org> > > +S: Supported > > F: arch/*/include/asm/jump_label*.h > F: arch/*/include/asm/static_call*.h > F: arch/*/kernel/jump_label.c > F: arch/*/kernel/static_call.c > > These too? Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org> -- Steve > > > +F: include/linux/jump_label*.h > > +F: include/linux/static_call*.h > > +F: kernel/jump_label.c > > +F: kernel/static_call.c > > + > > STEC S1220 SKD DRIVER > > M: Damien Le Moal <Damien.LeMoal@wdc.com> > > L: linux-block@vger.kernel.org ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] jump_label/static_call: Add MAINTAINERS 2020-12-16 13:42 ` Peter Zijlstra 2020-12-16 14:23 ` Steven Rostedt @ 2020-12-16 16:19 ` Josh Poimboeuf 2020-12-16 16:19 ` Ard Biesheuvel 2020-12-16 21:16 ` Jason Baron 3 siblings, 0 replies; 20+ messages in thread From: Josh Poimboeuf @ 2020-12-16 16:19 UTC (permalink / raw) To: Peter Zijlstra Cc: Dexuan Cui, Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, jeyu, ardb, Steven Rostedt, Jason Baron On Wed, Dec 16, 2020 at 02:42:49PM +0100, Peter Zijlstra wrote: > > +STATIC BRANCH/CALL > > +M: Peter Zijlstra <peterz@infradead.org> > > +M: Josh Poimboeuf <jpoimboe@redhat.com> > > +M: Jason Baron <jbaron@akamai.com> > > +R: Steven Rostedt <rostedt@goodmis.org> > > +R: Ard Biesheuvel <ardb@kernel.org> > > +S: Supported > > F: arch/*/include/asm/jump_label*.h > F: arch/*/include/asm/static_call*.h > F: arch/*/kernel/jump_label.c > F: arch/*/kernel/static_call.c > > These too? > > > +F: include/linux/jump_label*.h > > +F: include/linux/static_call*.h > > +F: kernel/jump_label.c > > +F: kernel/static_call.c Acked-by: Josh Poimboeuf <jpoimboe@redhat.com> -- Josh ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] jump_label/static_call: Add MAINTAINERS 2020-12-16 13:42 ` Peter Zijlstra 2020-12-16 14:23 ` Steven Rostedt 2020-12-16 16:19 ` Josh Poimboeuf @ 2020-12-16 16:19 ` Ard Biesheuvel 2020-12-16 21:16 ` Jason Baron 3 siblings, 0 replies; 20+ messages in thread From: Ard Biesheuvel @ 2020-12-16 16:19 UTC (permalink / raw) To: Peter Zijlstra Cc: Dexuan Cui, Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, Jessica Yu, Josh Poimboeuf, Steven Rostedt, Jason Baron On Wed, 16 Dec 2020 at 14:42, Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Dec 16, 2020 at 02:30:14PM +0100, Peter Zijlstra wrote: > > > > FWIW, I recently noticed we're not being Cc'ed on patches for this > > stuff, so how about we do something like the below? > > > > Anybody holler if they don't agree with the letter assigned, or if they > > feel they've been left out entirely and want in on the 'fun' :-) > > > > --- > > Subject: jump_label/static_call: Add MAINTAINERS > > From: Peter Zijlstra <peterz@infradead.org> > > > > These files don't appear to have a MAINTAINERS entry and as such > > patches miss being seen by people who know this code. > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > --- > > MAINTAINERS | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -16766,6 +16766,18 @@ M: Ion Badulescu <ionut@badula.org> > > S: Odd Fixes > > F: drivers/net/ethernet/adaptec/starfire* > > > > +STATIC BRANCH/CALL > > +M: Peter Zijlstra <peterz@infradead.org> > > +M: Josh Poimboeuf <jpoimboe@redhat.com> > > +M: Jason Baron <jbaron@akamai.com> > > +R: Steven Rostedt <rostedt@goodmis.org> > > +R: Ard Biesheuvel <ardb@kernel.org> > > +S: Supported > > F: arch/*/include/asm/jump_label*.h > F: arch/*/include/asm/static_call*.h > F: arch/*/kernel/jump_label.c > F: arch/*/kernel/static_call.c > > These too? > Yes, that makes sense. Acked-by: Ard Biesheuvel <ardb@kernel.org> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] jump_label/static_call: Add MAINTAINERS 2020-12-16 13:42 ` Peter Zijlstra ` (2 preceding siblings ...) 2020-12-16 16:19 ` Ard Biesheuvel @ 2020-12-16 21:16 ` Jason Baron 3 siblings, 0 replies; 20+ messages in thread From: Jason Baron @ 2020-12-16 21:16 UTC (permalink / raw) To: Peter Zijlstra, Dexuan Cui Cc: Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, jeyu, Josh Poimboeuf, ardb, Steven Rostedt On 12/16/20 8:42 AM, Peter Zijlstra wrote: > On Wed, Dec 16, 2020 at 02:30:14PM +0100, Peter Zijlstra wrote: >> >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -16766,6 +16766,18 @@ M: Ion Badulescu <ionut@badula.org> >> S: Odd Fixes >> F: drivers/net/ethernet/adaptec/starfire* >> >> +STATIC BRANCH/CALL >> +M: Peter Zijlstra <peterz@infradead.org> >> +M: Josh Poimboeuf <jpoimboe@redhat.com> >> +M: Jason Baron <jbaron@akamai.com> >> +R: Steven Rostedt <rostedt@goodmis.org> >> +R: Ard Biesheuvel <ardb@kernel.org> >> +S: Supported > > F: arch/*/include/asm/jump_label*.h > F: arch/*/include/asm/static_call*.h > F: arch/*/kernel/jump_label.c > F: arch/*/kernel/static_call.c > > These too? > >> +F: include/linux/jump_label*.h >> +F: include/linux/static_call*.h >> +F: kernel/jump_label.c >> +F: kernel/static_call.c >> + >> STEC S1220 SKD DRIVER >> M: Damien Le Moal <Damien.LeMoal@wdc.com> >> L: linux-block@vger.kernel.org Thanks Peter. Acked-by: Jason Baron <jbaron@akamai.com> ^ permalink raw reply [flat|nested] 20+ messages in thread
* [tip: locking/urgent] jump_label/static_call: Add MAINTAINERS 2020-12-16 13:30 ` [RFC][PATCH] jump_label/static_call: Add MAINTAINERS Peter Zijlstra 2020-12-16 13:42 ` Peter Zijlstra @ 2020-12-18 16:02 ` tip-bot2 for Peter Zijlstra 1 sibling, 0 replies; 20+ messages in thread From: tip-bot2 for Peter Zijlstra @ 2020-12-18 16:02 UTC (permalink / raw) To: linux-tip-commits Cc: Peter Zijlstra (Intel), Steven Rostedt (VMware), Ard Biesheuvel, Josh Poimboeuf, Jason Baron, x86, linux-kernel The following commit has been merged into the locking/urgent branch of tip: Commit-ID: 441fa3409769180df2fd12fcada35441435a120c Gitweb: https://git.kernel.org/tip/441fa3409769180df2fd12fcada35441435a120c Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Wed, 16 Dec 2020 14:19:22 +01:00 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Fri, 18 Dec 2020 16:53:12 +01:00 jump_label/static_call: Add MAINTAINERS These files don't appear to have a MAINTAINERS entry and as such patches miss being seen by people who know this code. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org> Acked-by: Ard Biesheuvel <ardb@kernel.org> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com> Acked-by: Jason Baron <jbaron@akamai.com> Link: https://lkml.kernel.org/r/20201216133014.GT3092@hirez.programming.kicks-ass.net --- MAINTAINERS | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 281de21..be02614 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -16675,6 +16675,22 @@ M: Ion Badulescu <ionut@badula.org> S: Odd Fixes F: drivers/net/ethernet/adaptec/starfire* +STATIC BRANCH/CALL +M: Peter Zijlstra <peterz@infradead.org> +M: Josh Poimboeuf <jpoimboe@redhat.com> +M: Jason Baron <jbaron@akamai.com> +R: Steven Rostedt <rostedt@goodmis.org> +R: Ard Biesheuvel <ardb@kernel.org> +S: Supported +F: arch/*/include/asm/jump_label*.h +F: arch/*/include/asm/static_call*.h +F: arch/*/kernel/jump_label.c +F: arch/*/kernel/static_call.c +F: include/linux/jump_label*.h +F: include/linux/static_call*.h +F: kernel/jump_label.c +F: kernel/static_call.c + STEC S1220 SKD DRIVER M: Damien Le Moal <Damien.LeMoal@wdc.com> L: linux-block@vger.kernel.org ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] jump_label: Fix usage in module __init 2020-12-16 10:59 ` Peter Zijlstra 2020-12-16 13:30 ` [RFC][PATCH] jump_label/static_call: Add MAINTAINERS Peter Zijlstra @ 2020-12-16 13:54 ` Peter Zijlstra 2020-12-16 16:36 ` Josh Poimboeuf 2020-12-18 16:02 ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra 2020-12-16 20:45 ` static_branch_enable() does not work from a __init function? Dexuan Cui 2 siblings, 2 replies; 20+ messages in thread From: Peter Zijlstra @ 2020-12-16 13:54 UTC (permalink / raw) To: Dexuan Cui Cc: Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, jeyu, Josh Poimboeuf, ardb Final patch looks like this. --- Subject: jump_label: Fix usage in module __init From: Peter Zijlstra <peterz@infradead.org> Date: Wed Dec 16 12:21:36 CET 2020 When the static_key is part of the module, and the module calls static_key_inc/enable() from it's __init section *AND* has a static_branch_*() user in that very same __init section, things go wobbly. If the static_key lives outside the module, jump_label_add_module() would append this module's sites to the key and jump_label_update() would take the static_key_linked() branch and all would be fine. If all the sites are outside of __init, then everything will be fine too. However, when all is aligned just as described above, jump_label_update() calls __jump_label_update(.init = false) and we'll not update sites in __init text. Fixes: 19483677684b ("jump_label: Annotate entries that operate on __init code earlier") Reported-by: Dexuan Cui <decui@microsoft.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Tested-by: Jessica Yu <jeyu@kernel.org> --- kernel/jump_label.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -793,6 +793,7 @@ int jump_label_text_reserved(void *start static void jump_label_update(struct static_key *key) { struct jump_entry *stop = __stop___jump_table; + bool init = system_state < SYSTEM_RUNNING; struct jump_entry *entry; #ifdef CONFIG_MODULES struct module *mod; @@ -804,15 +805,16 @@ static void jump_label_update(struct sta preempt_disable(); mod = __module_address((unsigned long)key); - if (mod) + if (mod) { stop = mod->jump_entries + mod->num_jump_entries; + init = mod->state == MODULE_STATE_COMING; + } preempt_enable(); #endif entry = static_key_entries(key); /* if there are no users, entry can be NULL */ if (entry) - __jump_label_update(key, entry, stop, - system_state < SYSTEM_RUNNING); + __jump_label_update(key, entry, stop, init); } #ifdef CONFIG_STATIC_KEYS_SELFTEST ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] jump_label: Fix usage in module __init 2020-12-16 13:54 ` [PATCH] jump_label: Fix usage in module __init Peter Zijlstra @ 2020-12-16 16:36 ` Josh Poimboeuf 2020-12-18 16:02 ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra 1 sibling, 0 replies; 20+ messages in thread From: Josh Poimboeuf @ 2020-12-16 16:36 UTC (permalink / raw) To: Peter Zijlstra Cc: Dexuan Cui, Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, jeyu, ardb On Wed, Dec 16, 2020 at 02:54:35PM +0100, Peter Zijlstra wrote: > Subject: jump_label: Fix usage in module __init > From: Peter Zijlstra <peterz@infradead.org> > Date: Wed Dec 16 12:21:36 CET 2020 > > When the static_key is part of the module, and the module calls > static_key_inc/enable() from it's __init section *AND* has a > static_branch_*() user in that very same __init section, things go > wobbly. > > If the static_key lives outside the module, jump_label_add_module() > would append this module's sites to the key and jump_label_update() > would take the static_key_linked() branch and all would be fine. > > If all the sites are outside of __init, then everything will be fine > too. > > However, when all is aligned just as described above, > jump_label_update() calls __jump_label_update(.init = false) and we'll > not update sites in __init text. > > Fixes: 19483677684b ("jump_label: Annotate entries that operate on __init code earlier") > Reported-by: Dexuan Cui <decui@microsoft.com> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > Tested-by: Jessica Yu <jeyu@kernel.org> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com> -- Josh ^ permalink raw reply [flat|nested] 20+ messages in thread
* [tip: locking/urgent] jump_label: Fix usage in module __init 2020-12-16 13:54 ` [PATCH] jump_label: Fix usage in module __init Peter Zijlstra 2020-12-16 16:36 ` Josh Poimboeuf @ 2020-12-18 16:02 ` tip-bot2 for Peter Zijlstra 1 sibling, 0 replies; 20+ messages in thread From: tip-bot2 for Peter Zijlstra @ 2020-12-18 16:02 UTC (permalink / raw) To: linux-tip-commits Cc: Dexuan Cui, Peter Zijlstra (Intel), Josh Poimboeuf, Jessica Yu, x86, linux-kernel The following commit has been merged into the locking/urgent branch of tip: Commit-ID: 55d2eba8e7cd439c11cdb204898c2d384227629b Gitweb: https://git.kernel.org/tip/55d2eba8e7cd439c11cdb204898c2d384227629b Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Wed, 16 Dec 2020 12:21:36 +01:00 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Fri, 18 Dec 2020 16:53:12 +01:00 jump_label: Fix usage in module __init When the static_key is part of the module, and the module calls static_key_inc/enable() from it's __init section *AND* has a static_branch_*() user in that very same __init section, things go wobbly. If the static_key lives outside the module, jump_label_add_module() would append this module's sites to the key and jump_label_update() would take the static_key_linked() branch and all would be fine. If all the sites are outside of __init, then everything will be fine too. However, when all is aligned just as described above, jump_label_update() calls __jump_label_update(.init = false) and we'll not update sites in __init text. Fixes: 19483677684b ("jump_label: Annotate entries that operate on __init code earlier") Reported-by: Dexuan Cui <decui@microsoft.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com> Tested-by: Jessica Yu <jeyu@kernel.org> Link: https://lkml.kernel.org/r/20201216135435.GV3092@hirez.programming.kicks-ass.net --- kernel/jump_label.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/kernel/jump_label.c b/kernel/jump_label.c index 015ef90..c6a39d6 100644 --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -793,6 +793,7 @@ int jump_label_text_reserved(void *start, void *end) static void jump_label_update(struct static_key *key) { struct jump_entry *stop = __stop___jump_table; + bool init = system_state < SYSTEM_RUNNING; struct jump_entry *entry; #ifdef CONFIG_MODULES struct module *mod; @@ -804,15 +805,16 @@ static void jump_label_update(struct static_key *key) preempt_disable(); mod = __module_address((unsigned long)key); - if (mod) + if (mod) { stop = mod->jump_entries + mod->num_jump_entries; + init = mod->state == MODULE_STATE_COMING; + } preempt_enable(); #endif entry = static_key_entries(key); /* if there are no users, entry can be NULL */ if (entry) - __jump_label_update(key, entry, stop, - system_state < SYSTEM_RUNNING); + __jump_label_update(key, entry, stop, init); } #ifdef CONFIG_STATIC_KEYS_SELFTEST ^ permalink raw reply related [flat|nested] 20+ messages in thread
* RE: static_branch_enable() does not work from a __init function? 2020-12-16 10:59 ` Peter Zijlstra 2020-12-16 13:30 ` [RFC][PATCH] jump_label/static_call: Add MAINTAINERS Peter Zijlstra 2020-12-16 13:54 ` [PATCH] jump_label: Fix usage in module __init Peter Zijlstra @ 2020-12-16 20:45 ` Dexuan Cui 2 siblings, 0 replies; 20+ messages in thread From: Dexuan Cui @ 2020-12-16 20:45 UTC (permalink / raw) To: Peter Zijlstra Cc: Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, jeyu, Josh Poimboeuf, ardb > From: Peter Zijlstra <peterz@infradead.org> > Sent: Wednesday, December 16, 2020 2:59 AM > ... > So I think the reason your above module doesn't work, while the one in > vmx_init() does work (for 5.10) should be fixed by the completely > untested below. > > I've no clue about 5.4 and no desire to investigate. That's what distro > people are for. > > Can you verify? > > --- > diff --git a/kernel/jump_label.c b/kernel/jump_label.c > index 015ef903ce8c..c6a39d662935 100644 > --- a/kernel/jump_label.c > +++ b/kernel/jump_label.c > @@ -793,6 +793,7 @@ int jump_label_text_reserved(void *start, void *end) > static void jump_label_update(struct static_key *key) > { > struct jump_entry *stop = __stop___jump_table; > + bool init = system_state < SYSTEM_RUNNING; > struct jump_entry *entry; > #ifdef CONFIG_MODULES > struct module *mod; > @@ -804,15 +805,16 @@ static void jump_label_update(struct static_key > *key) > > preempt_disable(); > mod = __module_address((unsigned long)key); > - if (mod) > + if (mod) { > stop = mod->jump_entries + mod->num_jump_entries; > + init = mod->state == MODULE_STATE_COMING; > + } > preempt_enable(); > #endif > entry = static_key_entries(key); > /* if there are no users, entry can be NULL */ > if (entry) > - __jump_label_update(key, entry, stop, > - system_state < SYSTEM_RUNNING); > + __jump_label_update(key, entry, stop, init); > } > > #ifdef CONFIG_STATIC_KEYS_SELFTEST Yes, this patch fixes the issue found by the test module for both v5.10 and v5.4. Thank you, Peter! Dexuan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: static_branch_enable() does not work from a __init function? 2020-12-16 9:26 ` Peter Zijlstra 2020-12-16 10:59 ` Peter Zijlstra @ 2020-12-16 11:55 ` Jessica Yu 2020-12-16 12:47 ` Peter Zijlstra 2020-12-16 12:38 ` Jessica Yu 2 siblings, 1 reply; 20+ messages in thread From: Jessica Yu @ 2020-12-16 11:55 UTC (permalink / raw) To: Peter Zijlstra Cc: Dexuan Cui, Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, Josh Poimboeuf +++ Peter Zijlstra [16/12/20 10:26 +0100]: [snip] >> PS, I originally found: in arch/x86/kvm/vmx/vmx.c: vmx_init(), it looks >> like the line "static_branch_enable(&enable_evmcs);" does not take effect >> in a v5.4-based kernel, but does take effect in the v5.10 kernel in the >> same x86-64 virtual machine on Hyper-V, so I made the above test module >> to test static_branch_enable(), and found that static_branch_enable() in >> the test module does not work with both v5.10 and my v5.4 kernel, if the >> __init marker is used. Because the jump label code currently does not allow you to update if the entry resides in an init section. By marking the module init section __init you place it in the .init.text section. jump_label_add_module() detects this (by calling within_module_init()) and marks the entry by calling jump_entry_set_init(). Then you have the following sequence of calls (roughly): static_branch_enable static_key_enable static_key_enable_cpuslocked jump_label_update jump_label_can_update jump_entry_is_init returns true, so bail out Judging from the comment in jump_label_can_update(), this seems to be intentional behavior: static bool jump_label_can_update(struct jump_entry *entry, bool init) { /* * Cannot update code that was in an init text area. */ if (!init && jump_entry_is_init(entry)) return false; By removing the __init marker you're bypassing the within_module_init() check and that's why it works. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: static_branch_enable() does not work from a __init function? 2020-12-16 11:55 ` Jessica Yu @ 2020-12-16 12:47 ` Peter Zijlstra 2020-12-16 13:10 ` Jessica Yu 0 siblings, 1 reply; 20+ messages in thread From: Peter Zijlstra @ 2020-12-16 12:47 UTC (permalink / raw) To: Jessica Yu Cc: Dexuan Cui, Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, Josh Poimboeuf On Wed, Dec 16, 2020 at 12:55:25PM +0100, Jessica Yu wrote: > +++ Peter Zijlstra [16/12/20 10:26 +0100]: > [snip] > > > PS, I originally found: in arch/x86/kvm/vmx/vmx.c: vmx_init(), it looks > > > like the line "static_branch_enable(&enable_evmcs);" does not take effect > > > in a v5.4-based kernel, but does take effect in the v5.10 kernel in the > > > same x86-64 virtual machine on Hyper-V, so I made the above test module > > > to test static_branch_enable(), and found that static_branch_enable() in > > > the test module does not work with both v5.10 and my v5.4 kernel, if the > > > __init marker is used. > > Because the jump label code currently does not allow you to update if > the entry resides in an init section. By marking the module init > section __init you place it in the .init.text section. > jump_label_add_module() detects this (by calling within_module_init()) > and marks the entry by calling jump_entry_set_init(). Then you have > the following sequence of calls (roughly): > > static_branch_enable > static_key_enable > static_key_enable_cpuslocked > jump_label_update > jump_label_can_update > jump_entry_is_init returns true, so bail out > > Judging from the comment in jump_label_can_update(), this seems to be > intentional behavior: > > static bool jump_label_can_update(struct jump_entry *entry, bool init) > { > /* > * Cannot update code that was in an init text area. > */ > if (!init && jump_entry_is_init(entry)) > return false; > Only because we're having .init=false, incorrectly. See the other email. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: static_branch_enable() does not work from a __init function? 2020-12-16 12:47 ` Peter Zijlstra @ 2020-12-16 13:10 ` Jessica Yu 2020-12-16 13:23 ` Peter Zijlstra 0 siblings, 1 reply; 20+ messages in thread From: Jessica Yu @ 2020-12-16 13:10 UTC (permalink / raw) To: Peter Zijlstra Cc: Dexuan Cui, Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, Josh Poimboeuf +++ Peter Zijlstra [16/12/20 13:47 +0100]: >On Wed, Dec 16, 2020 at 12:55:25PM +0100, Jessica Yu wrote: >> +++ Peter Zijlstra [16/12/20 10:26 +0100]: >> [snip] >> > > PS, I originally found: in arch/x86/kvm/vmx/vmx.c: vmx_init(), it looks >> > > like the line "static_branch_enable(&enable_evmcs);" does not take effect >> > > in a v5.4-based kernel, but does take effect in the v5.10 kernel in the >> > > same x86-64 virtual machine on Hyper-V, so I made the above test module >> > > to test static_branch_enable(), and found that static_branch_enable() in >> > > the test module does not work with both v5.10 and my v5.4 kernel, if the >> > > __init marker is used. >> >> Because the jump label code currently does not allow you to update if >> the entry resides in an init section. By marking the module init >> section __init you place it in the .init.text section. >> jump_label_add_module() detects this (by calling within_module_init()) >> and marks the entry by calling jump_entry_set_init(). Then you have >> the following sequence of calls (roughly): >> >> static_branch_enable >> static_key_enable >> static_key_enable_cpuslocked >> jump_label_update >> jump_label_can_update >> jump_entry_is_init returns true, so bail out >> >> Judging from the comment in jump_label_can_update(), this seems to be >> intentional behavior: >> >> static bool jump_label_can_update(struct jump_entry *entry, bool init) >> { >> /* >> * Cannot update code that was in an init text area. >> */ >> if (!init && jump_entry_is_init(entry)) >> return false; >> > >Only because we're having .init=false, incorrectly. See the other email. Ah yeah, you're right. I also misread the intention of the if conditional :/ If we're currently running an init function it's fine, but after that it will be unsafe. Btw, your patch seems to work for me, using the test module provided by Dexuan. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: static_branch_enable() does not work from a __init function? 2020-12-16 13:10 ` Jessica Yu @ 2020-12-16 13:23 ` Peter Zijlstra 2020-12-16 13:27 ` Jessica Yu 0 siblings, 1 reply; 20+ messages in thread From: Peter Zijlstra @ 2020-12-16 13:23 UTC (permalink / raw) To: Jessica Yu Cc: Dexuan Cui, Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, Josh Poimboeuf On Wed, Dec 16, 2020 at 02:10:16PM +0100, Jessica Yu wrote: > +++ Peter Zijlstra [16/12/20 13:47 +0100]: > > Only because we're having .init=false, incorrectly. See the other email. > > Ah yeah, you're right. I also misread the intention of the if > conditional :/ If we're currently running an init function it's fine, > but after that it will be unsafe. Exactly, seeing how it'll end up being freed and such ;-) > Btw, your patch seems to work for me, using the test module provided > by Dexuan. Ah, excellent. I couldn't be bothered to figure out how to build a module ;-) I'll add your Tested-by and go queue it for /urgent I suppose. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: static_branch_enable() does not work from a __init function? 2020-12-16 13:23 ` Peter Zijlstra @ 2020-12-16 13:27 ` Jessica Yu 0 siblings, 0 replies; 20+ messages in thread From: Jessica Yu @ 2020-12-16 13:27 UTC (permalink / raw) To: Peter Zijlstra Cc: Dexuan Cui, Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, Josh Poimboeuf +++ Peter Zijlstra [16/12/20 14:23 +0100]: >On Wed, Dec 16, 2020 at 02:10:16PM +0100, Jessica Yu wrote: >> +++ Peter Zijlstra [16/12/20 13:47 +0100]: > >> > Only because we're having .init=false, incorrectly. See the other email. >> >> Ah yeah, you're right. I also misread the intention of the if >> conditional :/ If we're currently running an init function it's fine, >> but after that it will be unsafe. > >Exactly, seeing how it'll end up being freed and such ;-) > >> Btw, your patch seems to work for me, using the test module provided >> by Dexuan. > >Ah, excellent. I couldn't be bothered to figure out how to build a >module ;-) > >I'll add your Tested-by and go queue it for /urgent I suppose. That's fine by me :-) Tested-by: Jessica Yu <jeyu@kernel.org> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: static_branch_enable() does not work from a __init function? 2020-12-16 9:26 ` Peter Zijlstra 2020-12-16 10:59 ` Peter Zijlstra 2020-12-16 11:55 ` Jessica Yu @ 2020-12-16 12:38 ` Jessica Yu 2 siblings, 0 replies; 20+ messages in thread From: Jessica Yu @ 2020-12-16 12:38 UTC (permalink / raw) To: Peter Zijlstra Cc: Dexuan Cui, Ingo Molnar, Daniel Bristot de Oliveira, kvm, linux-kernel, Josh Poimboeuf +++ Peter Zijlstra [16/12/20 10:26 +0100]: >On Wed, Dec 16, 2020 at 03:54:29AM +0000, Dexuan Cui wrote: >> PS, I originally found: in arch/x86/kvm/vmx/vmx.c: vmx_init(), it looks >> like the line "static_branch_enable(&enable_evmcs);" does not take effect >> in a v5.4-based kernel, but does take effect in the v5.10 kernel in the >> same x86-64 virtual machine on Hyper-V, so I made the above test module >> to test static_branch_enable(), and found that static_branch_enable() in >> the test module does not work with both v5.10 and my v5.4 kernel, if the >> __init marker is used. By the way, it probably works now because there was a workaround merged in v5.10, that mentions this very issue: commit 064eedf2c50f692088e1418c553084bf9c1432f8 Author: Vitaly Kuznetsov <vkuznets@redhat.com> Date: Wed Oct 14 16:33:46 2020 +0200 KVM: VMX: eVMCS: make evmcs_sanitize_exec_ctrls() work again It was noticed that evmcs_sanitize_exec_ctrls() is not being executed nowadays despite the code checking 'enable_evmcs' static key looking correct. Turns out, static key magic doesn't work in '__init' section (and it is unclear when things changed) but setup_vmcs_config() is called only once per CPU so we don't really need it to. Switch to checking 'enlightened_vmcs' instead, it is supposed to be in sync with 'enable_evmcs'. Opportunistically make evmcs_sanitize_exec_ctrls '__init' and drop unneeded extra newline from it. Reported-by: Yang Weijiang <weijiang.yang@intel.com> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> Message-Id: <20201014143346.2430936-1-vkuznets@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-12-18 16:03 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-16 3:54 static_branch_enable() does not work from a __init function? Dexuan Cui 2020-12-16 9:26 ` Peter Zijlstra 2020-12-16 10:59 ` Peter Zijlstra 2020-12-16 13:30 ` [RFC][PATCH] jump_label/static_call: Add MAINTAINERS Peter Zijlstra 2020-12-16 13:42 ` Peter Zijlstra 2020-12-16 14:23 ` Steven Rostedt 2020-12-16 16:19 ` Josh Poimboeuf 2020-12-16 16:19 ` Ard Biesheuvel 2020-12-16 21:16 ` Jason Baron 2020-12-18 16:02 ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra 2020-12-16 13:54 ` [PATCH] jump_label: Fix usage in module __init Peter Zijlstra 2020-12-16 16:36 ` Josh Poimboeuf 2020-12-18 16:02 ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra 2020-12-16 20:45 ` static_branch_enable() does not work from a __init function? Dexuan Cui 2020-12-16 11:55 ` Jessica Yu 2020-12-16 12:47 ` Peter Zijlstra 2020-12-16 13:10 ` Jessica Yu 2020-12-16 13:23 ` Peter Zijlstra 2020-12-16 13:27 ` Jessica Yu 2020-12-16 12:38 ` Jessica Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).