static_branch_enable() does not work from a __init function?

static_branch_enable() does not work from a __init function?

[Dexuan Cui]

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

Re: static_branch_enable() does not work from a __init function?

[Peter Zijlstra]

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
> 
> 

Re: static_branch_enable() does not work from a __init function?

[Peter Zijlstra]

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

Re: static_branch_enable() does not work from a __init function?

[Jessica Yu]

+++ 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.

Re: static_branch_enable() does not work from a __init function?

[Jessica Yu]

+++ 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>

Re: static_branch_enable() does not work from a __init function?

[Peter Zijlstra]

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.

Re: static_branch_enable() does not work from a __init function?

[Jessica Yu]

+++ 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.

Re: static_branch_enable() does not work from a __init function?

[Peter Zijlstra]

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.

Re: static_branch_enable() does not work from a __init function?

[Jessica Yu]

+++ 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>

[RFC][PATCH] jump_label/static_call: Add MAINTAINERS

[Peter Zijlstra]

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

Re: [RFC][PATCH] jump_label/static_call: Add MAINTAINERS

[Peter Zijlstra]

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

[PATCH] jump_label: Fix usage in module __init

[Peter Zijlstra]

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

Re: [RFC][PATCH] jump_label/static_call: Add MAINTAINERS

[Steven Rostedt]

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  

Re: [RFC][PATCH] jump_label/static_call: Add MAINTAINERS

[Josh Poimboeuf]

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

Re: [RFC][PATCH] jump_label/static_call: Add MAINTAINERS

[Ard Biesheuvel]

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>

Re: [PATCH] jump_label: Fix usage in module __init

[Josh Poimboeuf]

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

RE: static_branch_enable() does not work from a __init function?

[Dexuan Cui]

> 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

Re: [RFC][PATCH] jump_label/static_call: Add MAINTAINERS

[Jason Baron]

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>

[tip: locking/urgent] jump_label: Fix usage in module __init

[tip-bot2 for Peter Zijlstra]

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

[tip: locking/urgent] jump_label/static_call: Add MAINTAINERS

[tip-bot2 for Peter Zijlstra]

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