* Re: [PATCH v2 00/11] Add Apple M1 support to PASemi i2c driver
From: Christian Zigotzky @ 2021-10-13 8:06 UTC (permalink / raw)
To: Wolfram Sang, Sven Peter, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Olof Johansson,
Arnd Bergmann, Hector Martin, Mohamed Mediouni, Stan Skowronek,
Mark Kettenis, Alyssa Rosenzweig, linux-arm-kernel, linuxppc-dev,
linux-i2c, linux-kernel, R.T.Dickinson, Matthew Leaman,
Darren Stevens
In-Reply-To: <8a8afc73-3756-a305-ce5f-70b4bce6999f@xenosoft.de>
On 09 October 2021 at 03:57 pm, Christian Zigotzky wrote:
> On 09 October 2021 at 12:10 pm, Wolfram Sang wrote:
>>> I still don't have access to any old PASemi hardware but the
changes from
>>> v1 are pretty small and I expect them to still work. Would still be
nice
>>> if someone with access to such hardware could give this a quick test.
>> Looks good to me. I will wait a few more days so that people can report
>> their tests. But it will be in the next merge window.
>>
> Series v2:
>
> Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de> [1]
>
> - Christian
>
> [1]
https://forum.hyperion-entertainment.com/viewtopic.php?p=54213#p54213
Series v2:
Tested-by: Damien Stewart (Hypex) [1]
- Christian
[1] https://forum.hyperion-entertainment.com/viewtopic.php?p=54217#p54217
^ permalink raw reply
* Re: [RESEND PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
From: Miroslav Benes @ 2021-10-13 7:55 UTC (permalink / raw)
To: 王贇
Cc: Peter Zijlstra (Intel), Paul Walmsley, James E.J. Bottomley,
Guo Ren, Jisheng Zhang, H. Peter Anvin, live-patching,
linux-riscv, Paul Mackerras, Joe Lawrence, Helge Deller, x86,
linux-csky, Ingo Molnar, Petr Mladek, Albert Ou, Jiri Kosina,
Steven Rostedt, Borislav Petkov, Nicholas Piggin, Josh Poimboeuf,
Thomas Gleixner, linux-parisc, linux-kernel, Palmer Dabbelt,
Masami Hiramatsu, Colin Ian King, linuxppc-dev
In-Reply-To: <75ee86ac-02f2-d687-ab1e-9c8c33032495@linux.alibaba.com>
> diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h
> index a9f9c57..101e1fb 100644
> --- a/include/linux/trace_recursion.h
> +++ b/include/linux/trace_recursion.h
> @@ -208,13 +208,29 @@ static __always_inline void trace_clear_recursion(int bit)
> * Use this for ftrace callbacks. This will detect if the function
> * tracing recursed in the same context (normal vs interrupt),
> *
> + * The ftrace_test_recursion_trylock() will disable preemption,
> + * which is required for the variant of synchronize_rcu() that is
> + * used to allow patching functions where RCU is not watching.
> + * See klp_synchronize_transition() for more details.
> + *
I think that you misunderstood. Steven proposed to put the comment before
ftrace_test_recursion_trylock() call site in klp_ftrace_handler().
> * Returns: -1 if a recursion happened.
> * >= 0 if no recursion
> */
> static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
> unsigned long parent_ip)
> {
> - return trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> + int bit;
> +
> + bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> + /*
> + * The zero bit indicate we are nested
> + * in another trylock(), which means the
> + * preemption already disabled.
> + */
> + if (bit > 0)
> + preempt_disable_notrace();
> +
> + return bit;
> }
[...]
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index e8029ae..6e66ccd 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -52,11 +52,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
Here
> bit = ftrace_test_recursion_trylock(ip, parent_ip);
> if (WARN_ON_ONCE(bit < 0))
> return;
> - /*
> - * A variant of synchronize_rcu() is used to allow patching functions
> - * where RCU is not watching, see klp_synchronize_transition().
> - */
> - preempt_disable_notrace();
>
> func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
> stack_node);
> @@ -120,7 +115,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> klp_arch_set_pc(fregs, (unsigned long)func->new_func);
>
> unlock:
> - preempt_enable_notrace();
> ftrace_test_recursion_unlock(bit);
> }
Side note... the comment will eventually conflict with peterz's
https://lore.kernel.org/all/20210929152429.125997206@infradead.org/.
Miroslav
^ permalink raw reply
* Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()
From: Christophe Leroy @ 2021-10-13 7:48 UTC (permalink / raw)
To: Kees Cook
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <be307455-b02b-f382-851f-091ea26040b7@csgroup.eu>
Le 13/10/2021 à 09:39, Christophe Leroy a écrit :
>
>
> Le 13/10/2021 à 09:23, Kees Cook a écrit :
>> On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
>>> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
>>> not a copy of do_nothing().
>>>
>>> So do it directly instead of using execute_location().
>>>
>>> And fix displayed addresses by dereferencing the function descriptors.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>> drivers/misc/lkdtm/perms.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
>>> index 442d60ed25ef..da16564e1ecd 100644
>>> --- a/drivers/misc/lkdtm/perms.c
>>> +++ b/drivers/misc/lkdtm/perms.c
>>> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>>>
>>> void lkdtm_EXEC_RODATA(void)
>>> {
>>> - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
>>> + pr_info("attempting ok execution at %px\n",
>>> + dereference_symbol_descriptor(do_nothing));
>>> + do_nothing();
>>> +
>>> + pr_info("attempting bad execution at %px\n",
>>> + dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
>>> + lkdtm_rodata_do_nothing();
>>> + pr_err("FAIL: func returned\n");
>>> }
>>
>> (In re-reading this more carefully, I see now why kallsyms.h is used
>> earlier: _function_ vs _symbol_ descriptor.)
>>
>> In the next patch:
>>
>> static noinline void execute_location(void *dst, bool write)
>> {
>> ...
>> func = setup_function_descriptor(&fdesc, dst);
>> if (IS_ERR(func))
>> return;
>>
>> pr_info("attempting bad execution at %px\n", dst);
>> func();
>> pr_err("FAIL: func returned\n");
>> }
>>
>> What are the conditions for which dereference_symbol_descriptor works
>> but dereference _function_descriptor doesn't?
>>
>
> When LKDTM is built as a module I guess ?
>
To be more precise, dereference_symbol_descriptor() calls either
dereference_kernel_function_descriptor() or
dereference_module_function_descriptor()
Both functions call dereference_function_descriptor() after checking
that we want to dereference something that is in the OPD section.
If we call dereference_function_descriptor() directly instead of
dereference_symbol_descriptor() we skip the range verification and may
dereference something that is not a function descriptor.
Should we do that ?
Christophe
^ permalink raw reply
* Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()
From: Christophe Leroy @ 2021-10-13 7:39 UTC (permalink / raw)
To: Kees Cook
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <202110130018.7B2129375@keescook>
Le 13/10/2021 à 09:23, Kees Cook a écrit :
> On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
>> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
>> not a copy of do_nothing().
>>
>> So do it directly instead of using execute_location().
>>
>> And fix displayed addresses by dereferencing the function descriptors.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> drivers/misc/lkdtm/perms.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
>> index 442d60ed25ef..da16564e1ecd 100644
>> --- a/drivers/misc/lkdtm/perms.c
>> +++ b/drivers/misc/lkdtm/perms.c
>> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>>
>> void lkdtm_EXEC_RODATA(void)
>> {
>> - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
>> + pr_info("attempting ok execution at %px\n",
>> + dereference_symbol_descriptor(do_nothing));
>> + do_nothing();
>> +
>> + pr_info("attempting bad execution at %px\n",
>> + dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
>> + lkdtm_rodata_do_nothing();
>> + pr_err("FAIL: func returned\n");
>> }
>
> (In re-reading this more carefully, I see now why kallsyms.h is used
> earlier: _function_ vs _symbol_ descriptor.)
>
> In the next patch:
>
> static noinline void execute_location(void *dst, bool write)
> {
> ...
> func = setup_function_descriptor(&fdesc, dst);
> if (IS_ERR(func))
> return;
>
> pr_info("attempting bad execution at %px\n", dst);
> func();
> pr_err("FAIL: func returned\n");
> }
>
> What are the conditions for which dereference_symbol_descriptor works
> but dereference _function_descriptor doesn't?
>
When LKDTM is built as a module I guess ?
Christophe
^ permalink raw reply
* Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()
From: Christophe Leroy @ 2021-10-13 7:35 UTC (permalink / raw)
To: Kees Cook
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <202110130008.EC5E957D4A@keescook>
Le 13/10/2021 à 09:09, Kees Cook a écrit :
> On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
>> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
>> not a copy of do_nothing().
>>
>> So do it directly instead of using execute_location().
>
> I don't understand this. Why does the next patch not fix this?
Well, probably it would, but it looked incorrect in my mind.
lkdtm_rodata_do_nothing() is a function which has its own function
descriptor, it is not a copy of do_nothing().
If we use execute_location() as modified by next patch, then we will
execute it using the function descriptor of do_nothing(). Allthough it
most likely works (at least on powerpc as it uses the same TOC) it looks
odd to me to do so.
Am I missing something ?
Christophe
>
> -Kees
>
>>
>> And fix displayed addresses by dereferencing the function descriptors.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> drivers/misc/lkdtm/perms.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
>> index 442d60ed25ef..da16564e1ecd 100644
>> --- a/drivers/misc/lkdtm/perms.c
>> +++ b/drivers/misc/lkdtm/perms.c
>> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>>
>> void lkdtm_EXEC_RODATA(void)
>> {
>> - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
>> + pr_info("attempting ok execution at %px\n",
>> + dereference_symbol_descriptor(do_nothing));
>> + do_nothing();
>> +
>> + pr_info("attempting bad execution at %px\n",
>> + dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
>> + lkdtm_rodata_do_nothing();
>> + pr_err("FAIL: func returned\n");
>> }
>>
>> void lkdtm_EXEC_USERSPACE(void)
>> --
>> 2.31.1
>>
>
^ permalink raw reply
* Re: [PATCH v1 08/10] lkdtm: Really write into kernel text in WRITE_KERN
From: Christophe Leroy @ 2021-10-13 7:29 UTC (permalink / raw)
To: Kees Cook
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <202110130004.880A6C841@keescook>
Le 13/10/2021 à 09:05, Kees Cook a écrit :
> On Mon, Oct 11, 2021 at 05:25:35PM +0200, Christophe Leroy wrote:
>> WRITE_KERN is supposed to overwrite some kernel text, namely
>> do_overwritten() function.
>>
>> But at the time being it overwrites do_overwritten() function
>> descriptor, not function text.
>>
>> Fix it by dereferencing the function descriptor to obtain
>> function text pointer.
>>
>> And make do_overwritten() noinline so that it is really
>> do_overwritten() which is called by lkdtm_WRITE_KERN().
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> drivers/misc/lkdtm/perms.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
>> index 60b3b2fe929d..442d60ed25ef 100644
>> --- a/drivers/misc/lkdtm/perms.c
>> +++ b/drivers/misc/lkdtm/perms.c
>> @@ -5,6 +5,7 @@
>> * even non-readable regions.
>> */
>> #include "lkdtm.h"
>> +#include <linux/kallsyms.h>
>
> Why not #include <asm/sections.h> instead here?
dereference_symbol_descriptor() is defined in linux/kallsyms.h
>
>> #include <linux/slab.h>
>> #include <linux/vmalloc.h>
>> #include <linux/mman.h>
>> @@ -37,7 +38,7 @@ static noinline void do_nothing(void)
>> }
>>
>> /* Must immediately follow do_nothing for size calculuations to work out. */
>> -static void do_overwritten(void)
>> +static noinline void do_overwritten(void)
>> {
>> pr_info("do_overwritten wasn't overwritten!\n");
>> return;
>> @@ -113,8 +114,9 @@ void lkdtm_WRITE_KERN(void)
>> size_t size;
>> volatile unsigned char *ptr;
>>
>> - size = (unsigned long)do_overwritten - (unsigned long)do_nothing;
>> - ptr = (unsigned char *)do_overwritten;
>> + size = (unsigned long)dereference_symbol_descriptor(do_overwritten) -
>> + (unsigned long)dereference_symbol_descriptor(do_nothing);
>> + ptr = dereference_symbol_descriptor(do_overwritten);
>
> But otherwise, yup, I expect there will be a bunch of things like this
> to clean up in LKDTM. :| Sorry about that!
>
> Acked-by: Kees Cook <keescook@chromium.org>
>
>>
>> pr_info("attempting bad %zu byte write at %px\n", size, ptr);
>> memcpy((void *)ptr, (unsigned char *)do_nothing, size);
>> --
>> 2.31.1
>>
>
^ permalink raw reply
* Re: [PATCH v1 05/10] asm-generic: Define 'funct_descr_t' to commonly describe function descriptors
From: Kees Cook @ 2021-10-13 7:27 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <cf0e465e-e678-692c-3ca5-fde70ba4fc97@csgroup.eu>
On Wed, Oct 13, 2021 at 09:23:56AM +0200, Christophe Leroy wrote:
>
>
> Le 13/10/2021 à 09:01, Kees Cook a écrit :
> > On Mon, Oct 11, 2021 at 05:25:32PM +0200, Christophe Leroy wrote:
> > > We have three architectures using function descriptors, each with its
> > > own name.
> > >
> > > Add a common typedef that can be used in generic code.
> > >
> > > Also add a stub typedef for architecture without function descriptors,
> >
> > nit: funct_descr_t reads weird to me. why not func_desc_t ? Either way:
>
> func_desc_t already exists in powerpc. I have a patch to remove it as it is
> redundant with struct ppc64_opd_entry, but I didnt' want to include it in
> this series.
>
> But after all I can add it in this series, I'll add it in v2.
Ah-ha! That works for me. :) Thanks!
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v1 05/10] asm-generic: Define 'funct_descr_t' to commonly describe function descriptors
From: Christophe Leroy @ 2021-10-13 7:23 UTC (permalink / raw)
To: Kees Cook
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <202110130001.11A50456@keescook>
Le 13/10/2021 à 09:01, Kees Cook a écrit :
> On Mon, Oct 11, 2021 at 05:25:32PM +0200, Christophe Leroy wrote:
>> We have three architectures using function descriptors, each with its
>> own name.
>>
>> Add a common typedef that can be used in generic code.
>>
>> Also add a stub typedef for architecture without function descriptors,
>
> nit: funct_descr_t reads weird to me. why not func_desc_t ? Either way:
func_desc_t already exists in powerpc. I have a patch to remove it as it
is redundant with struct ppc64_opd_entry, but I didnt' want to include
it in this series.
But after all I can add it in this series, I'll add it in v2.
Christophe
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
>
>> to avoid a forest of #ifdefs.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/ia64/include/asm/sections.h | 1 +
>> arch/parisc/include/asm/sections.h | 1 +
>> arch/powerpc/include/asm/sections.h | 1 +
>> include/asm-generic/sections.h | 3 +++
>> 4 files changed, 6 insertions(+)
>>
>> diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
>> index 80f5868afb06..929b5c535620 100644
>> --- a/arch/ia64/include/asm/sections.h
>> +++ b/arch/ia64/include/asm/sections.h
>> @@ -8,6 +8,7 @@
>> */
>>
>> #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
>> +typedef struct fdesc funct_descr_t;
>>
>> #include <linux/elf.h>
>> #include <linux/uaccess.h>
>> diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
>> index 2e781ee19b66..329e80f7af0a 100644
>> --- a/arch/parisc/include/asm/sections.h
>> +++ b/arch/parisc/include/asm/sections.h
>> @@ -4,6 +4,7 @@
>>
>> #ifdef CONFIG_64BIT
>> #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
>> +typedef Elf64_Fdesc funct_descr_t;
>> #endif
>>
>> /* nothing to see, move along */
>> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
>> index b7f1ba04e756..d0d5287fa568 100644
>> --- a/arch/powerpc/include/asm/sections.h
>> +++ b/arch/powerpc/include/asm/sections.h
>> @@ -10,6 +10,7 @@
>>
>> #ifdef PPC64_ELF_ABI_v1
>> #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
>> +typedef struct ppc64_opd_entry funct_descr_t;
>> #endif
>>
>> #include <asm-generic/sections.h>
>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
>> index 1db5cfd69817..436412d94054 100644
>> --- a/include/asm-generic/sections.h
>> +++ b/include/asm-generic/sections.h
>> @@ -63,6 +63,9 @@ extern __visible const void __nosave_begin, __nosave_end;
>> #else
>> #define dereference_function_descriptor(p) ((void *)(p))
>> #define dereference_kernel_function_descriptor(p) ((void *)(p))
>> +typedef struct {
>> + unsigned long addr;
>> +} funct_descr_t;
>> #endif
>>
>> /* random extra sections (if any). Override
>> --
>> 2.31.1
>>
>
^ permalink raw reply
* Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()
From: Kees Cook @ 2021-10-13 7:23 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <7da92e59e148bd23564d63bdd8bcfaba0ba6d1f1.1633964380.git.christophe.leroy@csgroup.eu>
On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
> not a copy of do_nothing().
>
> So do it directly instead of using execute_location().
>
> And fix displayed addresses by dereferencing the function descriptors.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> drivers/misc/lkdtm/perms.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 442d60ed25ef..da16564e1ecd 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>
> void lkdtm_EXEC_RODATA(void)
> {
> - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
> + pr_info("attempting ok execution at %px\n",
> + dereference_symbol_descriptor(do_nothing));
> + do_nothing();
> +
> + pr_info("attempting bad execution at %px\n",
> + dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
> + lkdtm_rodata_do_nothing();
> + pr_err("FAIL: func returned\n");
> }
(In re-reading this more carefully, I see now why kallsyms.h is used
earlier: _function_ vs _symbol_ descriptor.)
In the next patch:
static noinline void execute_location(void *dst, bool write)
{
...
func = setup_function_descriptor(&fdesc, dst);
if (IS_ERR(func))
return;
pr_info("attempting bad execution at %px\n", dst);
func();
pr_err("FAIL: func returned\n");
}
What are the conditions for which dereference_symbol_descriptor works
but dereference _function_descriptor doesn't?
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v1 10/10] lkdtm: Fix execute_[user]_location()
From: Kees Cook @ 2021-10-13 7:16 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <c551f3f4b803d1a4a184b0fa94475d405ba436d8.1633964380.git.christophe.leroy@csgroup.eu>
On Mon, Oct 11, 2021 at 05:25:37PM +0200, Christophe Leroy wrote:
> execute_location() and execute_user_location() intent
> to copy do_nothing() text and execute it at a new location.
> However, at the time being it doesn't copy do_nothing() function
> but do_nothing() function descriptor which still points to the
> original text. So at the end it still executes do_nothing() at
> its original location allthough using a copied function descriptor.
>
> So, fix that by really copying do_nothing() text and build a new
> function descriptor by copying do_nothing() function descriptor and
> updating the target address with the new location.
>
> Also fix the displayed addresses by dereferencing do_nothing()
> function descriptor.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> drivers/misc/lkdtm/perms.c | 45 +++++++++++++++++++++++++++++++-------
> 1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index da16564e1ecd..1d03cd44c21d 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -44,19 +44,42 @@ static noinline void do_overwritten(void)
> return;
> }
>
> +static void *setup_function_descriptor(funct_descr_t *fdesc, void *dst)
> +{
> + int err;
> +
> + if (!__is_defined(HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR))
> + return dst;
> +
> + err = copy_from_kernel_nofault(fdesc, do_nothing, sizeof(*fdesc));
> + if (err < 0)
> + return ERR_PTR(err);
> +
> + fdesc->addr = (unsigned long)dst;
> + barrier();
> +
> + return fdesc;
> +}
> +
> static noinline void execute_location(void *dst, bool write)
> {
> - void (*func)(void) = dst;
> + void (*func)(void);
> + funct_descr_t fdesc;
> + void *do_nothing_text = dereference_symbol_descriptor(do_nothing);
>
> - pr_info("attempting ok execution at %px\n", do_nothing);
> + pr_info("attempting ok execution at %px\n", do_nothing_text);
> do_nothing();
>
> if (write == CODE_WRITE) {
> - memcpy(dst, do_nothing, EXEC_SIZE);
> + memcpy(dst, do_nothing_text, EXEC_SIZE);
> flush_icache_range((unsigned long)dst,
> (unsigned long)dst + EXEC_SIZE);
> }
> - pr_info("attempting bad execution at %px\n", func);
> + func = setup_function_descriptor(&fdesc, dst);
> + if (IS_ERR(func))
> + return;
I think this error case should complain with some details. :) Maybe:
pr_err("FAIL: could not build function descriptor for %px\n", dst);
> +
> + pr_info("attempting bad execution at %px\n", dst);
And then leave this pr_info() as it was, before the
setup_function_descriptor() call.
> func();
> pr_err("FAIL: func returned\n");
> }
> @@ -66,16 +89,22 @@ static void execute_user_location(void *dst)
> int copied;
>
> /* Intentionally crossing kernel/user memory boundary. */
> - void (*func)(void) = dst;
> + void (*func)(void);
> + funct_descr_t fdesc;
> + void *do_nothing_text = dereference_symbol_descriptor(do_nothing);
>
> - pr_info("attempting ok execution at %px\n", do_nothing);
> + pr_info("attempting ok execution at %px\n", do_nothing_text);
> do_nothing();
>
> - copied = access_process_vm(current, (unsigned long)dst, do_nothing,
> + copied = access_process_vm(current, (unsigned long)dst, do_nothing_text,
> EXEC_SIZE, FOLL_WRITE);
> if (copied < EXEC_SIZE)
> return;
> - pr_info("attempting bad execution at %px\n", func);
> + func = setup_function_descriptor(&fdesc, dst);
> + if (IS_ERR(func))
> + return;
> +
> + pr_info("attempting bad execution at %px\n", dst);
Same here.
> func();
> pr_err("FAIL: func returned\n");
> }
> --
> 2.31.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()
From: Kees Cook @ 2021-10-13 7:09 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <7da92e59e148bd23564d63bdd8bcfaba0ba6d1f1.1633964380.git.christophe.leroy@csgroup.eu>
On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
> not a copy of do_nothing().
>
> So do it directly instead of using execute_location().
I don't understand this. Why does the next patch not fix this?
-Kees
>
> And fix displayed addresses by dereferencing the function descriptors.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> drivers/misc/lkdtm/perms.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 442d60ed25ef..da16564e1ecd 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>
> void lkdtm_EXEC_RODATA(void)
> {
> - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
> + pr_info("attempting ok execution at %px\n",
> + dereference_symbol_descriptor(do_nothing));
> + do_nothing();
> +
> + pr_info("attempting bad execution at %px\n",
> + dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
> + lkdtm_rodata_do_nothing();
> + pr_err("FAIL: func returned\n");
> }
>
> void lkdtm_EXEC_USERSPACE(void)
> --
> 2.31.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v1 08/10] lkdtm: Really write into kernel text in WRITE_KERN
From: Kees Cook @ 2021-10-13 7:05 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <624940395e5d81967246f911a65740b9a15b5a70.1633964380.git.christophe.leroy@csgroup.eu>
On Mon, Oct 11, 2021 at 05:25:35PM +0200, Christophe Leroy wrote:
> WRITE_KERN is supposed to overwrite some kernel text, namely
> do_overwritten() function.
>
> But at the time being it overwrites do_overwritten() function
> descriptor, not function text.
>
> Fix it by dereferencing the function descriptor to obtain
> function text pointer.
>
> And make do_overwritten() noinline so that it is really
> do_overwritten() which is called by lkdtm_WRITE_KERN().
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> drivers/misc/lkdtm/perms.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 60b3b2fe929d..442d60ed25ef 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -5,6 +5,7 @@
> * even non-readable regions.
> */
> #include "lkdtm.h"
> +#include <linux/kallsyms.h>
Why not #include <asm/sections.h> instead here?
> #include <linux/slab.h>
> #include <linux/vmalloc.h>
> #include <linux/mman.h>
> @@ -37,7 +38,7 @@ static noinline void do_nothing(void)
> }
>
> /* Must immediately follow do_nothing for size calculuations to work out. */
> -static void do_overwritten(void)
> +static noinline void do_overwritten(void)
> {
> pr_info("do_overwritten wasn't overwritten!\n");
> return;
> @@ -113,8 +114,9 @@ void lkdtm_WRITE_KERN(void)
> size_t size;
> volatile unsigned char *ptr;
>
> - size = (unsigned long)do_overwritten - (unsigned long)do_nothing;
> - ptr = (unsigned char *)do_overwritten;
> + size = (unsigned long)dereference_symbol_descriptor(do_overwritten) -
> + (unsigned long)dereference_symbol_descriptor(do_nothing);
> + ptr = dereference_symbol_descriptor(do_overwritten);
But otherwise, yup, I expect there will be a bunch of things like this
to clean up in LKDTM. :| Sorry about that!
Acked-by: Kees Cook <keescook@chromium.org>
>
> pr_info("attempting bad %zu byte write at %px\n", size, ptr);
> memcpy((void *)ptr, (unsigned char *)do_nothing, size);
> --
> 2.31.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v1 07/10] lkdtm: Force do_nothing() out of line
From: Kees Cook @ 2021-10-13 7:02 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <b353a85e50ac336c385b46459a5fc43f4a6171ac.1633964380.git.christophe.leroy@csgroup.eu>
On Mon, Oct 11, 2021 at 05:25:34PM +0200, Christophe Leroy wrote:
> LKDTM tests display that the run do_nothing() at a given
> address, but in reality do_nothing() is inlined into the
> caller.
>
> Force it out of line so that it really runs text at the
> displayed address.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Acked-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v1 06/10] asm-generic: Refactor dereference_[kernel]_function_descriptor()
From: Kees Cook @ 2021-10-13 7:02 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <c215b244a19a07327ec81bf99f3c5f89c68af7b4.1633964380.git.christophe.leroy@csgroup.eu>
On Mon, Oct 11, 2021 at 05:25:33PM +0200, Christophe Leroy wrote:
> dereference_function_descriptor() and
> dereference_kernel_function_descriptor() are identical on the
> three architectures implementing them.
>
> Make it common.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/ia64/include/asm/sections.h | 19 -------------------
> arch/parisc/include/asm/sections.h | 9 ---------
> arch/parisc/kernel/process.c | 21 ---------------------
> arch/powerpc/include/asm/sections.h | 23 -----------------------
> include/asm-generic/sections.h | 18 ++++++++++++++++++
> 5 files changed, 18 insertions(+), 72 deletions(-)
A diffstat to love. :)
Reviewed-by: Kees Cook <keescook@chromium.org>
>
> diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
> index 929b5c535620..d9addaea8339 100644
> --- a/arch/ia64/include/asm/sections.h
> +++ b/arch/ia64/include/asm/sections.h
> @@ -30,23 +30,4 @@ extern char __start_gate_brl_fsys_bubble_down_patchlist[], __end_gate_brl_fsys_b
> extern char __start_unwind[], __end_unwind[];
> extern char __start_ivt_text[], __end_ivt_text[];
>
> -#undef dereference_function_descriptor
> -static inline void *dereference_function_descriptor(void *ptr)
> -{
> - struct fdesc *desc = ptr;
> - void *p;
> -
> - if (!get_kernel_nofault(p, (void *)&desc->addr))
> - ptr = p;
> - return ptr;
> -}
> -
> -#undef dereference_kernel_function_descriptor
> -static inline void *dereference_kernel_function_descriptor(void *ptr)
> -{
> - if (ptr < (void *)__start_opd || ptr >= (void *)__end_opd)
> - return ptr;
> - return dereference_function_descriptor(ptr);
> -}
> -
> #endif /* _ASM_IA64_SECTIONS_H */
> diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
> index 329e80f7af0a..b041fb32a300 100644
> --- a/arch/parisc/include/asm/sections.h
> +++ b/arch/parisc/include/asm/sections.h
> @@ -12,13 +12,4 @@ typedef Elf64_Fdesc funct_descr_t;
>
> extern char __alt_instructions[], __alt_instructions_end[];
>
> -#ifdef CONFIG_64BIT
> -
> -#undef dereference_function_descriptor
> -void *dereference_function_descriptor(void *);
> -
> -#undef dereference_kernel_function_descriptor
> -void *dereference_kernel_function_descriptor(void *);
> -#endif
> -
> #endif
> diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c
> index 38ec4ae81239..7382576b52a8 100644
> --- a/arch/parisc/kernel/process.c
> +++ b/arch/parisc/kernel/process.c
> @@ -266,27 +266,6 @@ get_wchan(struct task_struct *p)
> return 0;
> }
>
> -#ifdef CONFIG_64BIT
> -void *dereference_function_descriptor(void *ptr)
> -{
> - Elf64_Fdesc *desc = ptr;
> - void *p;
> -
> - if (!get_kernel_nofault(p, (void *)&desc->addr))
> - ptr = p;
> - return ptr;
> -}
> -
> -void *dereference_kernel_function_descriptor(void *ptr)
> -{
> - if (ptr < (void *)__start_opd ||
> - ptr >= (void *)__end_opd)
> - return ptr;
> -
> - return dereference_function_descriptor(ptr);
> -}
> -#endif
> -
> static inline unsigned long brk_rnd(void)
> {
> return (get_random_int() & BRK_RND_MASK) << PAGE_SHIFT;
> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
> index d0d5287fa568..8f8e95f234e2 100644
> --- a/arch/powerpc/include/asm/sections.h
> +++ b/arch/powerpc/include/asm/sections.h
> @@ -72,29 +72,6 @@ static inline int overlaps_kernel_text(unsigned long start, unsigned long end)
> (unsigned long)_stext < end;
> }
>
> -#ifdef PPC64_ELF_ABI_v1
> -
> -#undef dereference_function_descriptor
> -static inline void *dereference_function_descriptor(void *ptr)
> -{
> - struct ppc64_opd_entry *desc = ptr;
> - void *p;
> -
> - if (!get_kernel_nofault(p, (void *)&desc->addr))
> - ptr = p;
> - return ptr;
> -}
> -
> -#undef dereference_kernel_function_descriptor
> -static inline void *dereference_kernel_function_descriptor(void *ptr)
> -{
> - if (ptr < (void *)__start_opd || ptr >= (void *)__end_opd)
> - return ptr;
> -
> - return dereference_function_descriptor(ptr);
> -}
> -#endif /* PPC64_ELF_ABI_v1 */
> -
> #endif
>
> #endif /* __KERNEL__ */
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 436412d94054..5baaf9d7c671 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -60,6 +60,24 @@ extern __visible const void __nosave_begin, __nosave_end;
>
> /* Function descriptor handling (if any). Override in asm/sections.h */
> #ifdef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
> +static inline void *dereference_function_descriptor(void *ptr)
> +{
> + funct_descr_t *desc = ptr;
> + void *p;
> +
> + if (!get_kernel_nofault(p, (void *)&desc->addr))
> + ptr = p;
> + return ptr;
> +}
> +
> +static inline void *dereference_kernel_function_descriptor(void *ptr)
> +{
> + if (ptr < (void *)__start_opd || ptr >= (void *)__end_opd)
> + return ptr;
> +
> + return dereference_function_descriptor(ptr);
> +}
> +
> #else
> #define dereference_function_descriptor(p) ((void *)(p))
> #define dereference_kernel_function_descriptor(p) ((void *)(p))
> --
> 2.31.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v1 05/10] asm-generic: Define 'funct_descr_t' to commonly describe function descriptors
From: Kees Cook @ 2021-10-13 7:01 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <02224551451ab9c37055499fc621c41246c81125.1633964380.git.christophe.leroy@csgroup.eu>
On Mon, Oct 11, 2021 at 05:25:32PM +0200, Christophe Leroy wrote:
> We have three architectures using function descriptors, each with its
> own name.
>
> Add a common typedef that can be used in generic code.
>
> Also add a stub typedef for architecture without function descriptors,
nit: funct_descr_t reads weird to me. why not func_desc_t ? Either way:
Reviewed-by: Kees Cook <keescook@chromium.org>
> to avoid a forest of #ifdefs.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/ia64/include/asm/sections.h | 1 +
> arch/parisc/include/asm/sections.h | 1 +
> arch/powerpc/include/asm/sections.h | 1 +
> include/asm-generic/sections.h | 3 +++
> 4 files changed, 6 insertions(+)
>
> diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
> index 80f5868afb06..929b5c535620 100644
> --- a/arch/ia64/include/asm/sections.h
> +++ b/arch/ia64/include/asm/sections.h
> @@ -8,6 +8,7 @@
> */
>
> #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +typedef struct fdesc funct_descr_t;
>
> #include <linux/elf.h>
> #include <linux/uaccess.h>
> diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
> index 2e781ee19b66..329e80f7af0a 100644
> --- a/arch/parisc/include/asm/sections.h
> +++ b/arch/parisc/include/asm/sections.h
> @@ -4,6 +4,7 @@
>
> #ifdef CONFIG_64BIT
> #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +typedef Elf64_Fdesc funct_descr_t;
> #endif
>
> /* nothing to see, move along */
> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
> index b7f1ba04e756..d0d5287fa568 100644
> --- a/arch/powerpc/include/asm/sections.h
> +++ b/arch/powerpc/include/asm/sections.h
> @@ -10,6 +10,7 @@
>
> #ifdef PPC64_ELF_ABI_v1
> #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +typedef struct ppc64_opd_entry funct_descr_t;
> #endif
>
> #include <asm-generic/sections.h>
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 1db5cfd69817..436412d94054 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -63,6 +63,9 @@ extern __visible const void __nosave_begin, __nosave_end;
> #else
> #define dereference_function_descriptor(p) ((void *)(p))
> #define dereference_kernel_function_descriptor(p) ((void *)(p))
> +typedef struct {
> + unsigned long addr;
> +} funct_descr_t;
> #endif
>
> /* random extra sections (if any). Override
> --
> 2.31.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v1 04/10] asm-generic: Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to define associated stubs
From: Kees Cook @ 2021-10-13 7:00 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <8db2a3ca2b26a8325c671baa3e0492914597f079.1633964380.git.christophe.leroy@csgroup.eu>
On Mon, Oct 11, 2021 at 05:25:31PM +0200, Christophe Leroy wrote:
> Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR instead of 'dereference_function_descriptor'
> to know whether arch has function descriptors.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
I'd mention the intentionally-empty #if/#else in the commit log, as I,
like Helge, mentally tripped over it in the review. :)
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v1 03/10] ia64: Rename 'ip' to 'addr' in 'struct fdesc'
From: Kees Cook @ 2021-10-13 6:59 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <a2443adcd006cb8004fe0602e2f8c43c30a7c504.1633964380.git.christophe.leroy@csgroup.eu>
On Mon, Oct 11, 2021 at 05:25:30PM +0200, Christophe Leroy wrote:
> There are three architectures with function descriptors, try to
> have common names for the address they contain in order to
> refactor some functions into generic functions later.
>
> powerpc has 'funcaddr'
> ia64 has 'ip'
> parisc has 'addr'
>
> Vote for 'addr' and update 'struct fdesc' accordingly.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v1 02/10] powerpc: Rename 'funcaddr' to 'addr' in 'struct ppc64_opd_entry'
From: Kees Cook @ 2021-10-13 6:59 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <892715d6f05fdf6ca80cf88a51a9e55298b68c4a.1633964380.git.christophe.leroy@csgroup.eu>
On Mon, Oct 11, 2021 at 05:25:29PM +0200, Christophe Leroy wrote:
> There are three architectures with function descriptors, try to
> have common names for the address they contain in order to
> refactor some functions into generic functions later.
>
> powerpc has 'funcaddr'
> ia64 has 'ip'
> parisc has 'addr'
>
> Vote for 'addr' and update 'struct ppc64_opd_entry' accordingly.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reasonable. :)
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v1 01/10] powerpc: Move 'struct ppc64_opd_entry' back into asm/elf.h
From: Kees Cook @ 2021-10-13 6:59 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <8ff3ec195d695033b652e9971fba2dc5528f7151.1633964380.git.christophe.leroy@csgroup.eu>
On Mon, Oct 11, 2021 at 05:25:28PM +0200, Christophe Leroy wrote:
> 'struct ppc64_opd_entry' doesn't belong to uapi/asm/elf.h
>
> It was initially in module_64.c and commit 2d291e902791 ("Fix compile
> failure with non modular builds") moved it into asm/elf.h
>
> But it was by mistake added outside of __KERNEL__ section,
> therefore commit c3617f72036c ("UAPI: (Scripted) Disintegrate
> arch/powerpc/include/asm") moved it to uapi/asm/elf.h
>
> Move it back into asm/elf.h, this brings it back in line with
> IA64 and PARISC architectures.
>
> Fixes: 2d291e902791 ("Fix compile failure with non modular builds")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
I'd agree with Arnd: this is a reasonable cleanup and nothing should be
using it.
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [RFC PATCH] powerpc: dts: Remove MPC5xxx platforms
From: Stephen Rothwell @ 2021-10-13 6:38 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, Arnd Bergmann, linux-kernel, Paul Mackerras,
Anatolij Gustschin, linuxppc-dev
In-Reply-To: <20211012153456.2844193-1-robh@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]
Hi Rob,
On Tue, 12 Oct 2021 10:34:56 -0500 Rob Herring <robh@kernel.org> wrote:
>
> The mpc5xxx platforms have had dts warnings for some time which no one
> seems to care to fix, so let's just remove the dts files.
>
> According to Arnd:
> "Specifically, MPC5200B has a 15 year lifetime, which ends in
> 11 months from now. The original bplan/Genesi Efika 5K2 was
> quite popular at the time it came out, and there are probably
> still some of those hanging around, but they came with Open
> Firmware rather than relying on the dts files that ship with the
> kernel.
>
> Grant Likely was the original maintainer for MPC52xx until 2011,
> Anatolij Gustschin is still listed as maintainer since then but hasn't
> been active in it for a while either. Anatolij can probably best judge
> which of these boards are still in going to be used with future kernels,
> but I suspect once you start removing bits from 52xx, the newer
> but less common 512x platform can go away as well."
>
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Sending this out as a feeler to see if anyone cares. If anyone does,
> please fix the warnings.
Thanks. However .. :-)
FATAL ERROR: Couldn't open "mpc5200b.dtsi": No such file or directory
make[2]: *** [/home/sfr/next/next/scripts/Makefile.lib:358: arch/powerpc/boot/dts/digsy_mtc.dtb] Error 1
$ grep -wrl mpc5200b.dtsi
arch/powerpc/boot/dts/digsy_mtc.dts
missed one :-)
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [RESEND PATCH v2 2/2] ftrace: do CPU checking after preemption disabled
From: 王贇 @ 2021-10-13 3:38 UTC (permalink / raw)
To: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, Colin Ian King, Masami Hiramatsu,
Peter Zijlstra (Intel), Nicholas Piggin, Jisheng Zhang,
linux-csky, linux-kernel, linux-parisc, linuxppc-dev, linux-riscv,
live-patching
In-Reply-To: <b1d7fe43-ce84-0ed7-32f7-ea1d12d0b716@linux.alibaba.com>
With CONFIG_DEBUG_PREEMPT we observed reports like:
BUG: using smp_processor_id() in preemptible
caller is perf_ftrace_function_call+0x6f/0x2e0
CPU: 1 PID: 680 Comm: a.out Not tainted
Call Trace:
<TASK>
dump_stack_lvl+0x8d/0xcf
check_preemption_disabled+0x104/0x110
? optimize_nops.isra.7+0x230/0x230
? text_poke_bp_batch+0x9f/0x310
perf_ftrace_function_call+0x6f/0x2e0
...
__text_poke+0x5/0x620
text_poke_bp_batch+0x9f/0x310
This telling us the CPU could be changed after task is preempted, and
the checking on CPU before preemption will be invalid.
Since now ftrace_test_recursion_trylock() will help to disable the
preemption, this patch just do the checking after trylock() to address
the issue.
CC: Steven Rostedt <rostedt@goodmis.org>
Reported-by: Abaci <abaci@linux.alibaba.com>
Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
---
kernel/trace/trace_event_perf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 6aed10e..fba8cb7 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -441,13 +441,13 @@ void perf_trace_buf_update(void *record, u16 type)
if (!rcu_is_watching())
return;
- if ((unsigned long)ops->private != smp_processor_id())
- return;
-
bit = ftrace_test_recursion_trylock(ip, parent_ip);
if (bit < 0)
return;
+ if ((unsigned long)ops->private != smp_processor_id())
+ goto out;
+
event = container_of(ops, struct perf_event, ftrace_ops);
/*
--
1.8.3.1
^ permalink raw reply related
* [RESEND PATCH v2 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
From: 王贇 @ 2021-10-13 3:37 UTC (permalink / raw)
To: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, Colin Ian King, Masami Hiramatsu,
Peter Zijlstra (Intel), Nicholas Piggin, Jisheng Zhang,
linux-csky, linux-kernel, linux-parisc, linuxppc-dev, linux-riscv,
live-patching
In-Reply-To: <b1d7fe43-ce84-0ed7-32f7-ea1d12d0b716@linux.alibaba.com>
As the documentation explained, ftrace_test_recursion_trylock()
and ftrace_test_recursion_unlock() were supposed to disable and
enable preemption properly, however currently this work is done
outside of the function, which could be missing by mistake.
This path will make sure the preemption was disabled when trylock()
succeed, and the unlock() will enable the preemption if previously
enabled.
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Miroslav Benes <mbenes@suse.cz>
Reported-by: Abaci <abaci@linux.alibaba.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
---
arch/csky/kernel/probes/ftrace.c | 2 --
arch/parisc/kernel/ftrace.c | 2 --
arch/powerpc/kernel/kprobes-ftrace.c | 2 --
arch/riscv/kernel/probes/ftrace.c | 2 --
arch/x86/kernel/kprobes/ftrace.c | 2 --
include/linux/trace_recursion.h | 22 +++++++++++++++++++++-
kernel/livepatch/patch.c | 6 ------
kernel/trace/trace_functions.c | 5 -----
8 files changed, 21 insertions(+), 22 deletions(-)
diff --git a/arch/csky/kernel/probes/ftrace.c b/arch/csky/kernel/probes/ftrace.c
index b388228..834cffc 100644
--- a/arch/csky/kernel/probes/ftrace.c
+++ b/arch/csky/kernel/probes/ftrace.c
@@ -17,7 +17,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
return;
regs = ftrace_get_regs(fregs);
- preempt_disable_notrace();
p = get_kprobe((kprobe_opcode_t *)ip);
if (!p) {
p = get_kprobe((kprobe_opcode_t *)(ip - MCOUNT_INSN_SIZE));
@@ -57,7 +56,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
__this_cpu_write(current_kprobe, NULL);
}
out:
- preempt_enable_notrace();
ftrace_test_recursion_unlock(bit);
}
NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c
index 0a1e75a..3543496 100644
--- a/arch/parisc/kernel/ftrace.c
+++ b/arch/parisc/kernel/ftrace.c
@@ -216,7 +216,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
return;
regs = ftrace_get_regs(fregs);
- preempt_disable_notrace();
p = get_kprobe((kprobe_opcode_t *)ip);
if (unlikely(!p) || kprobe_disabled(p))
goto out;
@@ -245,7 +244,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
}
__this_cpu_write(current_kprobe, NULL);
out:
- preempt_enable_notrace();
ftrace_test_recursion_unlock(bit);
}
NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/powerpc/kernel/kprobes-ftrace.c b/arch/powerpc/kernel/kprobes-ftrace.c
index 7154d58..072ebe7 100644
--- a/arch/powerpc/kernel/kprobes-ftrace.c
+++ b/arch/powerpc/kernel/kprobes-ftrace.c
@@ -26,7 +26,6 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
return;
regs = ftrace_get_regs(fregs);
- preempt_disable_notrace();
p = get_kprobe((kprobe_opcode_t *)nip);
if (unlikely(!p) || kprobe_disabled(p))
goto out;
@@ -61,7 +60,6 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
__this_cpu_write(current_kprobe, NULL);
}
out:
- preempt_enable_notrace();
ftrace_test_recursion_unlock(bit);
}
NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/riscv/kernel/probes/ftrace.c b/arch/riscv/kernel/probes/ftrace.c
index aab85a8..7142ec4 100644
--- a/arch/riscv/kernel/probes/ftrace.c
+++ b/arch/riscv/kernel/probes/ftrace.c
@@ -15,7 +15,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
if (bit < 0)
return;
- preempt_disable_notrace();
p = get_kprobe((kprobe_opcode_t *)ip);
if (unlikely(!p) || kprobe_disabled(p))
goto out;
@@ -52,7 +51,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
__this_cpu_write(current_kprobe, NULL);
}
out:
- preempt_enable_notrace();
ftrace_test_recursion_unlock(bit);
}
NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 596de2f..dd2ec14 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -25,7 +25,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
if (bit < 0)
return;
- preempt_disable_notrace();
p = get_kprobe((kprobe_opcode_t *)ip);
if (unlikely(!p) || kprobe_disabled(p))
goto out;
@@ -59,7 +58,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
__this_cpu_write(current_kprobe, NULL);
}
out:
- preempt_enable_notrace();
ftrace_test_recursion_unlock(bit);
}
NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h
index a9f9c57..101e1fb 100644
--- a/include/linux/trace_recursion.h
+++ b/include/linux/trace_recursion.h
@@ -208,13 +208,29 @@ static __always_inline void trace_clear_recursion(int bit)
* Use this for ftrace callbacks. This will detect if the function
* tracing recursed in the same context (normal vs interrupt),
*
+ * The ftrace_test_recursion_trylock() will disable preemption,
+ * which is required for the variant of synchronize_rcu() that is
+ * used to allow patching functions where RCU is not watching.
+ * See klp_synchronize_transition() for more details.
+ *
* Returns: -1 if a recursion happened.
* >= 0 if no recursion
*/
static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
unsigned long parent_ip)
{
- return trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
+ int bit;
+
+ bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX);
+ /*
+ * The zero bit indicate we are nested
+ * in another trylock(), which means the
+ * preemption already disabled.
+ */
+ if (bit > 0)
+ preempt_disable_notrace();
+
+ return bit;
}
/**
@@ -222,9 +238,13 @@ static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
* @bit: The return of a successful ftrace_test_recursion_trylock()
*
* This is used at the end of a ftrace callback.
+ *
+ * Preemption will be enabled (if it was previously enabled).
*/
static __always_inline void ftrace_test_recursion_unlock(int bit)
{
+ if (bit)
+ preempt_enable_notrace();
trace_clear_recursion(bit);
}
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index e8029ae..6e66ccd 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -52,11 +52,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
bit = ftrace_test_recursion_trylock(ip, parent_ip);
if (WARN_ON_ONCE(bit < 0))
return;
- /*
- * A variant of synchronize_rcu() is used to allow patching functions
- * where RCU is not watching, see klp_synchronize_transition().
- */
- preempt_disable_notrace();
func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
stack_node);
@@ -120,7 +115,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
klp_arch_set_pc(fregs, (unsigned long)func->new_func);
unlock:
- preempt_enable_notrace();
ftrace_test_recursion_unlock(bit);
}
diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 1f0e63f..9f1bfbe 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -186,7 +186,6 @@ static void function_trace_start(struct trace_array *tr)
return;
trace_ctx = tracing_gen_ctx();
- preempt_disable_notrace();
cpu = smp_processor_id();
data = per_cpu_ptr(tr->array_buffer.data, cpu);
@@ -194,7 +193,6 @@ static void function_trace_start(struct trace_array *tr)
trace_function(tr, ip, parent_ip, trace_ctx);
ftrace_test_recursion_unlock(bit);
- preempt_enable_notrace();
}
#ifdef CONFIG_UNWINDER_ORC
@@ -298,8 +296,6 @@ static inline void process_repeats(struct trace_array *tr,
if (bit < 0)
return;
- preempt_disable_notrace();
-
cpu = smp_processor_id();
data = per_cpu_ptr(tr->array_buffer.data, cpu);
if (atomic_read(&data->disabled))
@@ -324,7 +320,6 @@ static inline void process_repeats(struct trace_array *tr,
out:
ftrace_test_recursion_unlock(bit);
- preempt_enable_notrace();
}
static void
--
1.8.3.1
^ permalink raw reply related
* [RESEND PATCH v2 0/2] fix & prevent the missing preemption disabling
From: 王贇 @ 2021-10-13 3:37 UTC (permalink / raw)
To: Guo Ren, Steven Rostedt, Ingo Molnar, James E.J. Bottomley,
Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Thomas Gleixner, Borislav Petkov, x86, H. Peter Anvin,
Josh Poimboeuf, Jiri Kosina, Miroslav Benes, Petr Mladek,
Joe Lawrence, Colin Ian King, Masami Hiramatsu,
Peter Zijlstra (Intel), Nicholas Piggin, Jisheng Zhang,
linux-csky, linux-kernel, linux-parisc, linuxppc-dev, linux-riscv,
live-patching
The testing show that perf_ftrace_function_call() are using smp_processor_id()
with preemption enabled, all the checking on CPU could be wrong after preemption.
As Peter point out, the section between ftrace_test_recursion_trylock/unlock()
pair require the preemption to be disabled as 'Documentation/trace/ftrace-uses.rst'
explained, but currently the work is done outside of the helpers.
Patch 1/2 will make sure preemption disabled after trylock() succeed,
patch 2/2 will do smp_processor_id() checking after trylock to address the
issue.
v1: https://lore.kernel.org/all/8c7de46d-9869-aa5e-2bb9-5dbc2eda395e@linux.alibaba.com/
Michael Wang (2):
ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
ftrace: do CPU checking after preemption disabled
arch/csky/kernel/probes/ftrace.c | 2 --
arch/parisc/kernel/ftrace.c | 2 --
arch/powerpc/kernel/kprobes-ftrace.c | 2 --
arch/riscv/kernel/probes/ftrace.c | 2 --
arch/x86/kernel/kprobes/ftrace.c | 2 --
include/linux/trace_recursion.h | 22 +++++++++++++++++++++-
kernel/livepatch/patch.c | 6 ------
kernel/trace/trace_event_perf.c | 6 +++---
kernel/trace/trace_functions.c | 5 -----
9 files changed, 24 insertions(+), 25 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH v2 0/2] fix & prevent the missing preemption disabling
From: 王贇 @ 2021-10-13 3:33 UTC (permalink / raw)
To: Steven Rostedt
Cc: Peter Zijlstra (Intel), Paul Walmsley, James E.J. Bottomley,
Guo Ren, Jisheng Zhang, H. Peter Anvin, live-patching,
linux-riscv, Miroslav Benes, Paul Mackerras, Joe Lawrence,
Helge Deller, x86, linux-csky, Ingo Molnar, Petr Mladek,
Albert Ou, Jiri Kosina, Nicholas Piggin, Borislav Petkov,
Josh Poimboeuf, Thomas Gleixner, linux-parisc, linux-kernel,
Palmer Dabbelt, Masami Hiramatsu, Colin Ian King, linuxppc-dev
In-Reply-To: <20211012232658.7dac3f43@oasis.local.home>
On 2021/10/13 上午11:26, Steven Rostedt wrote:
> Please start a new thread when sending new versions. v2 should not be a
> reply to v1. If you want to reference v1, just add it to the cover
> letter with a link tag:
>
> Link: https://lore.kernel.org/all/8c7de46d-9869-aa5e-2bb9-5dbc2eda395e@linux.alibaba.com/
Ok, I'll resend it with link then.
Regards,
Michael Wang
>
> -- Steve
>
>
> On Wed, 13 Oct 2021 11:16:56 +0800
> 王贇 <yun.wang@linux.alibaba.com> wrote:
>
>> The testing show that perf_ftrace_function_call() are using smp_processor_id()
>> with preemption enabled, all the checking on CPU could be wrong after preemption.
>>
>> As Peter point out, the section between ftrace_test_recursion_trylock/unlock()
>> pair require the preemption to be disabled as 'Documentation/trace/ftrace-uses.rst'
>> explained, but currently the work is done outside of the helpers.
>>
>> Patch 1/2 will make sure preemption disabled after trylock() succeed,
>> patch 2/2 will do smp_processor_id() checking after trylock to address the
>> issue.
>>
>> Michael Wang (2):
>> ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
>> ftrace: do CPU checking after preemption disabled
>>
>> arch/csky/kernel/probes/ftrace.c | 2 --
>> arch/parisc/kernel/ftrace.c | 2 --
>> arch/powerpc/kernel/kprobes-ftrace.c | 2 --
>> arch/riscv/kernel/probes/ftrace.c | 2 --
>> arch/x86/kernel/kprobes/ftrace.c | 2 --
>> include/linux/trace_recursion.h | 22 +++++++++++++++++++++-
>> kernel/livepatch/patch.c | 6 ------
>> kernel/trace/trace_event_perf.c | 6 +++---
>> kernel/trace/trace_functions.c | 5 -----
>> 9 files changed, 24 insertions(+), 25 deletions(-)
>>
^ permalink raw reply
* Re: [PATCH v2 0/2] fix & prevent the missing preemption disabling
From: Steven Rostedt @ 2021-10-13 3:26 UTC (permalink / raw)
To: 王贇
Cc: Peter Zijlstra (Intel), Paul Walmsley, James E.J. Bottomley,
Guo Ren, Jisheng Zhang, H. Peter Anvin, live-patching,
linux-riscv, Miroslav Benes, Paul Mackerras, Joe Lawrence,
Helge Deller, x86, linux-csky, Ingo Molnar, Petr Mladek,
Albert Ou, Jiri Kosina, Nicholas Piggin, Borislav Petkov,
Josh Poimboeuf, Thomas Gleixner, linux-parisc, linux-kernel,
Palmer Dabbelt, Masami Hiramatsu, Colin Ian King, linuxppc-dev
In-Reply-To: <1a8e8d73-b508-f90b-0d82-eb2da45a720e@linux.alibaba.com>
Please start a new thread when sending new versions. v2 should not be a
reply to v1. If you want to reference v1, just add it to the cover
letter with a link tag:
Link: https://lore.kernel.org/all/8c7de46d-9869-aa5e-2bb9-5dbc2eda395e@linux.alibaba.com/
-- Steve
On Wed, 13 Oct 2021 11:16:56 +0800
王贇 <yun.wang@linux.alibaba.com> wrote:
> The testing show that perf_ftrace_function_call() are using smp_processor_id()
> with preemption enabled, all the checking on CPU could be wrong after preemption.
>
> As Peter point out, the section between ftrace_test_recursion_trylock/unlock()
> pair require the preemption to be disabled as 'Documentation/trace/ftrace-uses.rst'
> explained, but currently the work is done outside of the helpers.
>
> Patch 1/2 will make sure preemption disabled after trylock() succeed,
> patch 2/2 will do smp_processor_id() checking after trylock to address the
> issue.
>
> Michael Wang (2):
> ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
> ftrace: do CPU checking after preemption disabled
>
> arch/csky/kernel/probes/ftrace.c | 2 --
> arch/parisc/kernel/ftrace.c | 2 --
> arch/powerpc/kernel/kprobes-ftrace.c | 2 --
> arch/riscv/kernel/probes/ftrace.c | 2 --
> arch/x86/kernel/kprobes/ftrace.c | 2 --
> include/linux/trace_recursion.h | 22 +++++++++++++++++++++-
> kernel/livepatch/patch.c | 6 ------
> kernel/trace/trace_event_perf.c | 6 +++---
> kernel/trace/trace_functions.c | 5 -----
> 9 files changed, 24 insertions(+), 25 deletions(-)
>
^ 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