Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211109164549.1724710-1-ardb@kernel.org> <20211109164549.1724710-3-ardb@kernel.org>
 <YYrCn3mc+EbY+OB/@hirez.programming.kicks-ass.net>
In-Reply-To: <YYrCn3mc+EbY+OB/@hirez.programming.kicks-ass.net>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Tue, 9 Nov 2021 19:53:33 +0100
Message-ID: <CAMj1kXHArfyvZ-b11vLshpj84drDtuU+T_o+h+cWqhtajpon0A@mail.gmail.com>
Subject: Re: [RFC PATCH 2/7] static_call: deal with unexported keys without
 cluttering up the API
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Jason Baron <jbaron@akamai.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 9 Nov 2021 at 19:49, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Nov 09, 2021 at 05:45:44PM +0100, Ard Biesheuvel wrote:
>
> > diff --git a/include/linux/static_call_types.h b/include/linux/static_call_types.h
> > index 5a00b8b2cf9f..0bb36294cce7 100644
> > --- a/include/linux/static_call_types.h
> > +++ b/include/linux/static_call_types.h
> > @@ -32,15 +32,20 @@
> >  struct static_call_site {
> >       s32 addr;
> >       s32 key;
> > +     s32 tramp;
> >  };
>
> I can't say I'm thrilled at growing this thing, but the cleanup is nice.
> Perhaps we can increase alignment on struct static_call_key and instead
> frob it in .key still?
>

This is already a place-relative field, and one points into the data
section and the other into text. So I don't see how we can squeeze
enough bits out of it to make this fit.

> >
> >  #define DECLARE_STATIC_CALL(name, func)                                      \
> > -     extern struct static_call_key STATIC_CALL_KEY(name);            \
> > +     extern __weak struct static_call_key STATIC_CALL_KEY(name);     \
> >       extern typeof(func) STATIC_CALL_TRAMP(name);
>
> I'm a little bit confused on how this actually works. What does a __weak
> extern data symbol do?
>
> A __weak function definition would create a module local instance of the
> function barring a strong override.
>
> But what does a __weak extern do?
>

It is simply a reference that is permitted to remain undefined.

That is why (in another patch) I do sth like

extern __weak foo;

if (&foo)
  ... use foo
else
   ... use sth else


> > diff --git a/kernel/static_call.c b/kernel/static_call.c
> > index 43ba0b1e0edb..360cc3cd0fbf 100644
> > --- a/kernel/static_call.c
> > +++ b/kernel/static_call.c
> > @@ -366,18 +366,18 @@ static int static_call_add_module(struct module *mod)
> >                * means modules are allowed to call static_call_update() on
> >                * it.
> >                *
> > -              * Otherwise, the key isn't exported, and 'addr' points to the
> > +              * Otherwise, the key isn't exported, and 'tramp' points to the
> >                * trampoline so we need to lookup the key.
> >                *
> >                * We go through this dance to prevent crazy modules from
> >                * abusing sensitive static calls.
> >                */
> > -             if (!kernel_text_address(addr))
> > +             if (addr)
> >                       continue;
>
> This seems to imply that the __weak extern symbol gets resolved to 0 at
> module link time.
>

Yes, if the referenced symbol is not exported, it just uses 0x0 for its value.

> >
> > -             key = tramp_key_lookup(addr);
> > +             key = tramp_key_lookup((unsigned long)offset_to_ptr(&site->tramp));
> >               if (!key) {
> > -                     pr_warn("Failed to fixup __raw_static_call() usage at: %ps\n",
> > +                     pr_warn("Failed to fixup static_call() usage at: %ps\n",
> >                               static_call_addr(site));
> >                       return -EINVAL;
>
> >               }