On Wed, 17 Mar 2021 at 18:16, Peter Zijlstra <pet...@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 <pet...@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.g...@linaro.org>
> Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
> Tested-by: Sumit Garg <sumit.g...@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;
>
