* Re: [PATCH] objtool,static_call: Don't emit static_call_site for .exit.text
2021-03-17 12:45 ` [PATCH] objtool,static_call: Don't emit static_call_site for .exit.text Peter Zijlstra
@ 2021-03-17 13:37 ` Sumit Garg
2021-03-17 21:55 ` Jarkko Sakkinen
2021-03-17 21:01 ` Jarkko Sakkinen
2021-03-18 0:02 ` Josh Poimboeuf
2 siblings, 1 reply; 9+ messages in thread
From: Sumit Garg @ 2021-03-17 13:37 UTC (permalink / raw)
To: Peter Zijlstra, Jarkko Sakkinen
Cc: Oliver Sang, Josh Poimboeuf, jbaron, lkp, kbuild test robot,
Linux Kernel Mailing List
On Wed, 17 Mar 2021 at 18:16, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Mar 17, 2021 at 05:25:48PM +0530, Sumit Garg wrote:
> > Thanks Peter for this fix. It does work for me on qemu for x86. Can
> > you turn this into a proper fix patch? BTW, feel free to add:
>
> Per the below, the original patch ought to be fixed as well, to not use
> static_call() in __exit.
Okay, fair enough.
Jarkko,
Can you please incorporate the following change to the original patch as well?
diff --git a/security/keys/trusted-keys/trusted_core.c
b/security/keys/trusted-keys/trusted_core.c
index ec3a066a4b42..bef52d1ebe5e 100644
--- a/security/keys/trusted-keys/trusted_core.c
+++ b/security/keys/trusted-keys/trusted_core.c
@@ -41,7 +41,7 @@ DEFINE_STATIC_CALL_NULL(trusted_key_unseal,
*trusted_key_sources[0].ops->unseal);
DEFINE_STATIC_CALL_NULL(trusted_key_get_random,
*trusted_key_sources[0].ops->get_random);
-DEFINE_STATIC_CALL_NULL(trusted_key_exit, *trusted_key_sources[0].ops->exit);
+static void (*trusted_key_exit)(void);
static unsigned char migratable;
enum {
@@ -328,8 +328,7 @@ static int __init init_trusted(void)
trusted_key_sources[i].ops->unseal);
static_call_update(trusted_key_get_random,
trusted_key_sources[i].ops->get_random);
- static_call_update(trusted_key_exit,
- trusted_key_sources[i].ops->exit);
+ trusted_key_exit = trusted_key_sources[i].ops->exit;
migratable = trusted_key_sources[i].ops->migratable;
ret = static_call(trusted_key_init)();
@@ -349,7 +348,8 @@ static int __init init_trusted(void)
static void __exit cleanup_trusted(void)
{
- static_call(trusted_key_exit)();
+ if (trusted_key_exit)
+ trusted_key_exit();
}
late_initcall(init_trusted);
-Sumit
>
> ---
> Subject: objtool,static_call: Don't emit static_call_site for .exit.text
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Mar 17 13:35:05 CET 2021
>
> Functions marked __exit are (somewhat surprisingly) discarded at
> runtime when built-in. This means that static_call(), when used in
> __exit functions, will generate static_call_site entries that point
> into reclaimed space.
>
> Simply skip such sites and emit a WARN about it. By not emitting a
> static_call_site the site will remain pointed at the trampoline, which
> is also maintained, so things will work as expected, albeit with the
> extra indirection.
>
> The WARN is so that people are aware of this; and arguably it simply
> isn't a good idea to use static_call() in __exit code anyway, since
> module unload is never a performance critical path.
>
> Reported-by: Sumit Garg <sumit.garg@linaro.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Tested-by: Sumit Garg <sumit.garg@linaro.org>
> ---
> tools/objtool/check.c | 32 ++++++++++++++++++++------------
> 1 file changed, 20 insertions(+), 12 deletions(-)
>
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -850,6 +850,22 @@ static int add_ignore_alternatives(struc
> return 0;
> }
>
> +static inline void static_call_add(struct instruction *insn,
> + struct objtool_file *file)
> +{
> + if (!insn->call_dest->static_call_tramp)
> + return;
> +
> + if (!strcmp(insn->sec->name, ".exit.text")) {
> + WARN_FUNC("static_call in .exit.text, skipping inline patching",
> + insn->sec, insn->offset);
> + return;
> + }
> +
> + list_add_tail(&insn->static_call_node,
> + &file->static_call_list);
> +}
> +
> /*
> * Find the destination instructions for all jumps.
> */
> @@ -888,10 +904,7 @@ static int add_jump_destinations(struct
> } else if (insn->func) {
> /* internal or external sibling call (with reloc) */
> insn->call_dest = reloc->sym;
> - if (insn->call_dest->static_call_tramp) {
> - list_add_tail(&insn->static_call_node,
> - &file->static_call_list);
> - }
> + static_call_add(insn, file);
> continue;
> } else if (reloc->sym->sec->idx) {
> dest_sec = reloc->sym->sec;
> @@ -950,10 +963,7 @@ static int add_jump_destinations(struct
>
> /* internal sibling call (without reloc) */
> insn->call_dest = insn->jump_dest->func;
> - if (insn->call_dest->static_call_tramp) {
> - list_add_tail(&insn->static_call_node,
> - &file->static_call_list);
> - }
> + static_call_add(insn, file);
> }
> }
> }
> @@ -2746,10 +2756,8 @@ static int validate_branch(struct objtoo
> if (dead_end_function(file, insn->call_dest))
> return 0;
>
> - if (insn->type == INSN_CALL && insn->call_dest->static_call_tramp) {
> - list_add_tail(&insn->static_call_node,
> - &file->static_call_list);
> - }
> + if (insn->type == INSN_CALL)
> + static_call_add(insn, file);
>
> break;
>
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] objtool,static_call: Don't emit static_call_site for .exit.text
2021-03-17 13:37 ` Sumit Garg
@ 2021-03-17 21:55 ` Jarkko Sakkinen
2021-03-18 4:42 ` Sumit Garg
0 siblings, 1 reply; 9+ messages in thread
From: Jarkko Sakkinen @ 2021-03-17 21:55 UTC (permalink / raw)
To: Sumit Garg
Cc: Peter Zijlstra, Oliver Sang, Josh Poimboeuf, jbaron, lkp,
kbuild test robot, Linux Kernel Mailing List
On Wed, Mar 17, 2021 at 07:07:07PM +0530, Sumit Garg wrote:
> On Wed, 17 Mar 2021 at 18:16, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Wed, Mar 17, 2021 at 05:25:48PM +0530, Sumit Garg wrote:
> > > Thanks Peter for this fix. It does work for me on qemu for x86. Can
> > > you turn this into a proper fix patch? BTW, feel free to add:
> >
> > Per the below, the original patch ought to be fixed as well, to not use
> > static_call() in __exit.
>
> Okay, fair enough.
>
> Jarkko,
>
> Can you please incorporate the following change to the original patch as well?
Can you roll-out a proper patch of this?
/Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] objtool,static_call: Don't emit static_call_site for .exit.text
2021-03-17 21:55 ` Jarkko Sakkinen
@ 2021-03-18 4:42 ` Sumit Garg
0 siblings, 0 replies; 9+ messages in thread
From: Sumit Garg @ 2021-03-18 4:42 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Peter Zijlstra, Oliver Sang, Josh Poimboeuf, jbaron, lkp,
kbuild test robot, Linux Kernel Mailing List
On Thu, 18 Mar 2021 at 03:26, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Wed, Mar 17, 2021 at 07:07:07PM +0530, Sumit Garg wrote:
> > On Wed, 17 Mar 2021 at 18:16, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Wed, Mar 17, 2021 at 05:25:48PM +0530, Sumit Garg wrote:
> > > > Thanks Peter for this fix. It does work for me on qemu for x86. Can
> > > > you turn this into a proper fix patch? BTW, feel free to add:
> > >
> > > Per the below, the original patch ought to be fixed as well, to not use
> > > static_call() in __exit.
> >
> > Okay, fair enough.
> >
> > Jarkko,
> >
> > Can you please incorporate the following change to the original patch as well?
>
> Can you roll-out a proper patch of this?
Okay, I will post a separate patch for this.
-Sumit
>
> /Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] objtool,static_call: Don't emit static_call_site for .exit.text
2021-03-17 12:45 ` [PATCH] objtool,static_call: Don't emit static_call_site for .exit.text Peter Zijlstra
2021-03-17 13:37 ` Sumit Garg
@ 2021-03-17 21:01 ` Jarkko Sakkinen
2021-03-18 0:02 ` Josh Poimboeuf
2 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2021-03-17 21:01 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Sumit Garg, Oliver Sang, Josh Poimboeuf, jbaron, lkp,
kbuild test robot, linux-kernel
On Wed, Mar 17, 2021 at 01:45:57PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 17, 2021 at 05:25:48PM +0530, Sumit Garg wrote:
> > Thanks Peter for this fix. It does work for me on qemu for x86. Can
> > you turn this into a proper fix patch? BTW, feel free to add:
>
> Per the below, the original patch ought to be fixed as well, to not use
> static_call() in __exit.
>
> ---
> Subject: objtool,static_call: Don't emit static_call_site for .exit.text
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Mar 17 13:35:05 CET 2021
>
> Functions marked __exit are (somewhat surprisingly) discarded at
> runtime when built-in. This means that static_call(), when used in
> __exit functions, will generate static_call_site entries that point
> into reclaimed space.
>
> Simply skip such sites and emit a WARN about it. By not emitting a
> static_call_site the site will remain pointed at the trampoline, which
> is also maintained, so things will work as expected, albeit with the
> extra indirection.
>
> The WARN is so that people are aware of this; and arguably it simply
> isn't a good idea to use static_call() in __exit code anyway, since
> module unload is never a performance critical path.
>
> Reported-by: Sumit Garg <sumit.garg@linaro.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Tested-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
/Jarkko
> ---
> tools/objtool/check.c | 32 ++++++++++++++++++++------------
> 1 file changed, 20 insertions(+), 12 deletions(-)
>
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -850,6 +850,22 @@ static int add_ignore_alternatives(struc
> return 0;
> }
>
> +static inline void static_call_add(struct instruction *insn,
> + struct objtool_file *file)
> +{
> + if (!insn->call_dest->static_call_tramp)
> + return;
> +
> + if (!strcmp(insn->sec->name, ".exit.text")) {
> + WARN_FUNC("static_call in .exit.text, skipping inline patching",
> + insn->sec, insn->offset);
> + return;
> + }
> +
> + list_add_tail(&insn->static_call_node,
> + &file->static_call_list);
> +}
> +
> /*
> * Find the destination instructions for all jumps.
> */
> @@ -888,10 +904,7 @@ static int add_jump_destinations(struct
> } else if (insn->func) {
> /* internal or external sibling call (with reloc) */
> insn->call_dest = reloc->sym;
> - if (insn->call_dest->static_call_tramp) {
> - list_add_tail(&insn->static_call_node,
> - &file->static_call_list);
> - }
> + static_call_add(insn, file);
> continue;
> } else if (reloc->sym->sec->idx) {
> dest_sec = reloc->sym->sec;
> @@ -950,10 +963,7 @@ static int add_jump_destinations(struct
>
> /* internal sibling call (without reloc) */
> insn->call_dest = insn->jump_dest->func;
> - if (insn->call_dest->static_call_tramp) {
> - list_add_tail(&insn->static_call_node,
> - &file->static_call_list);
> - }
> + static_call_add(insn, file);
> }
> }
> }
> @@ -2746,10 +2756,8 @@ static int validate_branch(struct objtoo
> if (dead_end_function(file, insn->call_dest))
> return 0;
>
> - if (insn->type == INSN_CALL && insn->call_dest->static_call_tramp) {
> - list_add_tail(&insn->static_call_node,
> - &file->static_call_list);
> - }
> + if (insn->type == INSN_CALL)
> + static_call_add(insn, file);
>
> break;
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] objtool,static_call: Don't emit static_call_site for .exit.text
2021-03-17 12:45 ` [PATCH] objtool,static_call: Don't emit static_call_site for .exit.text Peter Zijlstra
2021-03-17 13:37 ` Sumit Garg
2021-03-17 21:01 ` Jarkko Sakkinen
@ 2021-03-18 0:02 ` Josh Poimboeuf
2021-03-18 7:59 ` Peter Zijlstra
2 siblings, 1 reply; 9+ messages in thread
From: Josh Poimboeuf @ 2021-03-18 0:02 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Sumit Garg, Oliver Sang, jbaron, lkp, kbuild test robot,
Jarkko Sakkinen, linux-kernel
On Wed, Mar 17, 2021 at 01:45:57PM +0100, Peter Zijlstra wrote:
> arguably it simply isn't a good idea to use static_call() in __exit
> code anyway, since module unload is never a performance critical path.
Couldn't you make the same argument about __init functions, which are
allowed to do static calls?
We might consider a STATIC_CALL_SITE_EXIT flag, but I suppose we've run
out of flag space.
--
Josh
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] objtool,static_call: Don't emit static_call_site for .exit.text
2021-03-18 0:02 ` Josh Poimboeuf
@ 2021-03-18 7:59 ` Peter Zijlstra
2021-03-18 8:30 ` Peter Zijlstra
0 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2021-03-18 7:59 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Sumit Garg, Oliver Sang, jbaron, lkp, kbuild test robot,
Jarkko Sakkinen, linux-kernel
On Wed, Mar 17, 2021 at 07:02:12PM -0500, Josh Poimboeuf wrote:
> On Wed, Mar 17, 2021 at 01:45:57PM +0100, Peter Zijlstra wrote:
> > arguably it simply isn't a good idea to use static_call() in __exit
> > code anyway, since module unload is never a performance critical path.
>
> Couldn't you make the same argument about __init functions, which are
> allowed to do static calls?
I suppose we could indeed make that argument. Much of that code was
copied from jump_label without much consideration. And I now I suppose
I'll have to consider jump_label in __exit too :/
> We might consider a STATIC_CALL_SITE_EXIT flag, but I suppose we've run
> out of flag space.
Yeah, we're definitely short on flags. Let me try and figure out when
exactly it's all discarded.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] objtool,static_call: Don't emit static_call_site for .exit.text
2021-03-18 7:59 ` Peter Zijlstra
@ 2021-03-18 8:30 ` Peter Zijlstra
2021-03-18 8:47 ` Peter Zijlstra
0 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2021-03-18 8:30 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Sumit Garg, Oliver Sang, jbaron, lkp, kbuild test robot,
Jarkko Sakkinen, linux-kernel
On Thu, Mar 18, 2021 at 08:59:45AM +0100, Peter Zijlstra wrote:
> On Wed, Mar 17, 2021 at 07:02:12PM -0500, Josh Poimboeuf wrote:
> > On Wed, Mar 17, 2021 at 01:45:57PM +0100, Peter Zijlstra wrote:
> > > arguably it simply isn't a good idea to use static_call() in __exit
> > > code anyway, since module unload is never a performance critical path.
> >
> > Couldn't you make the same argument about __init functions, which are
> > allowed to do static calls?
>
> I suppose we could indeed make that argument. Much of that code was
> copied from jump_label without much consideration. And I now I suppose
> I'll have to consider jump_label in __exit too :/
>
> > We might consider a STATIC_CALL_SITE_EXIT flag, but I suppose we've run
> > out of flag space.
>
> Yeah, we're definitely short on flags. Let me try and figure out when
> exactly it's all discarded.
Ha!, x86 stuffs .exit.text in [__init_begin, __init_end) and it is
discarded right along with initmem.
But that means it should match init and be tagged init and all *should*
work, but somehow it doesn't... clearly I'm missing something again
ARGH!
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] objtool,static_call: Don't emit static_call_site for .exit.text
2021-03-18 8:30 ` Peter Zijlstra
@ 2021-03-18 8:47 ` Peter Zijlstra
0 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2021-03-18 8:47 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Sumit Garg, Oliver Sang, jbaron, lkp, kbuild test robot,
Jarkko Sakkinen, linux-kernel
On Thu, Mar 18, 2021 at 09:30:18AM +0100, Peter Zijlstra wrote:
> On Thu, Mar 18, 2021 at 08:59:45AM +0100, Peter Zijlstra wrote:
> > On Wed, Mar 17, 2021 at 07:02:12PM -0500, Josh Poimboeuf wrote:
> > > On Wed, Mar 17, 2021 at 01:45:57PM +0100, Peter Zijlstra wrote:
> > > > arguably it simply isn't a good idea to use static_call() in __exit
> > > > code anyway, since module unload is never a performance critical path.
> > >
> > > Couldn't you make the same argument about __init functions, which are
> > > allowed to do static calls?
> >
> > I suppose we could indeed make that argument. Much of that code was
> > copied from jump_label without much consideration. And I now I suppose
> > I'll have to consider jump_label in __exit too :/
> >
> > > We might consider a STATIC_CALL_SITE_EXIT flag, but I suppose we've run
> > > out of flag space.
> >
> > Yeah, we're definitely short on flags. Let me try and figure out when
> > exactly it's all discarded.
>
> Ha!, x86 stuffs .exit.text in [__init_begin, __init_end) and it is
> discarded right along with initmem.
>
> But that means it should match init and be tagged init and all *should*
> work, but somehow it doesn't... clearly I'm missing something again
> ARGH!
I found a race, look at this:
kernel_init()
...
free_initmem();
...
system_state = SYSTEM_RUNNING;
vs
__static_call_update()
...
if (static_call_is_init()) {
if (system_state >= SYSTEM_RUNNING)
continue;
}
And this is *after* SMP bringup. Somehow I don't think you hit this
race, it is extremely unlikely
(jump_label has the exact same issue fwiw)
^ permalink raw reply [flat|nested] 9+ messages in thread