Re: [PATCH 2/6] module: use list_for_each_entry_rcu() on find_module_all()

[Miroslav Benes]

On Thu, 18 May 2017, Luis R. Rodriguez wrote:

> The module list has been using RCU in a lot of other calls
> for a while now, we just overlooked changing this one over to
> use RCU.
> 
> Signed-off-by: Luis R. Rodriguez 
> ---
>  kernel/module.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 4a3665f8f837..70f494638974 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -602,7 +602,7 @@ static struct module *find_module_all(const char *name, 
> size_t len,
>  
>   module_assert_mutex_or_preempt();
>  
> - list_for_each_entry(mod, , list) {
> + list_for_each_entry_rcu(mod, , list) {
>   if (!even_unformed && mod->state == MODULE_STATE_UNFORMED)
>   continue;
>   if (strlen(mod->name) == len && !memcmp(mod->name, name, len))

This makes sense. It would not be an issue if all callers of 
find_module_all() held module_mutex, but module_kallsyms_lookup_name() 
does not.

There is one more occurrence of just list_for_each_entry(mod, , 
list) in kernel/module.c -- in module_kallsyms_on_each_symbol(). But that 
one is safe because we have to hold the mutex there.

Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation/livepatch: remove the limitation for schedule() patching

[Miroslav Benes]

On Fri, 6 Jan 2017, Josh Poimboeuf wrote:

> On Fri, Jan 06, 2017 at 03:00:45PM +0100, Miroslav Benes wrote:
> > 
> > 2. reversion of the process does not work as expected. The kernel
> > crashes after the removal of the module. A task very likely slept in
> > schedule and was not migrated properly. It might be because of the races
> > in klp_reverse_transition() described by Petr, or might be somewhere
> > else. I'll look into it.
> 
> Hm, will be interesting to see the cause of this...

The absence of the patched schedule() on the stack was the cause. 
klp_try_switch_task() thus did not see it and happily migrated the task. 

The reason is funny. One cannot patch __schedule() (which is of 
interested) because of the notrace attribute. So all the callers need to 
be processed. I tried to make my life easier and patched only schedule(). 
GCC then inlined new __schedule() to the new schedule(). When I added 
noinline attribute to the new __schedule() everything was fine (because 
suddenly new schedule() was on the stack as expected).

There is still one thing which I don't understand. Why __schedule() 
(patched or the original) is not on the stack. The actual "sleep" 
should happen in __switch_to_asm() which is C function now. And there is a 
call to __switch_to_asm() in __schedule(). __schedule() thus should be on 
the stack, shouldn't it? What am I missing? __switch_to_asm() pushes %rbp 
on the stack...

Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation/livepatch: remove the limitation for schedule() patching

[Miroslav Benes]

On Fri, 6 Jan 2017, Petr Mladek wrote:

> On Fri 2017-01-06 15:00:45, Miroslav Benes wrote:
> > The Limitations section of the documentation describes the impossibility
> > to livepatch anything that is inlined to __schedule() function. This had
> > been true till 4.9 kernel came. Thanks to commit 0100301bfdf5
> > ("sched/x86: Rewrite the switch_to() code") from Brian Gerst there is
> > __switch_to_asm function now (implemented in assembly) called properly
> > from context_switch(). RIP is thus saved on the stack and a task would
> > return to proper version of __schedule() et al. functions.
> > 
> > Of course __switch_to_asm() is not patchable for the reason described in
> > the section. But there is no __fentry__ call and I cannot imagine a
> > reason to do it anyway.
> > 
> > Therefore, remove the paragraphs from the section.
> > 
> > Signed-off-by: Miroslav Benes <mbe...@suse.cz>
> 
> It is great to get a feature for free ;-)
> 
> Reviewed-by: Petr Mladek <pmla...@suse.com>
> 
> Best Regards,
> Petr
> 
> ---
> > FWIW, I also tested this to be sure on top of the consistency model
> > patch set. I patched schedule() function which calls __schedule() (it is
> > impossible to patch it directly due to notrace attribute). It works well
> > except...
> > 
> > 1. the patching process does not finish, because many tasks sleep in
> > schedule. STOP/CONT signal does not help. I'll investigate.
> 
> Are these userspace processes or kthreads? Kthreads would cause
> problems because they do not handle signals.

Userspace processes, but I take it back. Stupid typo in my script. It 
works as expected. Kthreads sleeping in schedule() are of course there and 
a signal does not help.

Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Documentation/livepatch: remove the limitation for schedule() patching

[Miroslav Benes]

The Limitations section of the documentation describes the impossibility
to livepatch anything that is inlined to __schedule() function. This had
been true till 4.9 kernel came. Thanks to commit 0100301bfdf5
("sched/x86: Rewrite the switch_to() code") from Brian Gerst there is
__switch_to_asm function now (implemented in assembly) called properly
from context_switch(). RIP is thus saved on the stack and a task would
return to proper version of __schedule() et al. functions.

Of course __switch_to_asm() is not patchable for the reason described in
the section. But there is no __fentry__ call and I cannot imagine a
reason to do it anyway.

Therefore, remove the paragraphs from the section.

Signed-off-by: Miroslav Benes <mbe...@suse.cz>
---
FWIW, I also tested this to be sure on top of the consistency model
patch set. I patched schedule() function which calls __schedule() (it is
impossible to patch it directly due to notrace attribute). It works well
except...

1. the patching process does not finish, because many tasks sleep in
schedule. STOP/CONT signal does not help. I'll investigate.

2. reversion of the process does not work as expected. The kernel
crashes after the removal of the module. A task very likely slept in
schedule and was not migrated properly. It might be because of the races
in klp_reverse_transition() described by Petr, or might be somewhere
else. I'll look into it.

 Documentation/livepatch/livepatch.txt | 19 ---
 1 file changed, 19 deletions(-)

diff --git a/Documentation/livepatch/livepatch.txt 
b/Documentation/livepatch/livepatch.txt
index f5967316deb9..7f04e13ec53d 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -329,25 +329,6 @@ See Documentation/ABI/testing/sysfs-kernel-livepatch for 
more details.
 by "notrace".
 
 
-  + Anything inlined into __schedule() can not be patched.
-
-The switch_to macro is inlined into __schedule(). It switches the
-context between two processes in the middle of the macro. It does
-not save RIP in x86_64 version (contrary to 32-bit version). Instead,
-the currently used __schedule()/switch_to() handles both processes.
-
-Now, let's have two different tasks. One calls the original
-__schedule(), its registers are stored in a defined order and it
-goes to sleep in the switch_to macro and some other task is restored
-using the original __schedule(). Then there is the second task which
-calls patched__schedule(), it goes to sleep there and the first task
-is picked by the patched__schedule(). Its RSP is restored and now
-the registers should be restored as well. But the order is different
-in the new patched__schedule(), so...
-
-There is work in progress to remove this limitation.
-
-
   + Livepatch modules can not be removed.
 
 The current implementation just redirects the functions at the very
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/5] (mostly) Arch-independent livepatch

[Miroslav Benes]

On Tue, 22 Mar 2016, Jessica Yu wrote:

> v6:
>  - Since we hard-code the field widths for the objname and symbol name
>for the sscanf() calls, which are supposed to correspond to the values
>of MODULE_NAME_LEN and KSYM_NAME_LEN, use BUILD_BUG_ON() to detect when
>the values of these constants deviate from the expected values.
>  - Squash the sample livepatch module patch into patch 4
>("livepatch: reuse module loader code to write relocations") so 
>git bisects don't break
>  - Don't need the klp_buf struct, just use plain char arrays to hold the
>output of sscanf(). Also, no need to clear the bufs after every
>invocation, as sscanf() takes care to put a null byte at the end of
>the bufs.
>  - Fix compiler kbuild errors for the !CONFIG_LIVEPATCH case
>  - Fixed some small module.c nits

FWIW I am fine with all the changes and my Reviewed-by tag is still valid.

Great job, Jessica.

Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 6/6] Documentation: livepatch: outline Elf format and requirements for patch modules

[Miroslav Benes]

On Wed, 16 Mar 2016, Jessica Yu wrote:

> Document livepatch module requirements and the special Elf constants patch
> modules use.
> 
> Signed-off-by: Jessica Yu <j...@redhat.com>

Fantastic to have it

Acked-by: Miroslav Benes <mbe...@suse.cz>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 5/6] samples: livepatch: mark as livepatch module

[Miroslav Benes]

On Wed, 16 Mar 2016, Jessica Yu wrote:

> Mark the module as a livepatch module so that the module loader can
> appropriately identify and initialize it.
> 
> Signed-off-by: Jessica Yu <j...@redhat.com>

Reviewed-by: Miroslav Benes <mbe...@suse.cz>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations

[Miroslav Benes]

> > +/* .klp.sym.[objname].symbol_name,sympos */
> > +static int klp_get_sym_objname(char *s, char **result)
> > +{
> 
> Do we need result to be a double-pointer? If I am not mistaken just 'char 
> *result' could be sufficient. You check the return value, so result could 
> be NULL or objname as found. No?

Um, no. We need this. Sorry for the noise.

Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v4 4/6] livepatch: reuse module loader code to write relocations

[Miroslav Benes]

On Tue, 9 Feb 2016, Petr Mladek wrote:

> On Wed 2016-02-03 20:11:09, Jessica Yu wrote:
> > Reuse module loader code to write relocations, thereby eliminating the need
> > for architecture specific relocation code in livepatch. Specifically, reuse
> > the apply_relocate_add() function in the module loader to write relocations
> > instead of duplicating functionality in livepatch's arch-dependent
> > klp_write_module_reloc() function.
> > 
> > In order to accomplish this, livepatch modules manage their own relocation
> > sections (marked with the SHF_RELA_LIVEPATCH section flag) and
> > livepatch-specific symbols (marked with SHN_LIVEPATCH symbol section
> > index). To apply livepatch relocation sections, livepatch symbols
> > referenced by relocs are resolved and then apply_relocate_add() is called
> > to apply those relocations.
> > 
> > In addition, remove x86 livepatch relocation code and the s390
> > klp_write_module_reloc() function stub. They are no longer needed since
> > relocation work has been offloaded to module loader.
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 7aa975d..c1fe57c 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -28,6 +28,9 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> > +#include 
> >  #include 
> >  
> >  /**
> > @@ -87,6 +90,166 @@ static bool klp_is_object_loaded(struct klp_object *obj)
> > return !obj->name || obj->mod;
> >  }
> >  
> > +/*
> > + * Check if a livepatch symbol is formatted properly.
> > + *
> > + * See Documentation/livepatch/module-elf-format.txt for a
> > + * detailed outline of requirements.
> > + */
> > +static int klp_check_symbol_format(struct module *pmod, Elf_Sym *sym)
> > +{
> > +   size_t len;
> > +   char *s, *objname, *symname;
> > +
> > +   if (sym->st_shndx != SHN_LIVEPATCH)
> > +   return -EINVAL;
> > +
> > +   /*
> > +* Livepatch symbol names must follow this format:
> > +* .klp.sym.objname.symbol_name,sympos
> > +*/
> > +   s = pmod->strtab + sym->st_name;
> > +   /* [.klp.sym.]objname.symbol_name,sympos */
> > +   if (!s || strncmp(s, KLP_SYM_PREFIX, KLP_SYM_PREFIX_LEN))
> > +   return -EINVAL;
> > +
> > +   /* .klp.sym.[objname].symbol_name,sympos */
> > +   objname = s + KLP_SYM_PREFIX_LEN;
> > +   len = strcspn(objname, ".");
> > +   if (!(len > 0))
> > +   return -EINVAL;
> > +
> > +   /* .klp.sym.objname.symbol_name,[sympos] */
> > +   if (!strchr(s, ','))
> > +   return -EINVAL;
> > +
> > +   /* .klp.sym.objname.[symbol_name],sympos */
> > +   symname = objname + len + 1;
> > +   len = strcspn(symname, ",");
> > +   if (!(len > 0))
> > +   return -EINVAL;
> > +
> > +   return 0;
> > +}
> > +
> > +/*
> > + * Check if a livepatch relocation section is formatted properly.
> > + *
> > + * See Documentation/livepatch/module-elf-format.txt for a
> > + * detailed outline of requirements.
> > + */
> > +static int klp_check_relasec_format(struct module *pmod, Elf_Shdr *relasec)
> > +{
> > +   char *secname;
> > +   size_t len;
> > +
> > +   secname = pmod->klp_info->secstrings + relasec->sh_name;
> > +   /* [.klp.rela.]objname.section_name */
> > +   if (!secname || strncmp(secname, KLP_RELASEC_PREFIX,
> > +   KLP_RELASEC_PREFIX_LEN))
> > +   return -EINVAL;
> > +
> > +   /* .klp.rela.[objname].section_name */
> > +   len = strcspn(secname + KLP_RELASEC_PREFIX_LEN, ".");
> > +   if (!(len > 0))
> > +   return -EINVAL;
> > +
> > +   return 0;
> > +}
> > +
> > +/*
> > + * Check if obj->name matches the objname encoded in the rela
> > + * section name (.klp.rela.[objname].section_name)
> > + *
> > + * Must pass klp_check_relasec_format() before calling this.
> > + */
> > +static bool klp_relasec_matches_object(struct module *pmod, Elf_Shdr 
> > *relasec,
> > +  struct klp_object *obj)
> > +{
> > +   size_t len;
> > +   const char *obj_objname, *sec_objname, *secname;
> > +
> > +   secname = pmod->klp_info->secstrings + relasec->sh_name;
> > +   /* .klp.rela.[objname].section_name */
> > +   sec_objname = secname + KLP_RELASEC_PREFIX_LEN;
> > +   obj_objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > +
> > +   /* Get length of the objname encoded in the section name */
> > +   len = strcspn(sec_objname, ".");
> > +
> > +   if (strlen(obj_objname) != len)
> > +   return false;
> > +
> > +   return strncmp(sec_objname, obj_objname, len) ? false : true;
> > +}
> > +
> > +/*
> > + * klp_get_* helper functions
> > + *
> > + * klp_get_* functions extract different components of the name
> > + * of a livepatch symbol. The full symbol name from the strtab
> > + * is passed in as parameter @s, and @result is filled in with
> > + * the extracted component.
> > + *
> > + * These functions assume a correctly formatted symbol and the
> > + * klp_check_symbol_format() test *must* pass before calling any
> > + * of 

Re: [RFC PATCH v4 0/6] (mostly) Arch-independent livepatch

[Miroslav Benes]

On Wed, 3 Feb 2016, Jessica Yu wrote:

> Jessica Yu (6):
>   Elf: add livepatch-specific Elf constants
>   module: preserve Elf information for livepatch modules
>   module: s390: keep mod_arch_specific for livepatch modules
>   livepatch: reuse module loader code to write relocations
>   samples: livepatch: mark as livepatch module
>   Documentation: livepatch: outline Elf format and requirements for
> patch modules

Hi,

I walked through the code and it looks good except for several minor 
things in the fourth patch (livepatch: reuse module loader code to write 
relocations). I'd propose to send the next version as a regular PATCH set 
and not RFC. We can start collecting Reviews and Acks. Hopefully it won't 
take more than one or two iterations. Would that be ok with everyone?

Thanks,
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html