Message-ID: <CAKv+Gu9bMkFEx_GxH3uLm0gveQurp7ELDi6C5NvpugufPFMbRA@mail.gmail.com>
Date:   Fri, 9 Nov 2018 14:39:17 +0100
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Jason Baron <jbaron@...mai.com>, Jiri Kosina <jkosina@...e.cz>,
        David Laight <David.Laight@...lab.com>,
        Borislav Petkov <bp@...en8.de>
Subject: Re: [RFC PATCH 1/3] static_call: Add static call infrastructure

On 8 November 2018 at 22:15, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> Add a static call infrastructure.  Static calls use code patching to
> hard-code function pointers into direct branch instructions.  They give
> the flexibility of function pointers, but with improved performance.
> This is especially important for cases where retpolines would otherwise
> be used, as retpolines can significantly impact performance.
>
> This code is heavily inspired by the jump label code (aka "static
> jumps"), as some of the concepts are very similar.
>
> There are three implementations, depending on arch support:
>
> 1) optimized: patched call sites (CONFIG_HAVE_STATIC_CALL_OPTIMIZED)
> 2) unoptimized: patched trampolines (CONFIG_HAVE_STATIC_CALL_UNOPTIMIZED)
> 3) basic function pointers
>
> For more details, see the comments in include/linux/static_call.h.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
> ---
>  arch/Kconfig                      |   6 +
>  include/asm-generic/vmlinux.lds.h |  11 ++
>  include/linux/module.h            |  10 +
>  include/linux/static_call.h       | 186 +++++++++++++++++++
>  include/linux/static_call_types.h |  19 ++
>  kernel/Makefile                   |   1 +
>  kernel/module.c                   |   5 +
>  kernel/static_call.c              | 297 ++++++++++++++++++++++++++++++
>  8 files changed, 535 insertions(+)
>  create mode 100644 include/linux/static_call.h
>  create mode 100644 include/linux/static_call_types.h
>  create mode 100644 kernel/static_call.c
>
> diff --git a/kernel/static_call.c b/kernel/static_call.c
> new file mode 100644
> index 000000000000..599ebc6fc4f1
> --- /dev/null
> +++ b/kernel/static_call.c
...
> +static void __init static_call_init(void)
> +{
> +       struct static_call_site *start = __start_static_call_sites;
> +       struct static_call_site *stop  = __stop_static_call_sites;
> +       struct static_call_site *site;
> +
> +       if (start == stop) {
> +               pr_warn("WARNING: empty static call table\n");
> +               return;
> +       }
> +
> +       cpus_read_lock();
> +       static_call_lock();
> +
> +       static_call_sort_entries(start, stop);
> +
> +       for (site = start; site < stop; site++) {
> +               struct static_call_key *key = static_call_key(site);
> +               unsigned long addr = static_call_addr(site);
> +
> +               if (list_empty(&key->site_mods)) {
> +                       struct static_call_mod *mod;
> +
> +                       mod = kzalloc(sizeof(*mod), GFP_KERNEL);
> +                       if (!mod) {
> +                               WARN(1, "Failed to allocate memory for static calls");
> +                               return;
> +                       }
> +
> +                       mod->sites = site;
> +                       list_add_tail(&mod->list, &key->site_mods);
> +
> +                       /*
> +                        * The trampoline should no longer be used.  Poison it
> +                        * it with a BUG() to catch any stray callers.
> +                        */
> +                       arch_static_call_poison_tramp(addr);

This patches the wrong thing: the trampoline is at key->func not addr.

However, patching it here means we poison it before all users are
patched. I added this on top

diff --git a/kernel/static_call.c b/kernel/static_call.c
index 599ebc6fc4f1..d9562329bec6 100644
--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -248,6 +248,7 @@ static void __init static_call_init(void)
        struct static_call_site *start = __start_static_call_sites;
        struct static_call_site *stop  = __stop_static_call_sites;
        struct static_call_site *site;
+       struct static_call_key *prev_key = NULL;

        if (start == stop) {
                pr_warn("WARNING: empty static call table\n");
@@ -279,7 +280,9 @@ static void __init static_call_init(void)
                         * The trampoline should no longer be used.  Poison it
                         * it with a BUG() to catch any stray callers.
                         */
-                       arch_static_call_poison_tramp(addr);
+                       if (prev_key)
+
arch_static_call_poison_tramp((unsigned long)prev_key->func);
+                       prev_key = key;
                }

                arch_static_call_transform(addr, key->func);


> +               }
> +
> +               arch_static_call_transform(addr, key->func);
> +       }
> +
> +       static_call_initialized = true;
> +
> +       static_call_unlock();
> +       cpus_read_unlock();
> +
> +#ifdef CONFIG_MODULES
> +       register_module_notifier(&static_call_module_nb);
> +#endif
> +}
> +early_initcall(static_call_init);
> --
> 2.17.2
>

Powered by blists - more mailing lists