Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks

[Jessica Yu]

+++ Michael Ellerman [16/06/21 12:37 +1000]:

Jessica Yu  writes:

+++ Nicholas Piggin [15/06/21 12:05 +1000]:

Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:

+++ Nicholas Piggin [11/06/21 19:39 +1000]:

The elf_check_arch() function is used to test usermode binaries, but
kernel modules may have more specific requirements. powerpc would like
to test for ABI version compatibility.

Add an arch-overridable function elf_check_module_arch() that defaults
to elf_check_arch() and use it in elf_validity_check().

Signed-off-by: Michael Ellerman 
[np: split patch, added changelog]
Signed-off-by: Nicholas Piggin 
---
include/linux/moduleloader.h | 5 +
kernel/module.c  | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..fdc042a84562 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -13,6 +13,11 @@
 * must be implemented by each architecture.
 */

+// Allow arch to optionally do additional checking of module ELF header
+#ifndef elf_check_module_arch
+#define elf_check_module_arch elf_check_arch
+#endif


Hi Nicholas,

Why not make elf_check_module_arch() consistent with the other
arch-specific functions? Please see module_frob_arch_sections(),
module_{init,exit}_section(), etc in moduleloader.h. That is, they are
all __weak functions that are overridable by arches. We can maybe make
elf_check_module_arch() a weak symbol, available for arches to
override if they want to perform additional elf checks. Then we don't
have to have this one-off #define.



Like this? I like it. Good idea.


Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
separate check entirely so that the powerpc implementation doesn't
have to include that extra elf_check_arch() call. Something like this maybe?


My thinking for making elf_check_module_arch() the only hook was that
conceivably you might not want/need to call elf_check_arch() from
elf_check_module_arch().

So having a single module specific hook allows arch code to decide
how to implement the check, which may or may not involve calling
elf_check_arch(), but that becomes an arch implementation detail.


Thanks for the feedback! Yeah, that's fair too. Well, I ended up doing
it this way mostly to create less churn/change of behavior, since in
its current state elf_check_arch() is already being called for each
arch. Additionally I wanted to save the powerpc implementation of
elf_check_module_arch() an extra elf_check_arch() call. In any case I
have a slight preference for having a second hook to allow arches add
any extra checks in addition to elf_check_arch(). Thanks!

Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks

[Jessica Yu]

+++ Nicholas Piggin [16/06/21 11:18 +1000]:

Excerpts from Jessica Yu's message of June 15, 2021 10:17 pm:

+++ Nicholas Piggin [15/06/21 12:05 +1000]:

Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:

+++ Nicholas Piggin [11/06/21 19:39 +1000]:

The elf_check_arch() function is used to test usermode binaries, but
kernel modules may have more specific requirements. powerpc would like
to test for ABI version compatibility.

Add an arch-overridable function elf_check_module_arch() that defaults
to elf_check_arch() and use it in elf_validity_check().

Signed-off-by: Michael Ellerman 
[np: split patch, added changelog]
Signed-off-by: Nicholas Piggin 
---
include/linux/moduleloader.h | 5 +
kernel/module.c  | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..fdc042a84562 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -13,6 +13,11 @@
 * must be implemented by each architecture.
 */

+// Allow arch to optionally do additional checking of module ELF header
+#ifndef elf_check_module_arch
+#define elf_check_module_arch elf_check_arch
+#endif


Hi Nicholas,

Why not make elf_check_module_arch() consistent with the other
arch-specific functions? Please see module_frob_arch_sections(),
module_{init,exit}_section(), etc in moduleloader.h. That is, they are
all __weak functions that are overridable by arches. We can maybe make
elf_check_module_arch() a weak symbol, available for arches to
override if they want to perform additional elf checks. Then we don't
have to have this one-off #define.



Like this? I like it. Good idea.


Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
separate check entirely so that the powerpc implementation doesn't
have to include that extra elf_check_arch() call. Something like this maybe?


Yeah we can do that. Would you be okay if it goes via powerpc tree? If
yes, then we should get your Ack (or SOB because it seems to be entirely
your patch now :D)


This can go through the powerpc tree. Will you do another respin
of this patch? And yes, feel free to take my SOB for this one -

Signed-off-by: Jessica Yu 

Thanks!

Jessica

Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks

[Jessica Yu]

+++ Segher Boessenkool [15/06/21 07:50 -0500]:

On Tue, Jun 15, 2021 at 02:17:40PM +0200, Jessica Yu wrote:

+int __weak elf_check_module_arch(Elf_Ehdr *hdr)
+{
+   return 1;
+}


But is this a good idea?  It isn't useful to be able to attempt to load
a module not compiled for your architecture, and it increases the attack
surface tremendously.  These checks are one of the few things that can
*not* be weak symbols, imo.


Hm, could you please elaborate a bit more? This patchset is adding
extra Elf header checks specifically for powerpc, and the module
loader usually provides arch-specific hooks via weak symbols. We are
just providing an new hook here, which should act as a no-op if it
isn't used.

So if an architecture wants to provide extra header checks, it can do
so by overriding the new weak symbol. Otherwise, the weak function acts as
a noop. We also already have the existing elf_check_arch() check for each
arch and that is *not* a weak symbol.

Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks

[Jessica Yu]

+++ Nicholas Piggin [15/06/21 12:05 +1000]:

Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:

+++ Nicholas Piggin [11/06/21 19:39 +1000]:

The elf_check_arch() function is used to test usermode binaries, but
kernel modules may have more specific requirements. powerpc would like
to test for ABI version compatibility.

Add an arch-overridable function elf_check_module_arch() that defaults
to elf_check_arch() and use it in elf_validity_check().

Signed-off-by: Michael Ellerman 
[np: split patch, added changelog]
Signed-off-by: Nicholas Piggin 
---
include/linux/moduleloader.h | 5 +
kernel/module.c  | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..fdc042a84562 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -13,6 +13,11 @@
 * must be implemented by each architecture.
 */

+// Allow arch to optionally do additional checking of module ELF header
+#ifndef elf_check_module_arch
+#define elf_check_module_arch elf_check_arch
+#endif


Hi Nicholas,

Why not make elf_check_module_arch() consistent with the other
arch-specific functions? Please see module_frob_arch_sections(),
module_{init,exit}_section(), etc in moduleloader.h. That is, they are
all __weak functions that are overridable by arches. We can maybe make
elf_check_module_arch() a weak symbol, available for arches to
override if they want to perform additional elf checks. Then we don't
have to have this one-off #define.



Like this? I like it. Good idea.


Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
separate check entirely so that the powerpc implementation doesn't
have to include that extra elf_check_arch() call. Something like this maybe?

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..2f9ebd593b4f 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -39,6 +39,9 @@ bool module_init_section(const char *name);
  */
 bool module_exit_section(const char *name);

+/* Arch may override to do additional checking of ELF header architecture */
+int elf_check_module_arch(Elf_Ehdr *hdr);
+
 /*
  * Apply the given relocation to the (simplified) ELF.  Return -error
  * or 0.
diff --git a/kernel/module.c b/kernel/module.c
index fdd6047728df..9963a979ed54 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2923,6 +2923,11 @@ static int validate_section_offset(struct load_info 
*info, Elf_Shdr *shdr)
return 0;
 }

+int __weak elf_check_module_arch(Elf_Ehdr *hdr)
+{
+   return 1;
+}
+
 /*
  * Sanity checks against invalid binaries, wrong arch, weird elf version.
  *
@@ -2941,6 +2946,7 @@ static int elf_validity_check(struct load_info *info)
if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
|| info->hdr->e_type != ET_REL
|| !elf_check_arch(info->hdr)
+   || !elf_check_module_arch(info->hdr)
|| info->hdr->e_shentsize != sizeof(Elf_Shdr))
return -ENOEXEC;
 

Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks

[Jessica Yu]

+++ Nicholas Piggin [11/06/21 19:39 +1000]:

The elf_check_arch() function is used to test usermode binaries, but
kernel modules may have more specific requirements. powerpc would like
to test for ABI version compatibility.

Add an arch-overridable function elf_check_module_arch() that defaults
to elf_check_arch() and use it in elf_validity_check().

Signed-off-by: Michael Ellerman 
[np: split patch, added changelog]
Signed-off-by: Nicholas Piggin 
---
include/linux/moduleloader.h | 5 +
kernel/module.c  | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..fdc042a84562 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -13,6 +13,11 @@
 * must be implemented by each architecture.
 */

+// Allow arch to optionally do additional checking of module ELF header
+#ifndef elf_check_module_arch
+#define elf_check_module_arch elf_check_arch
+#endif


Hi Nicholas,

Why not make elf_check_module_arch() consistent with the other
arch-specific functions? Please see module_frob_arch_sections(),
module_{init,exit}_section(), etc in moduleloader.h. That is, they are
all __weak functions that are overridable by arches. We can maybe make
elf_check_module_arch() a weak symbol, available for arches to
override if they want to perform additional elf checks. Then we don't
have to have this one-off #define.

Thanks,

Jessica


+
/* Adjust arch-specific sections.  Return 0 on success.  */
int module_frob_arch_sections(Elf_Ehdr *hdr,
  Elf_Shdr *sechdrs,
diff --git a/kernel/module.c b/kernel/module.c
index 7e78dfabca97..7c3f9b7478dc 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2946,7 +2946,7 @@ static int elf_validity_check(struct load_info *info)

if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
|| info->hdr->e_type != ET_REL
-   || !elf_check_arch(info->hdr)
+   || !elf_check_module_arch(info->hdr)
|| info->hdr->e_shentsize != sizeof(Elf_Shdr))
return -ENOEXEC;

--
2.23.0

Re: module loader dead code removal and cleanups v3

[Jessica Yu]

+++ Christoph Hellwig [02/02/21 13:13 +0100]:

Hi all,

this series removes support for long term unused export types and
cleans up various loose ends in the module loader.

Changes since v2:
- clean up klp_find_object_symbol a bit
- remove the now unused module_assert_mutex helper

Changes since v1:
- move struct symsearch to module.c
- rework drm to not call find_module at all
- allow RCU-sched locking for find_module
- keep find_module as a public API instead of module_loaded
- update a few comments and commit logs

Diffstat:


Queued on modules-next (along with the updated patch 10).

Thanks everyone,

Jessica

Re: module loader dead code removal and cleanups v3

[Jessica Yu]

+++ Christoph Hellwig [02/02/21 13:13 +0100]:

Hi all,

this series removes support for long term unused export types and
cleans up various loose ends in the module loader.

Changes since v2:
- clean up klp_find_object_symbol a bit
- remove the now unused module_assert_mutex helper

Changes since v1:
- move struct symsearch to module.c
- rework drm to not call find_module at all
- allow RCU-sched locking for find_module
- keep find_module as a public API instead of module_loaded
- update a few comments and commit logs


Thanks Christoph for cleaning up all that aged cruft, and thanks everyone
for the reviews.

I was curious about EXPORT_SYMBOL_GPL_FUTURE and EXPORT_UNUSED_SYMBOL
variants, and found that most of that stuff was introduced between
2006 - 2008. All the of the unused symbols were removed and gpl future
symbols were converted to gpl quite a long time ago, and I don't
believe these export types have been used ever since. So I
think it's safe to retire those export types now.

The patchset looks good so far. After Miroslav's comments are
addressed, I'll wait an extra day or two in case there are more
comments before queueing them onto modules-next. I can take the first
two patches as well provided the acks are there (I think patch 2 is
missing Daniel Vetter's ack from v1 of the series, but I'll add that
back in).

Thanks,

Jessica

Re: [PATCH 04/13] module: use RCU to synchronize find_module

[Jessica Yu]

+++ Miroslav Benes [29/01/21 16:29 +0100]:

On Thu, 28 Jan 2021, Christoph Hellwig wrote:


Allow for a RCU-sched critical section around find_module, following
the lower level find_module_all helper, and switch the two callers
outside of module.c to use such a RCU-sched critical section instead
of module_mutex.


That's a nice idea.


@@ -57,7 +58,7 @@ static void klp_find_object_module(struct klp_object *obj)
if (!klp_is_module(obj))
return;

-   mutex_lock(_mutex);
+   rcu_read_lock_sched();
/*
 * We do not want to block removal of patched modules and therefore
 * we do not take a reference here. The patches are removed by
@@ -74,7 +75,7 @@ static void klp_find_object_module(struct klp_object *obj)
if (mod && mod->klp_alive)


RCU always baffles me a bit, so I'll ask. Don't we need
rcu_dereference_sched() here? "mod" comes from a RCU-protected list, so I
wonder.


Same here :-) I had to double check the RCU documentation. For our
modules list case I believe the rcu list API should take care of that
for us. Worth noting is this snippet from Documentation/RCU/whatisRCU.txt:

   rcu_dereference() is typically used indirectly, via the _rcu
   list-manipulation primitives, such as list_for_each_entry_rcu()

Re: [PATCH 04/13] livepatch: move klp_find_object_module to module.c

[Jessica Yu]

+++ 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 
---
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(_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(_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

Re: [PATCH v2] kbuild: preprocess module linker script

[Jessica Yu]

+++ Masahiro Yamada [08/09/20 13:27 +0900]:

There was a request to preprocess the module linker script like we
do for the vmlinux one. (https://lkml.org/lkml/2020/8/21/512)

The difference between vmlinux.lds and module.lds is that the latter
is needed for external module builds, thus must be cleaned up by
'make mrproper' instead of 'make clean'. Also, it must be created
by 'make modules_prepare'.

You cannot put it in arch/$(SRCARCH)/kernel/, which is cleaned up by
'make clean'. I moved arch/$(SRCARCH)/kernel/module.lds to
arch/$(SRCARCH)/include/asm/module.lds.h, which is included from
scripts/module.lds.S.

scripts/module.lds is fine because 'make clean' keeps all the
build artifacts under scripts/.

You can add arch-specific sections in .

Signed-off-by: Masahiro Yamada 
Tested-by: Jessica Yu 
Acked-by: Will Deacon 


Acked-by: Jessica Yu 

Thanks for working on this! 

Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper

[Jessica Yu]

+++ Jarkko Sakkinen [24/07/20 10:36 +0300]:

On Thu, Jul 23, 2020 at 03:42:09PM +0300, Ard Biesheuvel wrote:

On Thu, 23 Jul 2020 at 04:52, Jarkko Sakkinen
 wrote:
>
> On Thu, Jul 16, 2020 at 06:49:09PM +0200, Christophe Leroy wrote:
> > Jarkko Sakkinen  a écrit :
> >
> > > Rename module_alloc() to text_alloc() and module_memfree() to
> > > text_memfree(), and move them to kernel/text.c, which is unconditionally
> > > compiled to the kernel proper. This allows kprobes, ftrace and bpf to
> > > allocate space for executable code without requiring to compile the 
modules
> > > support (CONFIG_MODULES=y) in.
> >
> > You are not changing enough in powerpc to have this work.
> > On powerpc 32 bits (6xx), when STRICT_KERNEL_RWX is selected, the vmalloc
> > space is set to NX (no exec) at segment level (ie by 256Mbytes zone) unless
> > CONFIG_MODULES is selected.
> >
> > Christophe
>
> This has been deduced down to:
>
> 
https://lore.kernel.org/lkml/20200717030422.679972-1-jarkko.sakki...@linux.intel.com/
>
> I.e. not intruding PPC anymore :-)
>

Ok, so after the elaborate discussion we had between Jessica, Russell,
Peter, Will, Mark, you and myself, where we pointed out that
a) a single text_alloc() abstraction for bpf, kprobes and ftrace does
not fit other architectures very well, and
b) that module_alloc() is not suitable as a default to base text_alloc() on,


In the latest iteration (v5) it is conditionally available only if arch
defines and fallback has been removed.


you went ahead and implemented that anyway, but only cc'ing Peter,
akpm, Masami and the mm list this time?


No problems with that. Actually each patch gets everything that
get_maintainer.pl gives with a cc cmd script, not just the ones
explicitly listed in the patch.

Should I explicitly CC you to the next version? I'm happy to grow
the list when requested.


Yes, please CC everybody that was part of the discussion last time
especially during v2, and please use a consistent CC list for the
whole patchset. It is difficult to review when you only receive patch
1 out of 6 with no mention of text_alloc() anywhere and without being
CC'd on the cover letter.

Jessica

Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper

[Jessica Yu]

+++ Jarkko Sakkinen [14/07/20 12:45 +0300]:

Rename module_alloc() to text_alloc() and module_memfree() to
text_memfree(), and move them to kernel/text.c, which is unconditionally
compiled to the kernel proper. This allows kprobes, ftrace and bpf to
allocate space for executable code without requiring to compile the modules
support (CONFIG_MODULES=y) in.

Cc: Andi Kleen 
Suggested-by: Peter Zijlstra 
Signed-off-by: Jarkko Sakkinen 


As Ard and Will have already explained, the major issue I'm having
with this is that we're taking module_alloc(), an allocator that was
originally specific to module loading, and turning it into a generic
interface to be used by other subsystems. You're pulling in all the
module loading semantics that vary by architecture and expecting it to
work as a generic text allocator. I'm not against the existence of a
generic text_alloc() but I would very much rather that module_alloc()
be left alone to the module loader and instead work on introducing a
*separate* generic text_alloc() interface that would work for its
intended users (kprobes, bpf, etc) and have existing users of
module_alloc() switch to that instead.

Jessica

Re: [PATCH 29/29] module: move the set_fs hack for flush_icache_range to m68k

[Jessica Yu]

+++ Christoph Hellwig [15/05/20 16:36 +0200]:

flush_icache_range generally operates on kernel addresses, but for some
reason m68k needed a set_fs override.  Move that into the m68k code
insted of keeping it in the module loader.

Signed-off-by: Christoph Hellwig 
Reviewed-by: Geert Uytterhoeven 
Acked-by: Geert Uytterhoeven 
---
arch/m68k/mm/cache.c | 4 
kernel/module.c  | 8 
2 files changed, 4 insertions(+), 8 deletions(-)


Thanks for cleaning this up. For module.c:

Acked-by: Jessica Yu 

Re: [PATCH v12 01/11] MODSIGN: Export module signature definitions

[Jessica Yu]

+++ Thiago Jung Bauermann [27/06/19 23:19 -0300]:

IMA will use the module_signature format for append signatures, so export
the relevant definitions and factor out the code which verifies that the
appended signature trailer is valid.

Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
and be able to use mod_check_sig() without having to depend on either
CONFIG_MODULE_SIG or CONFIG_MODULES.

Signed-off-by: Thiago Jung Bauermann 
Reviewed-by: Mimi Zohar 
Cc: Jessica Yu 
---
include/linux/module.h   |  3 --
include/linux/module_signature.h | 44 +
init/Kconfig |  6 +++-
kernel/Makefile  |  1 +
kernel/module.c  |  1 +
kernel/module_signature.c| 46 ++
kernel/module_signing.c  | 56 +---
scripts/Makefile |  2 +-
8 files changed, 106 insertions(+), 53 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 188998d3dca9..aa56f531cf1e 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -25,9 +25,6 @@
#include 
#include 

-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
-#define MODULE_SIG_STRING "~Module signature appended~\n"
-


Hi Thiago, apologies for the delay.

It looks like arch/s390/kernel/machine_kexec_file.c also relies on
MODULE_SIG_STRING being defined, so module_signature.h will need to be
included there too, otherwise we'll run into a compilation error.

Other than that, the module-related changes look good to me:

Acked-by: Jessica Yu 

Thanks!

Jessica


/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)

diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
new file mode 100644
index ..523617fc5b6a
--- /dev/null
+++ b/include/linux/module_signature.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Module signature handling.
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowe...@redhat.com)
+ */
+
+#ifndef _LINUX_MODULE_SIGNATURE_H
+#define _LINUX_MODULE_SIGNATURE_H
+
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+enum pkey_id_type {
+   PKEY_ID_PGP,/* OpenPGP generated key ID */
+   PKEY_ID_X509,   /* X.509 arbitrary subjectKeyIdentifier */
+   PKEY_ID_PKCS7,  /* Signature in PKCS#7 message */
+};
+
+/*
+ * Module signature information block.
+ *
+ * The constituents of the signature section are, in order:
+ *
+ * - Signer's name
+ * - Key identifier
+ * - Signature data
+ * - Information block
+ */
+struct module_signature {
+   u8  algo;   /* Public-key crypto algorithm [0] */
+   u8  hash;   /* Digest algorithm [0] */
+   u8  id_type;/* Key identifier type [PKEY_ID_PKCS7] */
+   u8  signer_len; /* Length of signer's name [0] */
+   u8  key_id_len; /* Length of key identifier [0] */
+   u8  __pad[3];
+   __be32  sig_len;/* Length of signature data */
+};
+
+int mod_check_sig(const struct module_signature *ms, size_t file_len,
+ const char *name);
+
+#endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/init/Kconfig b/init/Kconfig
index 8b9ffe236e4f..c2286a3c74c5 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1852,6 +1852,10 @@ config BASE_SMALL
default 0 if BASE_FULL
default 1 if !BASE_FULL

+config MODULE_SIG_FORMAT
+   def_bool n
+   select SYSTEM_DATA_VERIFICATION
+
menuconfig MODULES
bool "Enable loadable module support"
option modules
@@ -1929,7 +1933,7 @@ config MODULE_SRCVERSION_ALL
config MODULE_SIG
bool "Module signature verification"
depends on MODULES
-   select SYSTEM_DATA_VERIFICATION
+   select MODULE_SIG_FORMAT
help
  Check modules for valid signatures upon load: the signature
  is simply appended to the module. For more information see
diff --git a/kernel/Makefile b/kernel/Makefile
index 33824f0385b3..f29ae2997a43 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -58,6 +58,7 @@ endif
obj-$(CONFIG_UID16) += uid16.o
obj-$(CONFIG_MODULES) += module.o
obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signature.o
obj-$(CONFIG_KALLSYMS) += kallsyms.o
obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
obj-$(CONFIG_CRASH_CORE) += crash_core.o
diff --git a/kernel/module.c b/kernel/module.c
index 6e6712b3aaf5..2712f4d217f5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -19,6 +19,7 @@
#include 
#include 
#include 
+#include 
#include 
#include 
#include 
diff --git a/kernel/module_signature.c b/kernel/module_signature.c
new file mode 100644
index ..4224a1086b7d
--- /dev/null
+++ b/kernel/module_signature.c
@@ -0,0 +

Re: [PATCH] powerpc/modules: If mprofile-kernel is enabled add it to vermagic

[Jessica Yu]

+++ Michael Ellerman [10/05/17 16:57 +1000]:

On powerpc we can build the kernel with two different ABIs for mcount(), which
is used by ftrace. Kernels built with one ABI do not know how to load modules
built with the other ABI. The new style ABI is called "mprofile-kernel", for
want of a better name.

Currently if we build a module using the old style ABI, and the kernel with
mprofile-kernel, when we load the module we'll oops something like:

 # insmod autofs4-no-mprofile-kernel.ko
 ftrace-powerpc: Unexpected instruction f8810028 around bl _mcount
 [ cut here ]
 WARNING: CPU: 6 PID: 3759 at ../kernel/trace/ftrace.c:2024 
ftrace_bug+0x2b8/0x3c0
 CPU: 6 PID: 3759 Comm: insmod Not tainted 
4.11.0-rc3-gcc-5.4.1-00017-g5a61ef74f269 #11
 ...
 NIP [c01eaa48] ftrace_bug+0x2b8/0x3c0
 LR [c01eaff8] ftrace_process_locs+0x4a8/0x590
 Call Trace:
   alloc_pages_current+0xc4/0x1d0 (unreliable)
   ftrace_process_locs+0x4a8/0x590
   load_module+0x1c8c/0x28f0
   SyS_finit_module+0x110/0x140
   system_call+0x38/0xfc
 ...
 ftrace failed to modify
 [] 0xd2a31024
  actual:   35:65:00:48

We can avoid this by including in the vermagic whether the kernel/module was
built with mprofile-kernel. Which results in:

 # insmod autofs4-pg.ko
 autofs4: version magic
 '4.11.0-rc3-gcc-5.4.1-00017-g5a61ef74f269 SMP mod_unload modversions '
 should be
 '4.11.0-rc3-gcc-5.4.1-00017-g5a61ef74f269-dirty SMP mod_unload modversions 
mprofile-kernel'
 insmod: ERROR: could not insert module autofs4-pg.ko: Invalid module format

Signed-off-by: Michael Ellerman <m...@ellerman.id.au>


Looks good to me:

Acked-by: Jessica Yu <j...@redhat.com>


---
arch/powerpc/include/asm/module.h | 4 
1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/include/asm/module.h 
b/arch/powerpc/include/asm/module.h
index 53885512b8d3..6c0132c7212f 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -14,6 +14,10 @@
#include 


+#ifdef CC_USING_MPROFILE_KERNEL
+#define MODULE_ARCH_VERMAGIC   "mprofile-kernel"
+#endif
+
#ifndef __powerpc64__
/*
 * Thanks to Paul M for explaining this.
--
2.7.4

Re: [PATCH] module: set __jump_table alignment to 8

[Jessica Yu]

+++ David Daney [02/03/17 11:24 -0800]:

On 03/02/2017 10:26 AM, Jessica Yu wrote:

+++ Steven Rostedt [02/03/17 13:11 -0500]:


Can I get an Ack from a module maintainer?


Acked-by: Jessica Yu <j...@redhat.com>

Thanks!

Jessica


Thanks Jessica,

Can you also add scripts/module-common.lds to MAINTAINERS so that 
get_maintainers.pl will indicate that Jessica Yu and Rusty Russell be 
CCed on things like this in the future?


Sure thing. Thanks for the heads up!

Jessica


On Wed,  1 Mar 2017 14:04:53 -0800
David Daney <david.da...@cavium.com> wrote:


For powerpc the __jump_table section in modules is not aligned, this
causes a WARN_ON() splat when loading a module containing a
__jump_table.

Strict alignment became necessary with commit 3821fd35b58d
("jump_label: Reduce the size of struct static_key"), currently in
linux-next, which uses the two least significant bits of pointers to
__jump_table elements.

Fix by forcing __jump_table to 8, which is the same alignment used for
this section in the kernel proper.

Signed-off-by: David Daney <david.da...@cavium.com>
Tested-by: Sachin Sant <sach...@linux.vnet.ibm.com>
---
scripts/module-common.lds | 2 ++
1 file changed, 2 insertions(+)

diff --git a/scripts/module-common.lds b/scripts/module-common.lds
index 73a2c7d..53234e8 100644
--- a/scripts/module-common.lds
+++ b/scripts/module-common.lds
@@ -19,4 +19,6 @@ SECTIONS {

. = ALIGN(8);
.init_array0 : { *(SORT(.init_array.*)) *(.init_array) }
+
+__jump_table0 : ALIGN(8) { KEEP(*(__jump_table)) }
}

Re: [PATCH] module: set __jump_table alignment to 8

[Jessica Yu]

+++ Steven Rostedt [02/03/17 13:11 -0500]:


Can I get an Ack from a module maintainer?


Acked-by: Jessica Yu <j...@redhat.com>

Thanks!

Jessica


On Wed,  1 Mar 2017 14:04:53 -0800
David Daney <david.da...@cavium.com> wrote:


For powerpc the __jump_table section in modules is not aligned, this
causes a WARN_ON() splat when loading a module containing a __jump_table.

Strict alignment became necessary with commit 3821fd35b58d
("jump_label: Reduce the size of struct static_key"), currently in
linux-next, which uses the two least significant bits of pointers to
__jump_table elements.

Fix by forcing __jump_table to 8, which is the same alignment used for
this section in the kernel proper.

Signed-off-by: David Daney <david.da...@cavium.com>
Tested-by: Sachin Sant <sach...@linux.vnet.ibm.com>
---
 scripts/module-common.lds | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/module-common.lds b/scripts/module-common.lds
index 73a2c7d..53234e8 100644
--- a/scripts/module-common.lds
+++ b/scripts/module-common.lds
@@ -19,4 +19,6 @@ SECTIONS {

. = ALIGN(8);
.init_array 0 : { *(SORT(.init_array.*)) *(.init_array) }
+
+   __jump_table0 : ALIGN(8) { KEEP(*(__jump_table)) }
 }

Re: modversions: redefine kcrctab entries as 32-bit values

[Jessica Yu]

+++ Jessica Yu [02/02/17 22:54 -0500]:

+++ Ard Biesheuvel [24/01/17 16:16 +]:

This v4 is a followup to [0] 'modversions: redefine kcrctab entries as
relative CRC pointers', but since relative CRC pointers do not work in
modules, and are actually only needed by powerpc with CONFIG_RELOCATABLE=y,
I have made it a Kconfig selectable feature instead.

Patch #1 introduces the MODULE_REL_CRCS Kconfig symbol, and adds the kbuild
handling of it, i.e., modpost, genksyms and kallsyms.

Patch #2 switches all architectures to 32-bit CRC entries in kcrctab, where
all architectures except powerpc with CONFIG_RELOCATABLE=y use absolute ELF
symbol references as before.

v4: make relative CRCs kconfig selectable
  use absolute CRC symbols in modules regardless of kconfig selection
  split into two patches


This asymmetry threw me off a bit, especially the Kconfig naming (only
vmlinux crcs get the relative offsets, and only on powerpc atm, but all
modules keep the absolute syms, but it is called MODULE_REL_CRCS...),
if we keep this asymmetric crc treatment, it would be really nice to
note this discrepancy this somewhere, perhaps in the Kconfig, to keep
our heads from spinning :-)

I'm still catching up on the previous discussion threads, but can you
explain a bit more why you switched away from full blown relative crcs
from your last patchset [1]? I had lightly tested your v3 on ppc64le
previously, and relative offsets with modules seemed to worked very
well. I'm probably missing something very obvious.


Ah, I just saw your other comment about other arches not having support
for the rel32 offsets :-/

The asymmetry still bothers me though. Can't we have something that just
switches relative crcs on and off, that applies to *both* vmlinux and modules?
Then we can get rid of the crc_owner check in check_version() and just have
something like:

  if (IS_ENABLED(CONFIG_RELATIVE_CRCS))
  crcval = resolve_crc(crc);

Also we could get rid of the '&& !defined(MODULE)' checks scattered in
export.h. Then the arches that want relative crcs and that *do* have rel32
relocation support can turn relative crcs on, and powerpc can enable it, right?

Would that work or is there another reason this won't work with modules
(assuming that the arches that select this option support the relative
offsets)?

Jessica

Re: modversions: redefine kcrctab entries as 32-bit values

[Jessica Yu]

+++ Ard Biesheuvel [24/01/17 16:16 +]:

This v4 is a followup to [0] 'modversions: redefine kcrctab entries as
relative CRC pointers', but since relative CRC pointers do not work in
modules, and are actually only needed by powerpc with CONFIG_RELOCATABLE=y,
I have made it a Kconfig selectable feature instead.

Patch #1 introduces the MODULE_REL_CRCS Kconfig symbol, and adds the kbuild
handling of it, i.e., modpost, genksyms and kallsyms.

Patch #2 switches all architectures to 32-bit CRC entries in kcrctab, where
all architectures except powerpc with CONFIG_RELOCATABLE=y use absolute ELF
symbol references as before.

v4: make relative CRCs kconfig selectable
   use absolute CRC symbols in modules regardless of kconfig selection
   split into two patches


This asymmetry threw me off a bit, especially the Kconfig naming (only
vmlinux crcs get the relative offsets, and only on powerpc atm, but all
modules keep the absolute syms, but it is called MODULE_REL_CRCS...),
if we keep this asymmetric crc treatment, it would be really nice to
note this discrepancy this somewhere, perhaps in the Kconfig, to keep
our heads from spinning :-)

I'm still catching up on the previous discussion threads, but can you
explain a bit more why you switched away from full blown relative crcs
from your last patchset [1]? I had lightly tested your v3 on ppc64le
previously, and relative offsets with modules seemed to worked very
well. I'm probably missing something very obvious.

[1] http://marc.info/?l=linux-arch=148493613415294=2


v3: emit CRCs into .rodata rather than .rodata.modver, given that the latter
   will be emitted with read-write permissions, making the CRCs end up in a
   writable module segment.

   fold the modpost fix to ensure that the section address is only substracted
   from the symbol address when the ELF object in question is fully linked
   (i.e., ET_DYN or ET_EXEC, and not ET_REL)

v2: update modpost as well, so that genksyms no longer has to emit symbols
   for both the actual CRC value and the reference to where it is stored
   in the image

[0] http://marc.info/?l=linux-arch=148493613415294=2

Ard Biesheuvel (2):
 kbuild: modversions: add infrastructure for emitting relative CRCs
 modversions: treat symbol CRCs as 32 bit quantities

arch/powerpc/Kconfig  |  1 +
arch/powerpc/include/asm/module.h |  4 --
arch/powerpc/kernel/module_64.c   |  8 
include/asm-generic/export.h  | 11 ++---
include/linux/export.h| 14 ++
include/linux/module.h| 14 +++---
init/Kconfig  |  4 ++
kernel/module.c   | 45 ++--
scripts/Makefile.build|  2 +
scripts/genksyms/genksyms.c   | 19 ++---
scripts/kallsyms.c| 12 ++
scripts/mod/modpost.c | 10 +
12 files changed, 93 insertions(+), 51 deletions(-)

--
2.7.4

Re: livepatch: change to a per-task consistency model

[Jessica Yu]

+++ Josh Poimboeuf [28/04/16 15:44 -0500]:

[snip]


diff --git a/Documentation/livepatch/livepatch.txt 
b/Documentation/livepatch/livepatch.txt
index 6c43f6e..bee86d0 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -72,7 +72,8 @@ example, they add a NULL pointer or a boundary check, fix a 
race by adding
a missing memory barrier, or add some locking around a critical section.
Most of these changes are self contained and the function presents itself
the same way to the rest of the system. In this case, the functions might
-be updated independently one by one.
+be updated independently one by one.  (This can be done by setting the
+'immediate' flag in the klp_patch struct.)

But there are more complex fixes. For example, a patch might change
ordering of locking in multiple functions at the same time. Or a patch
@@ -86,20 +87,103 @@ or no data are stored in the modified structures at the 
moment.
The theory about how to apply functions a safe way is rather complex.
The aim is to define a so-called consistency model. It attempts to define
conditions when the new implementation could be used so that the system
-stays consistent. The theory is not yet finished. See the discussion at
-http://thread.gmane.org/gmane.linux.kernel/1823033/focus=1828189
-
-The current consistency model is very simple. It guarantees that either
-the old or the new function is called. But various functions get redirected
-one by one without any synchronization.
-
-In other words, the current implementation _never_ modifies the behavior
-in the middle of the call. It is because it does _not_ rewrite the entire
-function in the memory. Instead, the function gets redirected at the
-very beginning. But this redirection is used immediately even when
-some other functions from the same patch have not been redirected yet.
-
-See also the section "Limitations" below.
+stays consistent.
+
+Livepatch has a consistency model which is a hybrid of kGraft and
+kpatch:  it uses kGraft's per-task consistency and syscall barrier
+switching combined with kpatch's stack trace switching.  There are also
+a number of fallback options which make it quite flexible.
+
+Patches are applied on a per-task basis, when the task is deemed safe to
+switch over.  When a patch is enabled, livepatch enters into a
+transition state where tasks are converging to the patched state.
+Usually this transition state can complete in a few seconds.  The same
+sequence occurs when a patch is disabled, except the tasks converge from
+the patched state to the unpatched state.
+
+An interrupt handler inherits the patched state of the task it
+interrupts.  The same is true for forked tasks: the child inherits the
+patched state of the parent.
+
+Livepatch uses several complementary approaches to determine when it's
+safe to patch tasks:
+
+1. The first and most effective approach is stack checking of sleeping
+   tasks.  If no affected functions are on the stack of a given task,
+   the task is patched.  In most cases this will patch most or all of
+   the tasks on the first try.  Otherwise it'll keep trying
+   periodically.  This option is only available if the architecture has
+   reliable stacks (CONFIG_RELIABLE_STACKTRACE and
+   CONFIG_STACK_VALIDATION).
+
+2. The second approach, if needed, is kernel exit switching.  A
+   task is switched when it returns to user space from a system call, a
+   user space IRQ, or a signal.  It's useful in the following cases:
+
+   a) Patching I/O-bound user tasks which are sleeping on an affected
+  function.  In this case you have to send SIGSTOP and SIGCONT to
+  force it to exit the kernel and be patched.


See below -


+   b) Patching CPU-bound user tasks.  If the task is highly CPU-bound
+  then it will get patched the next time it gets interrupted by an
+  IRQ.
+   c) Applying patches for architectures which don't yet have
+  CONFIG_RELIABLE_STACKTRACE.  In this case you'll have to signal
+  most of the tasks on the system.  However this isn't a complete
+  solution, because there's currently no way to patch kthreads
+  without CONFIG_RELIABLE_STACKTRACE.
+
+   Note: since idle "swapper" tasks don't ever exit the kernel, they
+   instead have a kpatch_patch_task() call in the idle loop which allows
+   them to patched before the CPU enters the idle state.
+
+3. A third approach (not yet implemented) is planned for the case where
+   a kthread is sleeping on an affected function.  In that case we could
+   kick the kthread with a signal and then try to patch the task from
+   the to-be-patched function's livepatch ftrace handler when it
+   re-enters the function.  This will require
+   CONFIG_RELIABLE_STACKTRACE.
+
+All the above approaches may be skipped by setting the 'immediate' flag
+in the 'klp_patch' struct, which will patch all tasks immediately.  This
+can be useful if the patch doesn't change any function or data
+semantics.  Note that, even 

Re: livepatch: Add some basic LivePatch documentation

[Jessica Yu]

+++ Petr Mladek [25/04/16 17:14 +0200]:

LivePatch framework deserves some documentation, definitely.
This is an attempt to provide some basic info. I hope that
it will be useful for both LivePatch producers and also
potential developers of the framework itself.

Signed-off-by: Petr Mladek <pmla...@suse.com>


Nice work Petr. I read through it and didn't notice any other
additional issues. With Chris', Balbir's and Josh's comments taken
into account:

Acked-by: Jessica Yu <j...@redhat.com>

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: Live patching for powerpc

[Jessica Yu]

+++ Miroslav Benes [14/04/16 15:28 +0200]:

On Wed, 13 Apr 2016, Jessica Yu wrote:


+++ Miroslav Benes [13/04/16 15:01 +0200]:
> On Wed, 13 Apr 2016, Michael Ellerman wrote:
>
> > This series adds live patching support for powerpc (ppc64le only ATM).
> >
> > It's unchanged since the version I posted on March 24, with the exception
> > that
> > I've dropped the first patch, which was a testing-only patch.
> >
> > If there's no further comments I'll put this in a topic branch in the next
> > day
> > or two and Jiri & I will both merge that into next.
>
> Hi,
>
> I'll definitely give it a proper look today or tomorrow, but there is one
> thing that needs to be solved. The patch set from Jessica reworking
> relocations for live patching is now merged in our for-next branch. This
> means that we need to find out if there is something in struct
> mod_arch_specific for powerpc which needs to be preserved and do it.
>

I took a look around the powerpc module.c code and it looks like the
mod_arch_specific stuff should be fine, since it is statically allocated
in the module struct (unlike the situation in s390, where
mod->arch.syminfo was vmalloc'd and we had to delay the free).
However I'm not familiar with the powerpc code so I need to dig around
a bit more to be 100% sure.


I came to the same conclusion. There is only struct bug_entry *bug_table
in mod_arch_specific but it looks unimportant wrt relocations.


Yeah, I think we are fine. As long as none of the values in
mod_arch_specific are "cleared," and I don't see that happening
anywhere.


A second concern I have is that apply_relocate_add() relies on
sections like .stubs and .toc (for 64-bit) and .init.plt and .plt
sections (for 32-bit). In order for apply_relocate_add() to work for
livepatch, we must make sure these sections aren't thrown away and are
not in init module memory since this memory will be freed at the end
of module load (see how INIT_OFFSET_MASK is used in kernel/module.c).
As long as these sections are placed in module core memory, we will be
OK. I need to think about this a bit more.


I knew I shouldn't have opened arch/powerpc/kernel/module*.c.

We could always hack sh_flags of those sections in
module_arch_frob_sections() to make them stay.



I think we are fine here too. The onus would be on the patch build
tool (e.g., kpatch) to set the sh_flags to SHF_ALLOC, like we
already do to keep the klp relocation sections in memory :-)

For the 32-bit module code, I don't believe we would need to preserve
the .init.plt section for livepatch's call to apply_relocate_add(),
since relocations to init sections should've been applied during
module initialization, and we don't patch those types of functions.
Please correct me if my understanding is off.

Jessica

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: Live patching for powerpc

[Jessica Yu]

+++ Miroslav Benes [13/04/16 15:01 +0200]:

On Wed, 13 Apr 2016, Michael Ellerman wrote:


This series adds live patching support for powerpc (ppc64le only ATM).

It's unchanged since the version I posted on March 24, with the exception that
I've dropped the first patch, which was a testing-only patch.

If there's no further comments I'll put this in a topic branch in the next day
or two and Jiri & I will both merge that into next.


Hi,

I'll definitely give it a proper look today or tomorrow, but there is one
thing that needs to be solved. The patch set from Jessica reworking
relocations for live patching is now merged in our for-next branch. This
means that we need to find out if there is something in struct
mod_arch_specific for powerpc which needs to be preserved and do it.



I took a look around the powerpc module.c code and it looks like the
mod_arch_specific stuff should be fine, since it is statically allocated
in the module struct (unlike the situation in s390, where
mod->arch.syminfo was vmalloc'd and we had to delay the free).
However I'm not familiar with the powerpc code so I need to dig around
a bit more to be 100% sure.

A second concern I have is that apply_relocate_add() relies on
sections like .stubs and .toc (for 64-bit) and .init.plt and .plt
sections (for 32-bit). In order for apply_relocate_add() to work for
livepatch, we must make sure these sections aren't thrown away and are
not in init module memory since this memory will be freed at the end
of module load (see how INIT_OFFSET_MASK is used in kernel/module.c).
As long as these sections are placed in module core memory, we will be
OK. I need to think about this a bit more.

Third and unrelated comment: the klp_write_module_reloc stub isn't
needed anymore :-)

Thanks,
Jessica
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: livepatch: Add some basic LivePatch documentation

[Jessica Yu]

+++ Petr Mladek [09/03/16 15:01 +0100]:

LivePatch framework deserves some documentation, definitely.
This is an attempt to provide some basic info. I hope that
it will be useful for both LivePatch producers and also
potential developers of the framework itself.

Signed-off-by: Petr Mladek 
---

This patch was motivated by the LivePatch port for PPC. The guys
might want to document some PPC-specific limitations on top of it.

I am sure that it is far from perfect. But I hope that it is
an acceptable start that can be improved later. I hope that
I did not write that many factual mistakes.

I wrote only some generic info about the consistency model.
I am not sure if we have agreed on some specification yet.

I am sorry for grammar mistakes. I hope that some hairs will
stay on your head if you are sensitive.


Thanks for getting us started on much-needed documentation. :-) It's a
good starting point.

I skimmed the document and it looks well organized. There are just some areas
that need more clarification. I just provided some suggested rewordings, though
they are subjective so you could either take them or leave them.



Documentation/livepatch/livepatch.txt | 277 ++
MAINTAINERS   |   1 +
2 files changed, 278 insertions(+)
create mode 100644 Documentation/livepatch/livepatch.txt

diff --git a/Documentation/livepatch/livepatch.txt 
b/Documentation/livepatch/livepatch.txt
new file mode 100644
index ..28e8047abb61
--- /dev/null
+++ b/Documentation/livepatch/livepatch.txt
@@ -0,0 +1,277 @@
+=
+LivePatch
+=
+
+This document outlines basic information about kernel LivePatching.
+
+Table of Contents:
+
+1. Motivation
+2. Kprobes, Ftrace, LivePatching
+3. Consistency model
+4. LivePatch life-cycle
+   4.1. Registration
+   4.2. Enabling
+   4.3. Disabling
+   4.4. Unregistration
+5. Livepatch module
+   5.1. New functions
+   5.2. Metadata
+   5.3. Module handling
+6. Sysfs
+7. Limitations
+
+
+1. Motivation
+=
+
+There are situations when people are really reluctant to reboot a system.
+It might be because the computer is in the middle of a complex scientific
+computation. Or the system is busy handling customer requests in the high
+season.
+
+On the other hand, people also want to keep the system stable and secure.
+This is where LivePatch infrastructure comes handy.



It allows to redirect selected function calls to a fixed
implementation without rebooting the system.


It allows selected function calls to be redirected to a fixed
implementation without requiring a system reboot.


+
+
+2. Kprobes, Ftrace, LivePatching
+



+Linux kernel has more ways how to redirect an existing code into a new one.
+It happens with kernel probes, function tracing, and LivePatching:


There are multiple mechanisms in the Linux kernel that are directly related to
redirection of code execution; namely: kernel probes, function tracing, and
livepatching:


+
+  + The kernel probes are the most generic way. The code can be redirected
+by putting an interrupt instruction instead of any instruction.
+
+  + The function tracer calls the code from a predefined location that is
+close the function entry. The location is generated by the compiler,
+see -pg gcc option.
+
+  + LivePatching typically needs to redirect the code at the very beginning
+of the function entry before the function parameters or the stack
+are anyhow muffled.
+
+All three approaches need to modify the existing code at runtime. Therefore
+they need to be aware of each other and do not step over othres' toes. Most
+of these problems are solved by using the dynamic ftrace framework as a base.
+A Kprobe is registered as a ftrace handler when the function entry is probed,
+see CONFIG_KPROBES_ON_FTRACE. Also an alternative function from a live patch
+is called from a custom ftrace handler. But there are some limitations,
+see below.
+
+
+3. Consistency model
+
+
+Functions are there for a reason. They take some input parameters, get or
+release locks, read, process, and even write some data in a defined way,
+have return values. By other words, each function has a defined semantic.
+
+Many fixes do not change the semantic of the modified functions. For example,
+they add a NULL pointer or a boundary check, fix a race by adding a missing
+memory barrier, or add some locking about a critical section. Most of these
+changes are self contained and the function present itself the same way
+to the rest of the system. In this case, the functions might be updated
+independently one by one.
+
+But there are more complex fixes. For example, a patch might change
+ordering of locking in more functions at the same time. Or a patch
+might exchange meaning of some temporary structures and update
+all the relevant functions. In this case, the affected unit
+(thread, whole kernel) need to start using 

Re: Implement kernel live patching for ppc64le (ABIv2)

[Jessica Yu]

+++ Miroslav Benes [26/01/16 15:14 +0100]:


[ Jessica added to CC list so she is aware that there are plans to
implement livepatch on ppc64le ]

On Tue, 26 Jan 2016, Torsten Duwe wrote:


On Tue, Jan 26, 2016 at 11:50:25AM +0100, Miroslav Benes wrote:
> > + */
> > +int klp_write_module_reloc(struct module *mod, unsigned long type,
> > + unsigned long loc, unsigned long value)
> > +{
> > + /* This requires infrastructure changes; we need the loadinfos. */
> > + pr_err("lpc_write_module_reloc not yet supported\n");
>
> This is a nit, but there is no lpc_write_module_reloc. It should be
> klp_write_module_reloc.

Indeed. Michael, feel free to fix this on the fly or not. It needs to
disappear anyway and be replaced with functionality.


Or not at all thanks to Jessica's effort [1].

Miroslav

[1] http://lkml.kernel.org/g/1452281304-28618-1-git-send-email-j...@redhat.com


Miroslav, thanks for the CC. Indeed, if things go well, there may be
no need to implement klp_write_module_reloc() anymore in the near
future. :-)

Jessica
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev