* Re: [PATCH 03/13] livepatch: refactor klp_init_object
From: Petr Mladek @ 2021-01-27 12:58 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Frederic Barrat, Andrew Donnellan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Jessica Yu, Josh Poimboeuf, Jiri Kosina, Miroslav Benes,
Joe Lawrence, Masahiro Yamada, Michal Marek, linux-kernel,
linuxppc-dev, dri-devel, live-patching, linux-kbuild
In-Reply-To: <20210121074959.313333-4-hch@lst.de>
On Thu 2021-01-21 08:49:49, Christoph Hellwig wrote:
> Merge three calls to klp_is_module (including one hidden inside
> klp_find_object_module) into a single one to simplify the code a bit.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> kernel/livepatch/core.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index f76fdb9255323d..a7f625dc24add3 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -54,9 +54,6 @@ static void klp_find_object_module(struct klp_object *obj)
> {
> struct module *mod;
>
> - if (!klp_is_module(obj))
> - return;
> -
We need to either update the function description or keep this check.
I prefer to keep the check. The function does the right thing also
for the object "vmlinux". Also the livepatch code includes many
similar paranoid checks that makes the code less error prone
against any further changes.
Of course, it is a matter of taste.
> mutex_lock(&module_mutex);
> /*
> * We do not want to block removal of patched modules and therefore
Otherwise, the patch looks fine.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH] stacktrace: Move documentation for arch_stack_walk_reliable() to header
From: Vasily Gorbik @ 2021-01-27 12:27 UTC (permalink / raw)
To: Mark Brown
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, Heiko Carstens, Christian Borntraeger,
Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, x86, linux-s390, live-patching
In-Reply-To: <20210118211021.42308-1-broonie@kernel.org>
On Mon, Jan 18, 2021 at 09:10:21PM +0000, Mark Brown wrote:
> Currently arch_stack_wallk_reliable() is documented with an identical
> comment in both x86 and S/390 implementations which is a bit redundant.
> Move this to the header and convert to kerneldoc while we're at it.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: Miroslav Benes <mbenes@suse.cz>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Joe Lawrence <joe.lawrence@redhat.com>
> Cc: x86@kernel.org
> Cc: linux-s390@vger.kernel.org
> Cc: live-patching@vger.kernel.org
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> arch/s390/kernel/stacktrace.c | 6 ------
> arch/x86/kernel/stacktrace.c | 6 ------
> include/linux/stacktrace.h | 19 +++++++++++++++++++
> 3 files changed, 19 insertions(+), 12 deletions(-)
Acked-by: Vasily Gorbik <gor@linux.ibm.com>
^ permalink raw reply
* Re: [PATCH 04/13] livepatch: move klp_find_object_module to module.c
From: Petr Mladek @ 2021-01-27 11:55 UTC (permalink / raw)
To: Jessica Yu
Cc: Christoph Hellwig, Frederic Barrat, Andrew Donnellan,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Daniel Vetter, Josh Poimboeuf, Jiri Kosina, Miroslav Benes,
Joe Lawrence, Masahiro Yamada, Michal Marek, linux-kernel,
linuxppc-dev, dri-devel, live-patching, linux-kbuild
In-Reply-To: <YBAmTAsT3S01kU1x@gunter>
On Tue 2021-01-26 15:25:16, Jessica Yu wrote:
> +++ Christoph Hellwig [21/01/21 08:49 +0100]:
> > To uncouple the livepatch code from module loader internals move a
> > slightly refactored version of klp_find_object_module to module.c
> > This allows to mark find_module static and removes one of the last
> > users of module_mutex outside of module.c.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> > include/linux/module.h | 3 +--
> > kernel/livepatch/core.c | 39 +++++++++++++--------------------------
> > kernel/module.c | 17 ++++++++++++++++-
> > 3 files changed, 30 insertions(+), 29 deletions(-)
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index b4654f8a408134..8588482bde4116 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -586,8 +586,7 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
> > return within_module_init(addr, mod) || within_module_core(addr, mod);
> > }
> >
> > -/* Search for module by name: must hold module_mutex. */
> > -struct module *find_module(const char *name);
> > +struct module *find_klp_module(const char *name);
> >
> > /* Check if a module is loaded. */
> > bool module_loaded(const char *name);
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index a7f625dc24add3..878759baadd81c 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -49,30 +49,6 @@ static bool klp_is_module(struct klp_object *obj)
> > return obj->name;
> > }
> >
> > -/* sets obj->mod if object is not vmlinux and module is found */
> > -static void klp_find_object_module(struct klp_object *obj)
> > -{
> > - struct module *mod;
> > -
> > - mutex_lock(&module_mutex);
> > - /*
> > - * We do not want to block removal of patched modules and therefore
> > - * we do not take a reference here. The patches are removed by
> > - * klp_module_going() instead.
> > - */
> > - mod = find_module(obj->name);
> > - /*
> > - * Do not mess work of klp_module_coming() and klp_module_going().
> > - * Note that the patch might still be needed before klp_module_going()
> > - * is called. Module functions can be called even in the GOING state
> > - * until mod->exit() finishes. This is especially important for
> > - * patches that modify semantic of the functions.
> > - */
> > - if (mod && mod->klp_alive)
> > - obj->mod = mod;
> > - mutex_unlock(&module_mutex);
> > -}
>
> Hmm, I am not a huge fan of moving more livepatch code into module.c, I
> wonder if we can keep them separate.
>
> Why not have module_is_loaded() kill two birds with one stone? That
> is, just have it return a module pointer to signify that the module is
> loaded, NULL if not. Then we don't need an extra find_klp_module()
> function just to call find_module() and return a pointer, as
> module_is_loaded() can just do that for us.
>
> As for the mod->klp_alive check, I believe this function
> (klp_find_object_module()) is called with klp_mutex held, and
> mod->klp_alive is only modified under klp_mutex. Also, if klp_alive is
> true, the module is at least COMING and cannot be GOING until it
> acquires the klp_mutex again in klp_module_going(). So does that hunk
> really need to be under module_mutex? It has been a long time since
> I've looked at livepatch code so it would be great if someone could
> double check.
We need to make sure that the module is not freed before we manipulate
mod->klp_alive.
One solution would be to take the reference and block it during this
operation.
Alternatively it might be to rely on RCU. It seems that the struct
is protected by RCU because of kallsyms. But I am not sure if it
is safe in all module states. But it should be. We find the module
via the same list like kallsyms.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH] stacktrace: Move documentation for arch_stack_walk_reliable() to header
From: Miroslav Benes @ 2021-01-27 10:45 UTC (permalink / raw)
To: Mark Brown
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Josh Poimboeuf, Jiri Kosina, Petr Mladek,
Joe Lawrence, x86, linux-s390, live-patching
In-Reply-To: <20210118211021.42308-1-broonie@kernel.org>
On Mon, 18 Jan 2021, Mark Brown wrote:
> Currently arch_stack_wallk_reliable() is documented with an identical
> comment in both x86 and S/390 implementations which is a bit redundant.
> Move this to the header and convert to kerneldoc while we're at it.
Makes sense.
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: Miroslav Benes <mbenes@suse.cz>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Joe Lawrence <joe.lawrence@redhat.com>
> Cc: x86@kernel.org
> Cc: linux-s390@vger.kernel.org
> Cc: live-patching@vger.kernel.org
> Signed-off-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
> diff --git a/arch/s390/kernel/stacktrace.c b/arch/s390/kernel/stacktrace.c
> index 7f1266c24f6b..101477b3e263 100644
> --- a/arch/s390/kernel/stacktrace.c
> +++ b/arch/s390/kernel/stacktrace.c
> @@ -24,12 +24,6 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
> }
> }
>
> -/*
> - * This function returns an error if it detects any unreliable features of the
> - * stack. Otherwise it guarantees that the stack trace is reliable.
> - *
> - * If the task is not 'current', the caller *must* ensure the task is inactive.
> - */
> int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
> void *cookie, struct task_struct *task)
> {
Just a note. arch/powerpc/kernel/stacktrace.c has the same for
__save_stack_trace_tsk_reliable(), but it would not be nice to pollute
include/linux/stacktrace.h with that in my opinion. It is an old
infrastructure after all.
Miroslav
^ permalink raw reply
* Re: [PATCH v6 0/2] Documentation: livepatch: Document reliable stacktrace and minor cleanup
From: Miroslav Benes @ 2021-01-27 10:32 UTC (permalink / raw)
To: Mark Brown
Cc: linux-kernel, Mark Rutland, Jiri Kosina, Joe Lawrence,
Jonathan Corbet, Petr Mladek, Josh Poimboeuf, linux-doc,
live-patching
In-Reply-To: <20210120164714.16581-1-broonie@kernel.org>
Hi,
On Wed, 20 Jan 2021, Mark Brown wrote:
> This series adds a document, mainly written by Mark Rutland, which makes
> explicit the requirements for implementing reliable stacktrace in order
> to aid architectures adding this feature. It also updates the other
> livepatching documents to use automatically generated tables of contents
> following review comments on Mark's document.
>
> v6:
> - Remove a duplicated "points".
> v5:
> - Tweaks to the commit message for the new document.
> - Convert new and existing documents to autogenerated tables of
> contents.
> v4:
> - Renumber table of contents
> v3:
> - Incorporated objtool section from Mark.
> - Deleted confusing notes about using annotations.
>
> Mark Brown (1):
> Documentation: livepatch: Convert to automatically generated contents
>
> Mark Rutland (1):
> Documentation: livepatch: document reliable stacktrace
>
> Documentation/livepatch/index.rst | 1 +
> Documentation/livepatch/livepatch.rst | 15 +-
> Documentation/livepatch/module-elf-format.rst | 10 +-
> .../livepatch/reliable-stacktrace.rst | 309 ++++++++++++++++++
> 4 files changed, 313 insertions(+), 22 deletions(-)
> create mode 100644 Documentation/livepatch/reliable-stacktrace.rst
sorry for the late reply (slowly crawling through my email backlog).
Thanks a lot for putting this together!
FWIW (so it is at least archived in the thread)
Acked-by: Miroslav Benes <mbenes@suse.cz>
M
^ permalink raw reply
* Re: Live patching on ARM64
From: Madhavan T. Venkataraman @ 2021-01-26 18:03 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Mark Brown, Julien Thierry, jpoimboe,
live-patching, linux-kernel
In-Reply-To: <20210115123347.GB39776@C02TD0UTHF1T.local>
Hi all,
Mark Rutland had sent me some ideas on what work is pending for ARM64 live patching.
I sent some questions to Mark Rutland. I forgot to include everyone in the email.
Sorry about that. I have reproduced my questions and his responses below. Please
chime in with any comments:
Thanks!
On Mon, Jan 25, 2021 at 11:58:47AM -0600, Madhavan T. Venkataraman wrote:
> Some questions below:
I've answered thos below.
If possible, I'd prefer to handle future queries on a public list (so
that others can chime in, and so that it gets archived), so if you could
direct further questions to a thread on LAKML, that would be much
appreciated.
> On 1/15/21 6:33 AM, Mark Rutland wrote:
> [...]
>>
>> One general thing that I believe we'll need to do is to rework code to
>> be patch-safe (which implies being noinstr-safe too). For example, we'll
>> need to rework the instruction patching code such that this cannot end
>> up patching itself (or anything that has instrumented it) in an unsafe
>> way.
>>
>
> OK. I understand that. Are there are other scenarios that make patching
> unsafe?
I suspect so; these are simply the cases I'm immediately aware of. I
suspect there are other cases that we will need to consider that don't
immediately spring to mind.
> I expect the kernel already handles scenarios such as two CPUs patching
> the same location at the same time or a thread executing at a location that is
> currently being patched.
IIRC that is supposed to be catered for by ftrace (and so I assume for
livepatching too); I'm not certain about kprobes. In addition to
synchronization in the core ftrace code, arm64's ftrace_modify_code()
has a sanity-check with a non-atomic RMW sequence. We might be able to
make that more robust wiuth a faultable cmpxchg, and some changes around
ftrace_update_ftrace_func() and ftrace_make_nop() to get rid of the
unvalidated cases.
> Any other scenarios to be considered?
I'm not immediately aware of others, but suspect more cases will become
apparent as work progresses on the bits we already know about.
>> Once we have objtool it should be possible to identify those cases
>> automatically. Currently I'm aware that we'll need to do something in at
>> least the following places:
>>
>
> OK. AFAIK, objtool checks for the following:
>
> - returning from noinstr function with instrumentation enabled
>
> - calling instrumentable functions from noinstr code without:
>
> instrumentation_begin();
> instrumentation_end();
>
> Is that what you mean?
That's what I was thinking of, yes -- this should highlight some places
that will need attention.
> Does objtool check other things as well that is relevant to (un)safe
> patching?
I'm not entirely familiar with objtool, so I'm not exactly sure what it
can do; I expect Josh and Julien can give more detail here.
>> * The insn framework (which is used by some patching code), since the
>> bulk of it lives in arch/arm64/kernel/insn.c and isn't marked noinstr.
>>
>> We can probably shift the bulk of the aarch64_insn_gen_*() and
>> aarch64_get_*() helpers into a header as __always_inline functions,
>> which would allow them to be used in noinstr code. As those are
>> typically invoked with a number of constant arguments that the
>> compiler can fold, this /might/ work out as an optimization if the
>> compiler can elide the error paths.
>
> OK. I will take a look at the insn code.
IIRC Julien's objtool series had some patches had some patches moving
the insn code about, so it'd be worth checking whether that's a help or
a hindrance. If it's possible to split out a set of preparatory patches
that make that ready both for objtool and the kernel, that would make it
easier to review that and queue it early.
>> * The alternatives code, since we call instrumentable and patchable
>> functions between updating instructions and performing all the
>> necessary maintenance. There are a number of cases within
>> __apply_alternatives(), e.g.
>>
>> - test_bit()
>> - cpus_have_cap()
>> - pr_info_once()
>> - lm_alias()
>> - alt_cb, if the callback is not marked as noinstr, or if it calls
>> instrumentable code (e.g. from the insn framework).
>> - clean_dcache_range_nopatch(), as read_sanitised_ftr_reg() and
>> related code can be instrumented.
>>
>> This might need some underlying rework elsewhere (e.g. in the
>> cpufeature code, or atomics framework).
>>
>> So on the kernel side, maybe a first step would be to try to headerize
>> the insn generation code as __always_inline, and see whether that looks
>> ok? With that out of the way it'd be a bit easier to rework patching
>> code depending on the insn framework.
>
> OK. I will study this.
Great, thanks!
Mark.
^ permalink raw reply
* Re: [PATCH 04/13] livepatch: move klp_find_object_module to module.c
From: Jessica Yu @ 2021-01-26 14:25 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Frederic Barrat, Andrew Donnellan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, Masahiro Yamada, Michal Marek, linux-kernel,
linuxppc-dev, dri-devel, live-patching, linux-kbuild
In-Reply-To: <20210121074959.313333-5-hch@lst.de>
+++ Christoph Hellwig [21/01/21 08:49 +0100]:
>To uncouple the livepatch code from module loader internals move a
>slightly refactored version of klp_find_object_module to module.c
>This allows to mark find_module static and removes one of the last
>users of module_mutex outside of module.c.
>
>Signed-off-by: Christoph Hellwig <hch@lst.de>
>---
> include/linux/module.h | 3 +--
> kernel/livepatch/core.c | 39 +++++++++++++--------------------------
> kernel/module.c | 17 ++++++++++++++++-
> 3 files changed, 30 insertions(+), 29 deletions(-)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index b4654f8a408134..8588482bde4116 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -586,8 +586,7 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
> return within_module_init(addr, mod) || within_module_core(addr, mod);
> }
>
>-/* Search for module by name: must hold module_mutex. */
>-struct module *find_module(const char *name);
>+struct module *find_klp_module(const char *name);
>
> /* Check if a module is loaded. */
> bool module_loaded(const char *name);
>diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>index a7f625dc24add3..878759baadd81c 100644
>--- a/kernel/livepatch/core.c
>+++ b/kernel/livepatch/core.c
>@@ -49,30 +49,6 @@ static bool klp_is_module(struct klp_object *obj)
> return obj->name;
> }
>
>-/* sets obj->mod if object is not vmlinux and module is found */
>-static void klp_find_object_module(struct klp_object *obj)
>-{
>- struct module *mod;
>-
>- mutex_lock(&module_mutex);
>- /*
>- * We do not want to block removal of patched modules and therefore
>- * we do not take a reference here. The patches are removed by
>- * klp_module_going() instead.
>- */
>- mod = find_module(obj->name);
>- /*
>- * Do not mess work of klp_module_coming() and klp_module_going().
>- * Note that the patch might still be needed before klp_module_going()
>- * is called. Module functions can be called even in the GOING state
>- * until mod->exit() finishes. This is especially important for
>- * patches that modify semantic of the functions.
>- */
>- if (mod && mod->klp_alive)
>- obj->mod = mod;
>- mutex_unlock(&module_mutex);
>-}
Hmm, I am not a huge fan of moving more livepatch code into module.c, I
wonder if we can keep them separate.
Why not have module_is_loaded() kill two birds with one stone? That
is, just have it return a module pointer to signify that the module is
loaded, NULL if not. Then we don't need an extra find_klp_module()
function just to call find_module() and return a pointer, as
module_is_loaded() can just do that for us.
As for the mod->klp_alive check, I believe this function
(klp_find_object_module()) is called with klp_mutex held, and
mod->klp_alive is only modified under klp_mutex. Also, if klp_alive is
true, the module is at least COMING and cannot be GOING until it
acquires the klp_mutex again in klp_module_going(). So does that hunk
really need to be under module_mutex? It has been a long time since
I've looked at livepatch code so it would be great if someone could
double check.
Jessica
^ permalink raw reply
* Re: [PATCH v6 0/2] Documentation: livepatch: Document reliable stacktrace and minor cleanup
From: Jiri Kosina @ 2021-01-26 10:50 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Mark Brown, linux-kernel, Mark Rutland, Joe Lawrence,
Miroslav Benes, Petr Mladek, Josh Poimboeuf, linux-doc,
live-patching
In-Reply-To: <nycvar.YFH.7.76.2101221158450.5622@cbobk.fhfr.pm>
On Fri, 22 Jan 2021, Jiri Kosina wrote:
> > > This series adds a document, mainly written by Mark Rutland, which
> > > makes explicit the requirements for implementing reliable stacktrace
> > > in order to aid architectures adding this feature. It also updates
> > > the other livepatching documents to use automatically generated tables
> > > of contents following review comments on Mark's document.
> >
> > So...is this deemed ready and, if so, do you want it to go through the
> > docs tree or via some other path?
>
> I am planning to take it through livepatching tree unless there are any
> additional last-minutes comments.
Now applied to for-5.12/doc branch. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH 00/17] objtool: add base support for arm64
From: Josh Poimboeuf @ 2021-01-25 21:19 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Madhavan T. Venkataraman, Mark Brown, Mark Rutland, Michal Marek,
Julien Thierry, Peter Zijlstra, Catalin Marinas, Masahiro Yamada,
Linux Kernel Mailing List, linux-efi, linux-hardening,
live-patching, Will Deacon, Linux ARM, Kees Cook
In-Reply-To: <CAMj1kXFr0wvx-hG6nBY4ibju9ww4x0CGhQber3MZQ2ZZn9LHWw@mail.gmail.com>
On Fri, Jan 22, 2021 at 10:43:09PM +0100, Ard Biesheuvel wrote:
> On Fri, 22 Jan 2021 at 22:15, Madhavan T. Venkataraman <madvenka@linux.microsoft.com> wrote:
> > On 1/22/21 11:43 AM, Mark Brown wrote:
> > > On Thu, Jan 21, 2021 at 12:54:52PM -0600, Josh Poimboeuf wrote:
> > >
> > >> 2) The shadow stack idea sounds promising -- how hard would it be to
> > >> make a prototype reliable unwinder?
> > >
> > > In theory it doesn't look too hard and I can't see a particular reason
> > > not to try doing this - there's going to be edge cases but hopefully for
> > > reliable stack trace they're all in areas where we would be happy to
> > > just decide the stack isn't reliable anyway, things like nesting which
> > > allocates separate shadow stacks for each nested level for example.
> > > I'll take a look.
> > >
> >
> > I am a new comer to this discussion and I am learning. Just have some
> > questions. Pardon me if they are obvious or if they have already been
> > asked and answered.
> >
> > Doesn't Clang already have support for a shadow stack implementation for ARM64?
> > We could take a look at how Clang does it.
> >
> > Will there not be a significant performance hit? May be, some of it can be
> > mitigated by using a parallel shadow stack rather than a compact one.
> >
> > Are there any longjmp style situations in the kernel where the stack is
> > unwound by several frames? In these cases, the shadow stack must be unwound
> > accordingly.
> >
>
> Hello Madhavan,
>
> Let's discuss the details of shadow call stacks on a separate thread,
> instead of further hijacking Julien's series.
It's quite relevant to this thread. There's no need to consider merging
Julien's patches if you have a better approach. Why not discuss it
here? I'm also interested in the answers to Madhavan's questions.
--
Josh
^ permalink raw reply
* Re: [PATCH v4 00/10] Function Granular KASLR
From: Josh Poimboeuf @ 2021-01-25 17:21 UTC (permalink / raw)
To: Fangrui Song
Cc: Kristen Carlson Accardi, Miroslav Benes, Kees Cook, tglx, mingo,
bp, arjan, x86, linux-kernel, kernel-hardening, rick.p.edgecombe,
live-patching, Hongjiu Lu, joe.lawrence
In-Reply-To: <20210123225928.z5hkmaw6qjs2gu5g@google.com>
On Sat, Jan 23, 2021 at 02:59:28PM -0800, Fangrui Song wrote:
> On 2020-08-28, Josh Poimboeuf wrote:
> > On Fri, Aug 28, 2020 at 12:21:13PM +0200, Miroslav Benes wrote:
> > > > Hi there! I was trying to find a super easy way to address this, so I
> > > > thought the best thing would be if there were a compiler or linker
> > > > switch to just eliminate any duplicate symbols at compile time for
> > > > vmlinux. I filed this question on the binutils bugzilla looking to see
> > > > if there were existing flags that might do this, but H.J. Lu went ahead
> > > > and created a new one "-z unique", that seems to do what we would need
> > > > it to do.
> > > >
> > > > https://sourceware.org/bugzilla/show_bug.cgi?id=26391
> > > >
> > > > When I use this option, it renames any duplicate symbols with an
> > > > extension - for example duplicatefunc.1 or duplicatefunc.2. You could
> > > > either match on the full unique name of the specific binary you are
> > > > trying to patch, or you match the base name and use the extension to
> > > > determine original position. Do you think this solution would work?
> > >
> > > Yes, I think so (thanks, Joe, for testing!).
> > >
> > > It looks cleaner to me than the options above, but it may just be a matter
> > > of taste. Anyway, I'd go with full name matching, because -z unique-symbol
> > > would allow us to remove sympos altogether, which is appealing.
> > >
> > > > If
> > > > so, I can modify livepatch to refuse to patch on duplicated symbols if
> > > > CONFIG_FG_KASLR and when this option is merged into the tool chain I
> > > > can add it to KBUILD_LDFLAGS when CONFIG_FG_KASLR and livepatching
> > > > should work in all cases.
> > >
> > > Ok.
> > >
> > > Josh, Petr, would this work for you too?
> >
> > Sounds good to me. Kristen, thanks for finding a solution!
>
> (I am not subscribed. I came here via https://sourceware.org/bugzilla/show_bug.cgi?id=26391 (ld -z unique-symbol))
>
> > This works great after randomization because it always receives the
> > current address at runtime rather than relying on any kind of
> > buildtime address. The issue with with the live-patching code's
> > algorithm for resolving duplicate symbol names. If they request a
> > symbol by name from the kernel and there are 3 symbols with the same
> > name, they use the symbol's position in the built binary image to
> > select the correct symbol.
>
> If a.o, b.o and c.o define local symbol 'foo'.
> By position, do you mean that
>
> * the live-patching code uses something like (findall("foo")[0], findall("foo")[1], findall("foo")[2]) ?
Yes, it depends on their order in the symbol table of the linked binary
(vmlinux).
> * shuffling a.o/b.o/c.o will make the returned triple different
Yes, though it's actually functions that get shuffled.
> Local symbols are not required to be unique. Instead of patching the toolchain,
> have you thought about making the live-patching code smarter?
It's a possibility (more on that below).
> (Depend on the duplicates, such a linker option can increase the link time/binary size considerably
Have you tried it on vmlinux? Just wondering what the time/size impact
would be in real-world numbers.
Duplicate symbols make up a very small percentage of all symbols in the
kernel, so I would think the binary size change (to the strtab?) would
be insignificant?
> AND I don't know in what other cases such an option will be useful)
I believe some other kernel components (tracing, kprobes, bpf) have the
same problem as livepatch with respect to disambiguating duplicate
symbols, for the purposes of tracing/debugging. So I'm thinking it
would be a nice overall improvement to the kernel.
> For the following example,
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=26822
>
> # RUN: split-file %s %t
> # RUN: gcc -c %t/a.s -o %t/a.o
> # RUN: gcc -c %t/b.s -o %t/b.o
> # RUN: gcc -c %t/c.s -o %t/c.o
> # RUN: ld-new %t/a.o %t/b.o %t/c.o -z unique-symbol -o %t.exe
> #--- a.s
> a: a.1: a.2: nop
> #--- b.s
> a: nop
> #--- c.s
> a: nop
>
> readelf -Ws output:
>
> Symbol table '.symtab' contains 13 entries:
> Num: Value Size Type Bind Vis Ndx Name
> 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND 1:
> 0000000000000000 0 FILE LOCAL DEFAULT ABS a.o
> 2: 0000000000401000 0 NOTYPE LOCAL DEFAULT 1 a
> 3: 0000000000401000 0 NOTYPE LOCAL DEFAULT 1 a.1
> 4: 0000000000401000 0 NOTYPE LOCAL DEFAULT 1 a.2
> 5: 0000000000000000 0 FILE LOCAL DEFAULT ABS b.o
> 6: 0000000000401001 0 NOTYPE LOCAL DEFAULT 1 a.1
> 7: 0000000000000000 0 FILE LOCAL DEFAULT ABS c.o
> 8: 0000000000401002 0 NOTYPE LOCAL DEFAULT 1 a.2
> 9: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND _start
> 10: 0000000000402000 0 NOTYPE GLOBAL DEFAULT 1 __bss_start
> 11: 0000000000402000 0 NOTYPE GLOBAL DEFAULT 1 _edata
> 12: 0000000000402000 0 NOTYPE GLOBAL DEFAULT 1 _end
>
> Note that you have STT_FILE SHN_ABS symbols.
> If the compiler does not produce them, they will be synthesized by GNU ld.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=26822
> ld.bfd copies non-STT_SECTION local symbols from input object files. If an
> object file does not have STT_FILE symbols (no .file directive) but has
> non-STT_SECTION local symbols, ld.bfd synthesizes a STT_FILE symbol
Right, I see what you're getting at. As far as I can tell, there are
potentially two ways for fgkaslr to handle this:
a) shuffle files, not functions. i.e. keep the functions' order intact
within the STT_FILE group, shuffling the file groups themselves.
(NOTE: this may have an additional benefit of improving i-cache
performance, compared to the current fgkaslr implementation.)
or
b) shuffle functions, keeping track of what file they belonged to.
Maybe Kristen could comment on the feasibility of either of these
options. I believe the STT_FILE symbols are not currently available to
the kernel at runtime. They would need to be made available to both
fgkaslr and livepatch code.
Overall "ld -z unique-symbol" would be much easier from a kernel
standpoint, and would benefit multiple components as I mentioned above.
> The filenames are usually base names, so "a.o" and "a.o" in two directories will
> be indistinguishable. The live-patching code can possibly work around this by
> not changing the relative order of the two "a.o".
Right, there are some file:func duplicates so this case would indeed
need to be handled somehow.
$ readelf -s --wide vmlinux |awk '$4 == "FILE" {file=$8; next} $4 == "FUNC" {printf "%s:%s\n", file, $8}' |sort |uniq -d
bus.c:new_id_store
core.c:cmask_show
core.c:edge_show
core.c:event_show
core.c:inv_show
core.c:paravirt_read_msr
core.c:paravirt_read_msr_safe
core.c:type_show
core.c:umask_show
hid-core.c:hid_exit
hid-core.c:hid_init
inode.c:init_once
inode.c:remove_one
msr.c:msr_init
proc.c:c_next
proc.c:c_start
proc.c:c_stop
raw.c:dst_output
raw.c:raw_ioctl
route.c:dst_discard
super.c:init_once
udp.c:udp_lib_close
udp.c:udp_lib_hash
udp.c:udplite_getfrag
udplite.c:udp_lib_close
udplite.c:udp_lib_hash
udplite.c:udplite_sk_init
--
Josh
^ permalink raw reply
* Re: [PATCH v4 00/10] Function Granular KASLR
From: Fangrui Song @ 2021-01-23 22:59 UTC (permalink / raw)
To: Kristen Carlson Accardi
Cc: Miroslav Benes, Josh Poimboeuf, Kees Cook, tglx, mingo, bp, arjan,
x86, linux-kernel, kernel-hardening, rick.p.edgecombe,
live-patching, Hongjiu Lu, joe.lawrence
In-Reply-To: <20200828192413.p6rctr42xtuh2c2e@treble>
On 2020-08-28, Josh Poimboeuf wrote:
>On Fri, Aug 28, 2020 at 12:21:13PM +0200, Miroslav Benes wrote:
>> > Hi there! I was trying to find a super easy way to address this, so I
>> > thought the best thing would be if there were a compiler or linker
>> > switch to just eliminate any duplicate symbols at compile time for
>> > vmlinux. I filed this question on the binutils bugzilla looking to see
>> > if there were existing flags that might do this, but H.J. Lu went ahead
>> > and created a new one "-z unique", that seems to do what we would need
>> > it to do.
>> >
>> > https://sourceware.org/bugzilla/show_bug.cgi?id=26391
>> >
>> > When I use this option, it renames any duplicate symbols with an
>> > extension - for example duplicatefunc.1 or duplicatefunc.2. You could
>> > either match on the full unique name of the specific binary you are
>> > trying to patch, or you match the base name and use the extension to
>> > determine original position. Do you think this solution would work?
>>
>> Yes, I think so (thanks, Joe, for testing!).
>>
>> It looks cleaner to me than the options above, but it may just be a matter
>> of taste. Anyway, I'd go with full name matching, because -z unique-symbol
>> would allow us to remove sympos altogether, which is appealing.
>>
>> > If
>> > so, I can modify livepatch to refuse to patch on duplicated symbols if
>> > CONFIG_FG_KASLR and when this option is merged into the tool chain I
>> > can add it to KBUILD_LDFLAGS when CONFIG_FG_KASLR and livepatching
>> > should work in all cases.
>>
>> Ok.
>>
>> Josh, Petr, would this work for you too?
>
>Sounds good to me. Kristen, thanks for finding a solution!
(I am not subscribed. I came here via https://sourceware.org/bugzilla/show_bug.cgi?id=26391 (ld -z unique-symbol))
> This works great after randomization because it always receives the
> current address at runtime rather than relying on any kind of
> buildtime address. The issue with with the live-patching code's
> algorithm for resolving duplicate symbol names. If they request a
> symbol by name from the kernel and there are 3 symbols with the same
> name, they use the symbol's position in the built binary image to
> select the correct symbol.
If a.o, b.o and c.o define local symbol 'foo'.
By position, do you mean that
* the live-patching code uses something like (findall("foo")[0], findall("foo")[1], findall("foo")[2]) ?
* shuffling a.o/b.o/c.o will make the returned triple different
Local symbols are not required to be unique. Instead of patching the toolchain,
have you thought about making the live-patching code smarter?
(Depend on the duplicates, such a linker option can increase the link time/binary size considerably
AND I don't know in what other cases such an option will be useful)
For the following example,
https://sourceware.org/bugzilla/show_bug.cgi?id=26822
# RUN: split-file %s %t
# RUN: gcc -c %t/a.s -o %t/a.o
# RUN: gcc -c %t/b.s -o %t/b.o
# RUN: gcc -c %t/c.s -o %t/c.o
# RUN: ld-new %t/a.o %t/b.o %t/c.o -z unique-symbol -o %t.exe
#--- a.s
a: a.1: a.2: nop
#--- b.s
a: nop
#--- c.s
a: nop
readelf -Ws output:
Symbol table '.symtab' contains 13 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
1: 0000000000000000 0 FILE LOCAL DEFAULT ABS a.o
2: 0000000000401000 0 NOTYPE LOCAL DEFAULT 1 a
3: 0000000000401000 0 NOTYPE LOCAL DEFAULT 1 a.1
4: 0000000000401000 0 NOTYPE LOCAL DEFAULT 1 a.2
5: 0000000000000000 0 FILE LOCAL DEFAULT ABS b.o
6: 0000000000401001 0 NOTYPE LOCAL DEFAULT 1 a.1
7: 0000000000000000 0 FILE LOCAL DEFAULT ABS c.o
8: 0000000000401002 0 NOTYPE LOCAL DEFAULT 1 a.2
9: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND _start
10: 0000000000402000 0 NOTYPE GLOBAL DEFAULT 1 __bss_start
11: 0000000000402000 0 NOTYPE GLOBAL DEFAULT 1 _edata
12: 0000000000402000 0 NOTYPE GLOBAL DEFAULT 1 _end
Note that you have STT_FILE SHN_ABS symbols.
If the compiler does not produce them, they will be synthesized by GNU ld.
https://sourceware.org/bugzilla/show_bug.cgi?id=26822
ld.bfd copies non-STT_SECTION local symbols from input object files. If an
object file does not have STT_FILE symbols (no .file directive) but has
non-STT_SECTION local symbols, ld.bfd synthesizes a STT_FILE symbol
The filenames are usually base names, so "a.o" and "a.o" in two directories will
be indistinguishable. The live-patching code can possibly work around this by
not changing the relative order of the two "a.o".
^ permalink raw reply
* Re: [RFC PATCH 00/17] objtool: add base support for arm64
From: Madhavan T. Venkataraman @ 2021-01-22 21:44 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Mark Brown, Josh Poimboeuf, Mark Rutland, Michal Marek,
Julien Thierry, Peter Zijlstra, Catalin Marinas, Masahiro Yamada,
Linux Kernel Mailing List, linux-efi, linux-hardening,
live-patching, Will Deacon, Linux ARM, Kees Cook
In-Reply-To: <CAMj1kXFr0wvx-hG6nBY4ibju9ww4x0CGhQber3MZQ2ZZn9LHWw@mail.gmail.com>
On 1/22/21 3:43 PM, Ard Biesheuvel wrote:
> On Fri, 22 Jan 2021 at 22:15, Madhavan T. Venkataraman
> <madvenka@linux.microsoft.com> wrote:
>>
>>
>>
>> On 1/22/21 11:43 AM, Mark Brown wrote:
>>> On Thu, Jan 21, 2021 at 12:54:52PM -0600, Josh Poimboeuf wrote:
>>>
>>>> 2) The shadow stack idea sounds promising -- how hard would it be to
>>>> make a prototype reliable unwinder?
>>>
>>> In theory it doesn't look too hard and I can't see a particular reason
>>> not to try doing this - there's going to be edge cases but hopefully for
>>> reliable stack trace they're all in areas where we would be happy to
>>> just decide the stack isn't reliable anyway, things like nesting which
>>> allocates separate shadow stacks for each nested level for example.
>>> I'll take a look.
>>>
>>
>> I am a new comer to this discussion and I am learning. Just have some
>> questions. Pardon me if they are obvious or if they have already been
>> asked and answered.
>>
>> Doesn't Clang already have support for a shadow stack implementation for ARM64?
>> We could take a look at how Clang does it.
>>
>> Will there not be a significant performance hit? May be, some of it can be
>> mitigated by using a parallel shadow stack rather than a compact one.
>>
>> Are there any longjmp style situations in the kernel where the stack is
>> unwound by several frames? In these cases, the shadow stack must be unwound
>> accordingly.
>>
>
> Hello Madhavan,
>
> Let's discuss the details of shadow call stacks on a separate thread,
> instead of further hijacking Julien's series.
>
OK. Sounds good.
^ permalink raw reply
* Re: [RFC PATCH 00/17] objtool: add base support for arm64
From: Ard Biesheuvel @ 2021-01-22 21:43 UTC (permalink / raw)
To: Madhavan T. Venkataraman
Cc: Mark Brown, Josh Poimboeuf, Mark Rutland, Michal Marek,
Julien Thierry, Peter Zijlstra, Catalin Marinas, Masahiro Yamada,
Linux Kernel Mailing List, linux-efi, linux-hardening,
live-patching, Will Deacon, Linux ARM, Kees Cook
In-Reply-To: <bebccb15-1195-c004-923e-74d8444250e1@linux.microsoft.com>
On Fri, 22 Jan 2021 at 22:15, Madhavan T. Venkataraman
<madvenka@linux.microsoft.com> wrote:
>
>
>
> On 1/22/21 11:43 AM, Mark Brown wrote:
> > On Thu, Jan 21, 2021 at 12:54:52PM -0600, Josh Poimboeuf wrote:
> >
> >> 2) The shadow stack idea sounds promising -- how hard would it be to
> >> make a prototype reliable unwinder?
> >
> > In theory it doesn't look too hard and I can't see a particular reason
> > not to try doing this - there's going to be edge cases but hopefully for
> > reliable stack trace they're all in areas where we would be happy to
> > just decide the stack isn't reliable anyway, things like nesting which
> > allocates separate shadow stacks for each nested level for example.
> > I'll take a look.
> >
>
> I am a new comer to this discussion and I am learning. Just have some
> questions. Pardon me if they are obvious or if they have already been
> asked and answered.
>
> Doesn't Clang already have support for a shadow stack implementation for ARM64?
> We could take a look at how Clang does it.
>
> Will there not be a significant performance hit? May be, some of it can be
> mitigated by using a parallel shadow stack rather than a compact one.
>
> Are there any longjmp style situations in the kernel where the stack is
> unwound by several frames? In these cases, the shadow stack must be unwound
> accordingly.
>
Hello Madhavan,
Let's discuss the details of shadow call stacks on a separate thread,
instead of further hijacking Julien's series.
^ permalink raw reply
* Re: [RFC PATCH 00/17] objtool: add base support for arm64
From: Madhavan T. Venkataraman @ 2021-01-22 21:15 UTC (permalink / raw)
To: Mark Brown, Josh Poimboeuf
Cc: Mark Rutland, Michal Marek, Julien Thierry, Peter Zijlstra,
Catalin Marinas, Masahiro Yamada, Linux Kernel Mailing List,
linux-efi, linux-hardening, live-patching, Will Deacon,
Ard Biesheuvel, Linux ARM, Kees Cook
In-Reply-To: <20210122174342.GG6391@sirena.org.uk>
On 1/22/21 11:43 AM, Mark Brown wrote:
> On Thu, Jan 21, 2021 at 12:54:52PM -0600, Josh Poimboeuf wrote:
>
>> 2) The shadow stack idea sounds promising -- how hard would it be to
>> make a prototype reliable unwinder?
>
> In theory it doesn't look too hard and I can't see a particular reason
> not to try doing this - there's going to be edge cases but hopefully for
> reliable stack trace they're all in areas where we would be happy to
> just decide the stack isn't reliable anyway, things like nesting which
> allocates separate shadow stacks for each nested level for example.
> I'll take a look.
>
I am a new comer to this discussion and I am learning. Just have some
questions. Pardon me if they are obvious or if they have already been
asked and answered.
Doesn't Clang already have support for a shadow stack implementation for ARM64?
We could take a look at how Clang does it.
Will there not be a significant performance hit? May be, some of it can be
mitigated by using a parallel shadow stack rather than a compact one.
Are there any longjmp style situations in the kernel where the stack is
unwound by several frames? In these cases, the shadow stack must be unwound
accordingly.
Madhavan
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply
* Re: [RFC PATCH 00/17] objtool: add base support for arm64
From: Madhavan T. Venkataraman @ 2021-01-22 21:16 UTC (permalink / raw)
To: Mark Brown, Josh Poimboeuf
Cc: Mark Rutland, Michal Marek, Julien Thierry, Peter Zijlstra,
Catalin Marinas, Masahiro Yamada, Linux Kernel Mailing List,
linux-efi, linux-hardening, live-patching, Will Deacon,
Ard Biesheuvel, Linux ARM, Kees Cook
In-Reply-To: <20210122174342.GG6391@sirena.org.uk>
On 1/22/21 11:43 AM, Mark Brown wrote:
> On Thu, Jan 21, 2021 at 12:54:52PM -0600, Josh Poimboeuf wrote:
>
>> 2) The shadow stack idea sounds promising -- how hard would it be to
>> make a prototype reliable unwinder?
>
> In theory it doesn't look too hard and I can't see a particular reason
> not to try doing this - there's going to be edge cases but hopefully for
> reliable stack trace they're all in areas where we would be happy to
> just decide the stack isn't reliable anyway, things like nesting which
> allocates separate shadow stacks for each nested level for example.
> I'll take a look.
>
I am a new comer to this discussion and I am learning. Just have some
questions. Pardon me if they are obvious or if they have already been
asked and answered.
Doesn't Clang already have support for a shadow stack implementation for ARM64?
We could take a look at how Clang does it.
Will there not be a significant performance hit? May be, some of it can be
mitigated by using a parallel shadow stack rather than a compact one.
Are there any longjmp style situations in the kernel where the stack is
unwound by several frames? In these cases, the shadow stack must be unwound
accordingly.
Madhavan
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply
* Re: [RFC PATCH 00/17] objtool: add base support for arm64
From: Ard Biesheuvel @ 2021-01-22 17:54 UTC (permalink / raw)
To: Mark Brown
Cc: Josh Poimboeuf, Peter Zijlstra, Julien Thierry,
Linux Kernel Mailing List, Linux ARM, Catalin Marinas,
Will Deacon, Masahiro Yamada, Kees Cook, Michal Marek,
Mark Rutland, linux-efi, linux-hardening, live-patching
In-Reply-To: <20210122174342.GG6391@sirena.org.uk>
On Fri, 22 Jan 2021 at 18:44, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Jan 21, 2021 at 12:54:52PM -0600, Josh Poimboeuf wrote:
>
> > 2) The shadow stack idea sounds promising -- how hard would it be to
> > make a prototype reliable unwinder?
>
> In theory it doesn't look too hard and I can't see a particular reason
> not to try doing this - there's going to be edge cases but hopefully for
> reliable stack trace they're all in areas where we would be happy to
> just decide the stack isn't reliable anyway, things like nesting which
> allocates separate shadow stacks for each nested level for example.
> I'll take a look.
This reminds me - a while ago, I had a stab at writing a rudimentary
GCC plugin that pushes/pops return addresses to a shadow call stack
pointed to by x18 [0]
I am by no means suggesting that we should rely on a GCC plugin for
this, only that it does seem rather straight-forward for the compiler
to manage a stack with return addresses like that (although the devil
is probably in the details, as usual)
[0] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm64-scs-gcc
^ permalink raw reply
* Re: [RFC PATCH 00/17] objtool: add base support for arm64
From: Mark Brown @ 2021-01-22 17:43 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Ard Biesheuvel, Peter Zijlstra, Julien Thierry,
Linux Kernel Mailing List, Linux ARM, Catalin Marinas,
Will Deacon, Masahiro Yamada, Kees Cook, Michal Marek,
Mark Rutland, linux-efi, linux-hardening, live-patching
In-Reply-To: <20210121185452.fxoz4ehqfv75bdzq@treble>
[-- Attachment #1: Type: text/plain, Size: 548 bytes --]
On Thu, Jan 21, 2021 at 12:54:52PM -0600, Josh Poimboeuf wrote:
> 2) The shadow stack idea sounds promising -- how hard would it be to
> make a prototype reliable unwinder?
In theory it doesn't look too hard and I can't see a particular reason
not to try doing this - there's going to be edge cases but hopefully for
reliable stack trace they're all in areas where we would be happy to
just decide the stack isn't reliable anyway, things like nesting which
allocates separate shadow stacks for each nested level for example.
I'll take a look.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v6 0/2] Documentation: livepatch: Document reliable stacktrace and minor cleanup
From: Jiri Kosina @ 2021-01-22 10:59 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Mark Brown, linux-kernel, Mark Rutland, Joe Lawrence,
Miroslav Benes, Petr Mladek, Josh Poimboeuf, linux-doc,
live-patching
In-Reply-To: <20210121115226.565790ef@lwn.net>
On Thu, 21 Jan 2021, Jonathan Corbet wrote:
> > This series adds a document, mainly written by Mark Rutland, which
> > makes explicit the requirements for implementing reliable stacktrace
> > in order to aid architectures adding this feature. It also updates
> > the other livepatching documents to use automatically generated tables
> > of contents following review comments on Mark's document.
>
> So...is this deemed ready and, if so, do you want it to go through the
> docs tree or via some other path?
I am planning to take it through livepatching tree unless there are any
additional last-minutes comments.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 04/13] livepatch: move klp_find_object_module to module.c
From: Josh Poimboeuf @ 2021-01-21 21:45 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Frederic Barrat, Andrew Donnellan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Jessica Yu, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, Masahiro Yamada, Michal Marek, linux-kernel,
linuxppc-dev, dri-devel, live-patching, linux-kbuild
In-Reply-To: <20210121074959.313333-5-hch@lst.de>
On Thu, Jan 21, 2021 at 08:49:50AM +0100, Christoph Hellwig wrote:
> @@ -820,14 +796,25 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> const char *name;
>
> obj->patched = false;
> - obj->mod = NULL;
Why was this line removed?
> if (klp_is_module(obj)) {
> if (strlen(obj->name) >= MODULE_NAME_LEN)
> return -EINVAL;
> name = obj->name;
>
> - klp_find_object_module(obj);
> + /*
> + * We do not want to block removal of patched modules and
> + * therefore we do not take a reference here. The patches are
> + * removed by klp_module_going() instead.
> + *
> + * Do not mess work of klp_module_coming() and
> + * klp_module_going(). Note that the patch might still be
> + * needed before klp_module_going() is called. Module functions
> + * can be called even in the GOING state until mod->exit()
> + * finishes. This is especially important for patches that
> + * modify semantic of the functions.
> + */
> + obj->mod = find_klp_module(obj->name);
These comments don't make sense in this context, they should be kept
with the code in find_klp_module().
--
Josh
^ permalink raw reply
* module loader dead code removal and cleanusp
From: Christoph Hellwig @ 2021-01-21 7:49 UTC (permalink / raw)
To: Frederic Barrat, Andrew Donnellan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Jessica Yu, Josh Poimboeuf, Jiri Kosina, Miroslav Benes,
Petr Mladek, Joe Lawrence
Cc: Masahiro Yamada, Michal Marek, linux-kernel, linuxppc-dev,
dri-devel, live-patching, linux-kbuild
Hi all,
this series removes support for long term unused export types and
cleans up various loose ends in the module loader.
Diffstat:
arch/arm/configs/bcm2835_defconfig | 1
arch/arm/configs/mxs_defconfig | 1
arch/mips/configs/nlm_xlp_defconfig | 1
arch/mips/configs/nlm_xlr_defconfig | 1
arch/parisc/configs/generic-32bit_defconfig | 1
arch/parisc/configs/generic-64bit_defconfig | 1
arch/powerpc/configs/ppc6xx_defconfig | 1
arch/powerpc/platforms/powernv/pci-cxl.c | 22 -
arch/s390/configs/debug_defconfig | 1
arch/s390/configs/defconfig | 1
arch/sh/configs/edosk7760_defconfig | 1
arch/sh/configs/sdk7780_defconfig | 1
arch/x86/configs/i386_defconfig | 1
arch/x86/configs/x86_64_defconfig | 1
arch/x86/tools/relocs.c | 4
drivers/gpu/drm/drm_crtc_helper_internal.h | 10
drivers/gpu/drm/drm_fb_helper.c | 21 -
drivers/gpu/drm/drm_kms_helper_common.c | 26 +-
include/asm-generic/vmlinux.lds.h | 42 ---
include/linux/export.h | 9
include/linux/kallsyms.h | 17 -
include/linux/module.h | 42 ---
init/Kconfig | 17 -
kernel/kallsyms.c | 8
kernel/livepatch/core.c | 61 +----
kernel/module.c | 319 ++++++++++------------------
kernel/trace/trace_kprobe.c | 4
lib/bug.c | 3
scripts/checkpatch.pl | 6
scripts/mod/modpost.c | 50 ----
scripts/mod/modpost.h | 3
scripts/module.lds.S | 6
tools/include/linux/export.h | 3
33 files changed, 181 insertions(+), 505 deletions(-)
^ permalink raw reply
* [PATCH 01/13] powerpc/powernv: remove get_cxl_module
From: Christoph Hellwig @ 2021-01-21 7:49 UTC (permalink / raw)
To: Frederic Barrat, Andrew Donnellan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Jessica Yu, Josh Poimboeuf, Jiri Kosina, Miroslav Benes,
Petr Mladek, Joe Lawrence
Cc: Masahiro Yamada, Michal Marek, linux-kernel, linuxppc-dev,
dri-devel, live-patching, linux-kbuild
In-Reply-To: <20210121074959.313333-1-hch@lst.de>
The static inline get_cxl_module function is entirely unused,
remove it.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/platforms/powernv/pci-cxl.c | 22 ----------------------
1 file changed, 22 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-cxl.c b/arch/powerpc/platforms/powernv/pci-cxl.c
index 8c739c94ed28d6..53172862d23bd3 100644
--- a/arch/powerpc/platforms/powernv/pci-cxl.c
+++ b/arch/powerpc/platforms/powernv/pci-cxl.c
@@ -150,25 +150,3 @@ int pnv_cxl_ioda_msi_setup(struct pci_dev *dev, unsigned int hwirq,
return 0;
}
EXPORT_SYMBOL(pnv_cxl_ioda_msi_setup);
-
-#if IS_MODULE(CONFIG_CXL)
-static inline int get_cxl_module(void)
-{
- struct module *cxl_module;
-
- mutex_lock(&module_mutex);
-
- cxl_module = find_module("cxl");
- if (cxl_module)
- __module_get(cxl_module);
-
- mutex_unlock(&module_mutex);
-
- if (!cxl_module)
- return -ENODEV;
-
- return 0;
-}
-#else
-static inline int get_cxl_module(void) { return 0; }
-#endif
--
2.29.2
^ permalink raw reply related
* [PATCH 02/13] module: add a module_loaded helper
From: Christoph Hellwig @ 2021-01-21 7:49 UTC (permalink / raw)
To: Frederic Barrat, Andrew Donnellan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Jessica Yu, Josh Poimboeuf, Jiri Kosina, Miroslav Benes,
Petr Mladek, Joe Lawrence
Cc: Masahiro Yamada, Michal Marek, linux-kernel, linuxppc-dev,
dri-devel, live-patching, linux-kbuild
In-Reply-To: <20210121074959.313333-1-hch@lst.de>
Add a helper that takes modules_mutex and uses find_module to check if a
given module is loaded. This provides a better abstraction for the two
callers, and allows to unexport modules_mutex and find_module.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/gpu/drm/drm_fb_helper.c | 7 +------
include/linux/module.h | 3 +++
kernel/module.c | 14 ++++++++++++--
kernel/trace/trace_kprobe.c | 4 +---
4 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 4b81195106875d..ce6d63ca75c32a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2508,13 +2508,8 @@ int __init drm_fb_helper_modinit(void)
{
#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
const char name[] = "fbcon";
- struct module *fbcon;
- mutex_lock(&module_mutex);
- fbcon = find_module(name);
- mutex_unlock(&module_mutex);
-
- if (!fbcon)
+ if (!module_loaded(name))
request_module_nowait(name);
#endif
return 0;
diff --git a/include/linux/module.h b/include/linux/module.h
index 7a0bcb5b1ffccd..b4654f8a408134 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -589,6 +589,9 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
/* Search for module by name: must hold module_mutex. */
struct module *find_module(const char *name);
+/* Check if a module is loaded. */
+bool module_loaded(const char *name);
+
struct symsearch {
const struct kernel_symbol *start, *stop;
const s32 *crcs;
diff --git a/kernel/module.c b/kernel/module.c
index 4bf30e4b3eaaa1..619ea682e64cd1 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -88,7 +88,6 @@
* (delete and add uses RCU list operations).
*/
DEFINE_MUTEX(module_mutex);
-EXPORT_SYMBOL_GPL(module_mutex);
static LIST_HEAD(modules);
/* Work queue for freeing init sections in success case */
@@ -672,7 +671,18 @@ struct module *find_module(const char *name)
module_assert_mutex();
return find_module_all(name, strlen(name), false);
}
-EXPORT_SYMBOL_GPL(find_module);
+
+bool module_loaded(const char *name)
+{
+ bool ret;
+
+ mutex_lock(&module_mutex);
+ ret = !!find_module(name);
+ mutex_unlock(&module_mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(module_loaded);
#ifdef CONFIG_SMP
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index e6fba1798771b4..c2e453f88bce70 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -124,9 +124,7 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
if (!p)
return true;
*p = '\0';
- mutex_lock(&module_mutex);
- ret = !!find_module(tk->symbol);
- mutex_unlock(&module_mutex);
+ ret = module_loaded(tk->symbol);
*p = ':';
return ret;
--
2.29.2
^ permalink raw reply related
* [PATCH 04/13] livepatch: move klp_find_object_module to module.c
From: Christoph Hellwig @ 2021-01-21 7:49 UTC (permalink / raw)
To: Frederic Barrat, Andrew Donnellan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Jessica Yu, Josh Poimboeuf, Jiri Kosina, Miroslav Benes,
Petr Mladek, Joe Lawrence
Cc: Masahiro Yamada, Michal Marek, linux-kernel, linuxppc-dev,
dri-devel, live-patching, linux-kbuild
In-Reply-To: <20210121074959.313333-1-hch@lst.de>
To uncouple the livepatch code from module loader internals move a
slightly refactored version of klp_find_object_module to module.c
This allows to mark find_module static and removes one of the last
users of module_mutex outside of module.c.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/module.h | 3 +--
kernel/livepatch/core.c | 39 +++++++++++++--------------------------
kernel/module.c | 17 ++++++++++++++++-
3 files changed, 30 insertions(+), 29 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index b4654f8a408134..8588482bde4116 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -586,8 +586,7 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
return within_module_init(addr, mod) || within_module_core(addr, mod);
}
-/* Search for module by name: must hold module_mutex. */
-struct module *find_module(const char *name);
+struct module *find_klp_module(const char *name);
/* Check if a module is loaded. */
bool module_loaded(const char *name);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index a7f625dc24add3..878759baadd81c 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -49,30 +49,6 @@ static bool klp_is_module(struct klp_object *obj)
return obj->name;
}
-/* sets obj->mod if object is not vmlinux and module is found */
-static void klp_find_object_module(struct klp_object *obj)
-{
- struct module *mod;
-
- mutex_lock(&module_mutex);
- /*
- * We do not want to block removal of patched modules and therefore
- * we do not take a reference here. The patches are removed by
- * klp_module_going() instead.
- */
- mod = find_module(obj->name);
- /*
- * Do not mess work of klp_module_coming() and klp_module_going().
- * Note that the patch might still be needed before klp_module_going()
- * is called. Module functions can be called even in the GOING state
- * until mod->exit() finishes. This is especially important for
- * patches that modify semantic of the functions.
- */
- if (mod && mod->klp_alive)
- obj->mod = mod;
- mutex_unlock(&module_mutex);
-}
-
static bool klp_initialized(void)
{
return !!klp_root_kobj;
@@ -820,14 +796,25 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
const char *name;
obj->patched = false;
- obj->mod = NULL;
if (klp_is_module(obj)) {
if (strlen(obj->name) >= MODULE_NAME_LEN)
return -EINVAL;
name = obj->name;
- klp_find_object_module(obj);
+ /*
+ * We do not want to block removal of patched modules and
+ * therefore we do not take a reference here. The patches are
+ * removed by klp_module_going() instead.
+ *
+ * Do not mess work of klp_module_coming() and
+ * klp_module_going(). Note that the patch might still be
+ * needed before klp_module_going() is called. Module functions
+ * can be called even in the GOING state until mod->exit()
+ * finishes. This is especially important for patches that
+ * modify semantic of the functions.
+ */
+ obj->mod = find_klp_module(obj->name);
} else {
name = "vmlinux";
}
diff --git a/kernel/module.c b/kernel/module.c
index 619ea682e64cd1..299cbac0775cf2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -666,7 +666,7 @@ static struct module *find_module_all(const char *name, size_t len,
return NULL;
}
-struct module *find_module(const char *name)
+static struct module *find_module(const char *name)
{
module_assert_mutex();
return find_module_all(name, strlen(name), false);
@@ -684,6 +684,21 @@ bool module_loaded(const char *name)
}
EXPORT_SYMBOL_GPL(module_loaded);
+#ifdef CONFIG_LIVEPATCH
+struct module *find_klp_module(const char *name)
+{
+ struct module *mod;
+
+ mutex_lock(&module_mutex);
+ mod = find_module(name);
+ if (mod && !mod->klp_alive)
+ mod = NULL;
+ mutex_unlock(&module_mutex);
+
+ return mod;
+}
+#endif /* CONFIG_LIVEPATCH */
+
#ifdef CONFIG_SMP
static inline void __percpu *mod_percpu(struct module *mod)
--
2.29.2
^ permalink raw reply related
* [PATCH 05/13] kallsyms: refactor {,module_}kallsyms_on_each_symbol
From: Christoph Hellwig @ 2021-01-21 7:49 UTC (permalink / raw)
To: Frederic Barrat, Andrew Donnellan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Jessica Yu, Josh Poimboeuf, Jiri Kosina, Miroslav Benes,
Petr Mladek, Joe Lawrence
Cc: Masahiro Yamada, Michal Marek, linux-kernel, linuxppc-dev,
dri-devel, live-patching, linux-kbuild
In-Reply-To: <20210121074959.313333-1-hch@lst.de>
Require an explicit cll to module_kallsyms_on_each_symbol to look
for symbols in modules instead of the call from kallsyms_on_each_symbol,
and acquire module_mutex inside of module_kallsyms_on_each_symbol instead
of leaving that up to the caller.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
kernel/kallsyms.c | 6 +++++-
kernel/livepatch/core.c | 6 +-----
kernel/module.c | 8 ++++----
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index fe9de067771c34..a0d3f0865916f9 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -177,6 +177,10 @@ unsigned long kallsyms_lookup_name(const char *name)
return module_kallsyms_lookup_name(name);
}
+/*
+ * Iterate over all symbols in vmlinux. For symbols from modules use
+ * module_kallsyms_on_each_symbol instead.
+ */
int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
unsigned long),
void *data)
@@ -192,7 +196,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
if (ret != 0)
return ret;
}
- return module_kallsyms_on_each_symbol(fn, data);
+ return 0;
}
static unsigned long get_symbol_pos(unsigned long addr,
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 878759baadd81c..8063b9089bd2f8 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -135,12 +135,8 @@ static int klp_find_object_symbol(const char *objname, const char *name,
.pos = sympos,
};
- mutex_lock(&module_mutex);
- if (objname)
+ if (objname || !kallsyms_on_each_symbol(klp_find_callback, &args))
module_kallsyms_on_each_symbol(klp_find_callback, &args);
- else
- kallsyms_on_each_symbol(klp_find_callback, &args);
- mutex_unlock(&module_mutex);
/*
* Ensure an address was found. If sympos is 0, ensure symbol is unique;
diff --git a/kernel/module.c b/kernel/module.c
index 299cbac0775cf2..885feec64c1b6f 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -4407,8 +4407,7 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
unsigned int i;
int ret;
- module_assert_mutex();
-
+ mutex_lock(&module_mutex);
list_for_each_entry(mod, &modules, list) {
/* We hold module_mutex: no need for rcu_dereference_sched */
struct mod_kallsyms *kallsyms = mod->kallsyms;
@@ -4424,10 +4423,11 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
ret = fn(data, kallsyms_symbol_name(kallsyms, i),
mod, kallsyms_symbol_value(sym));
if (ret != 0)
- return ret;
+ break;
}
}
- return 0;
+ mutex_unlock(&module_mutex);
+ return ret;
}
#endif /* CONFIG_KALLSYMS */
--
2.29.2
^ permalink raw reply related
* Re: [PATCH v6 0/2] Documentation: livepatch: Document reliable stacktrace and minor cleanup
From: Jonathan Corbet @ 2021-01-21 18:52 UTC (permalink / raw)
To: Mark Brown
Cc: linux-kernel, Mark Rutland, Jiri Kosina, Joe Lawrence,
Miroslav Benes, Petr Mladek, Josh Poimboeuf, linux-doc,
live-patching
In-Reply-To: <20210120164714.16581-1-broonie@kernel.org>
On Wed, 20 Jan 2021 16:47:12 +0000
Mark Brown <broonie@kernel.org> wrote:
> This series adds a document, mainly written by Mark Rutland, which makes
> explicit the requirements for implementing reliable stacktrace in order
> to aid architectures adding this feature. It also updates the other
> livepatching documents to use automatically generated tables of contents
> following review comments on Mark's document.
So...is this deemed ready and, if so, do you want it to go through the
docs tree or via some other path?
Thanks,
jon
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox