* [PATCH] powerpc/ptrace: Remove duplicate check from pt_regs_check()
From: Denis Efremov @ 2021-03-05 11:28 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev, Paul Mackerras, linux-kernel, Denis Efremov
"offsetof(struct pt_regs, msr) == offsetof(struct user_pt_regs, msr)"
checked in pt_regs_check() twice in a row. Remove the second check.
Signed-off-by: Denis Efremov <efremov@linux.com>
---
arch/powerpc/kernel/ptrace/ptrace.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
index 4f3d4ff3728c..51801777906c 100644
--- a/arch/powerpc/kernel/ptrace/ptrace.c
+++ b/arch/powerpc/kernel/ptrace/ptrace.c
@@ -354,8 +354,6 @@ void __init pt_regs_check(void)
offsetof(struct user_pt_regs, nip));
BUILD_BUG_ON(offsetof(struct pt_regs, msr) !=
offsetof(struct user_pt_regs, msr));
- BUILD_BUG_ON(offsetof(struct pt_regs, msr) !=
- offsetof(struct user_pt_regs, msr));
BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
offsetof(struct user_pt_regs, orig_gpr3));
BUILD_BUG_ON(offsetof(struct pt_regs, ctr) !=
--
2.26.2
^ permalink raw reply related
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Marco Elver @ 2021-03-05 9:14 UTC (permalink / raw)
To: Christophe Leroy
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <f6e47f4f-6953-6584-f023-8b9c22d6974e@csgroup.eu>
On Fri, 5 Mar 2021 at 09:23, Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 05/03/2021 à 08:50, Marco Elver a écrit :
> > On Fri, Mar 05, 2021 at 04:01PM +1100, Michael Ellerman wrote:
> >> Marco Elver <elver@google.com> writes:
> >>> On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
> >>>> Le 04/03/2021 à 12:31, Marco Elver a écrit :
> >>>>> On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
> >>>>> <christophe.leroy@csgroup.eu> wrote:
> >>>>>> Le 03/03/2021 à 11:56, Marco Elver a écrit :
> >>>>>>>
> >>>>>>> Somewhat tangentially, I also note that e.g. show_regs(regs) (which
> >>>>>>> was printed along the KFENCE report above) didn't include the top
> >>>>>>> frame in the "Call Trace", so this assumption is definitely not
> >>>>>>> isolated to KFENCE.
> >>>>>>>
> >>>>>>
> >>>>>> Now, I have tested PPC64 (with the patch I sent yesterday to modify save_stack_trace_regs()
> >>>>>> applied), and I get many failures. Any idea ?
> >>>>>>
> >>>>>> [ 17.653751][ T58] ==================================================================
> >>>>>> [ 17.654379][ T58] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530
> >>>>>> [ 17.654379][ T58]
> >>>>>> [ 17.654831][ T58] Invalid free of 0xc00000003c9c0000 (in kfence-#77):
> >>>>>> [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
> >>>>>> [ 17.655775][ T58] .__slab_free+0x320/0x5a0
> >>>>>> [ 17.656039][ T58] .test_double_free+0xe0/0x198
> >>>>>> [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
> >>>>>> [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
> >>>>>> [ 17.657161][ T58] .kthread+0x18c/0x1a0
> >>>>>> [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
> >>>>>> [ 17.659869][ T58]
> >>> [...]
> >>>>>
> >>>>> Looks like something is prepending '.' to function names. We expect
> >>>>> the function name to appear as-is, e.g. "kfence_guarded_free",
> >>>>> "test_double_free", etc.
> >>>>>
> >>>>> Is there something special on ppc64, where the '.' is some convention?
> >>>>>
> >>>>
> >>>> I think so, see https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
> >>>>
> >>>> Also see commit https://github.com/linuxppc/linux/commit/02424d896
> >>>
> >>> Thanks -- could you try the below patch? You'll need to define
> >>> ARCH_FUNC_PREFIX accordingly.
> >>>
> >>> We think, since there are only very few architectures that add a prefix,
> >>> requiring <asm/kfence.h> to define something like ARCH_FUNC_PREFIX is
> >>> the simplest option. Let me know if this works for you.
> >>>
> >>> There an alternative option, which is to dynamically figure out the
> >>> prefix, but if this simpler option is fine with you, we'd prefer it.
> >>
> >> We have rediscovered this problem in basically every tracing / debugging
> >> feature added in the last 20 years :)
> >>
> >> I think the simplest solution is the one tools/perf/util/symbol.c uses,
> >> which is to just skip a leading '.'.
> >>
> >> Does that work?
> >>
> >> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> >> index ab83d5a59bb1..67b49dc54b38 100644
> >> --- a/mm/kfence/report.c
> >> +++ b/mm/kfence/report.c
> >> @@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
> >> for (skipnr = 0; skipnr < num_entries; skipnr++) {
> >> int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
> >>
> >> + if (buf[0] == '.')
> >> + buf++;
> >> +
> >
> > Unfortunately this does not work, since buf is an array. We'd need an
> > offset, and it should be determined outside the loop. I had a solution
> > like this, but it turned out quite complex (see below). And since most
> > architectures do not require this, decided that the safest option is to
> > use the macro approach with ARCH_FUNC_PREFIX, for which Christophe
> > already prepared a patch and tested:
> > https://lore.kernel.org/linux-mm/20210304144000.1148590-1-elver@google.com/
> > https://lkml.kernel.org/r/afaec81a551ef15345cb7d7563b3fac3d7041c3a.1614868445.git.christophe.leroy@csgroup.eu
> >
> > Since KFENCE requires <asm/kfence.h> anyway, we'd prefer this approach
> > (vs. dynamically detecting).
> >
> > Thanks,
> > -- Marco
> >
>
> What about
Sure something like that would work. But I explicitly did *not* want
to hard-code the '.' in non-arch code.
The choice is between:
1. ARCH_FUNC_PREFIX (as a matter of fact, the ARCH_FUNC_PREFIX patch
is already in -mm). Perhaps we could optimize it further, by checking
ARCH_FUNC_PREFIX in buf, and advancing buf like you propose, but I'm
not sure it's worth worrying about.
2. The dynamic solution that I proposed that does not use a hard-coded
'.' (or some variation thereof).
Please tell me which solution you prefer, 1 or 2 -- I'd like to stop
bikeshedding here. If there's a compelling argument for hard-coding
the '.' in non-arch code, please clarify, but otherwise I'd like to
keep arch-specific things out of generic code.
Thanks.
> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> index 519f037720f5..5e196625fb34 100644
> --- a/mm/kfence/report.c
> +++ b/mm/kfence/report.c
> @@ -43,7 +43,7 @@ static void seq_con_printf(struct seq_file *seq, const char *fmt, ...)
> static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries,
> const enum kfence_error_type *type)
> {
> - char buf[64];
> + char _buf[64];
> int skipnr, fallback = 0;
>
> if (type) {
> @@ -65,7 +65,11 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
> }
>
> for (skipnr = 0; skipnr < num_entries; skipnr++) {
> - int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
> + char *buf = _buf;
> + int len = scnprintf(_buf, sizeof(_buf), "%ps", (void *)stack_entries[skipnr]);
> +
> + if (_buf[0] == '.')
> + buf++, len--;
>
> if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf, "__kfence_") ||
> !strncmp(buf, "__slab_free", len)) {
> ---
>
> Christophe
^ permalink raw reply
* Re: [PATCH v5 05/22] powerpc/irq: Add helper to set regs->softe
From: Christophe Leroy @ 2021-03-05 8:54 UTC (permalink / raw)
To: Nicholas Piggin, Benjamin Herrenschmidt, Michael Ellerman,
msuchanek, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1612856863.0x6ebz3hce.astroid@bobo.none>
Le 09/02/2021 à 08:49, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of February 9, 2021 4:18 pm:
>>
>>
>> Le 09/02/2021 à 02:11, Nicholas Piggin a écrit :
>>> Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am:
>>>> regs->softe doesn't exist on PPC32.
>>>>
>>>> Add irq_soft_mask_regs_set_state() helper to set regs->softe.
>>>> This helper will void on PPC32.
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>> ---
>>>
>>> You could do the same with the kuap_ functions to change some ifdefs
>>> to IS_ENABLED.
>>>
>>> That's just my preference but if you prefer this way I guess that's
>>> okay.
>>>
>>
>>
>> That's also my preference on the long term.
>>
>> Here it is ephemeral, I have a follow up series implementing interrupt exit/entry in C and getting
>> rid of all the assembly kuap hence getting rid of those ifdefs.
>
> I thought it might have been because you hate ifdef more tha most :)
>
>> The issue I see when using IS_ENABLED() is that you have to indent to the right, then you interfere
>> with the file history and 'git blame'
>
> Valid point if it's just going to indent back the other way in your next
> series.
>
>> Thanks for reviewing my series and looking forward to your feedback on my series on the interrupt
>> entry/exit that I will likely release later today.
>
> Cool, I'm eager to see them.
>
Hi Nick, have you been able to look at it ?
https://patchwork.ozlabs.org/project/linuxppc-dev/cover/cover.1612864003.git.christophe.leroy@csgroup.eu/
Thanks
Christophe
^ permalink raw reply
* [PATCH v1] ibmvnic: remove excessive irqsave
From: angkery @ 2021-03-05 8:48 UTC (permalink / raw)
To: mpe, benh, paulus, drt, ljp, sukadev, davem, kuba
Cc: netdev, linuxppc-dev, linux-kernel, Junlin Yang
From: Junlin Yang <yangjunlin@yulong.com>
ibmvnic_remove locks multiple spinlocks while disabling interrupts:
spin_lock_irqsave(&adapter->state_lock, flags);
spin_lock_irqsave(&adapter->rwi_lock, flags);
As reported by coccinelle, the second _irqsave() overwrites the value
saved in 'flags' by the first _irqsave(), therefore when the second
_irqrestore() comes,the value in 'flags' is not valid,the value saved
by the first _irqsave() has been lost.
This likely leads to IRQs remaining disabled. So remove the second
_irqsave():
spin_lock_irqsave(&adapter->state_lock, flags);
spin_lock(&adapter->rwi_lock);
Generated by: ./scripts/coccinelle/locks/flags.cocci
./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
ERROR: nested lock+irqsave that reuses flags from line 5404.
Fixes: 4a41c421f367 ("ibmvnic: serialize access to work queue on remove")
Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
---
Changes in v1:
a.According to Christophe Leroy's explanation, update the commit information.
b.Add fixes tags.
drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 2464c8a..a52668d 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev *dev)
* after setting state, so __ibmvnic_reset() which is called
* from the flush_work() below, can make progress.
*/
- spin_lock_irqsave(&adapter->rwi_lock, flags);
+ spin_lock(&adapter->rwi_lock);
adapter->state = VNIC_REMOVING;
- spin_unlock_irqrestore(&adapter->rwi_lock, flags);
+ spin_unlock(&adapter->rwi_lock);
spin_unlock_irqrestore(&adapter->state_lock, flags);
--
1.9.1
^ permalink raw reply related
* [PATCH v3] powerpc/32: remove bogus ppc_select syscall
From: Christophe Leroy @ 2021-03-05 8:40 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Arnd Bergmann
Cc: linuxppc-dev, linux-kernel
From: Arnd Bergmann <arnd@arndb.de>
The ppc_select function was introduced in linux-2.3.48 in order to support
code confusing the legacy select() calling convention with the standard one.
Even 24 years ago, all correctly built code should not have done this and
could have easily been phased out. Nothing that was compiled later should
actually try to use the old_select interface, and it would have been broken
already on all ppc64 kernels with the syscall emulation layer.
This patch brings the 32 bit compat ABI and the native 32 bit ABI for
powerpc into a consistent state, by removing support for both the
old_select system call number and the handler for it.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
[chleroy: Rebased and updated the number of years elapsed and dropped last part of the commit message]
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
First version was in 2008, at that time it was rejected, see
http://patchwork.ozlabs.org/project/linuxppc-dev/patch/200809240839.14902.arnd@arndb.de/
A reduced version of it was merged as commit dad2f2fb0fc7 ("powerpc: Fix wrong error code from ppc32 select syscall")
If we decide to still keep this, then we'll have to:
- take into account -4096 < fd < 0 case
- use unsafe_get_user inside a uaccess_begin block
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/asm-prototypes.h | 3 ---
arch/powerpc/kernel/syscalls.c | 25 -----------------------
arch/powerpc/kernel/syscalls/syscall.tbl | 4 +---
3 files changed, 1 insertion(+), 31 deletions(-)
diff --git a/arch/powerpc/include/asm/asm-prototypes.h b/arch/powerpc/include/asm/asm-prototypes.h
index 939f3c94c8f3..78e0a3bd448a 100644
--- a/arch/powerpc/include/asm/asm-prototypes.h
+++ b/arch/powerpc/include/asm/asm-prototypes.h
@@ -63,9 +63,6 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
#ifdef CONFIG_PPC32
long sys_debug_setcontext(struct ucontext __user *ctx,
int ndbg, struct sig_dbg_op __user *dbg);
-int
-ppc_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp,
- struct __kernel_old_timeval __user *tvp);
unsigned long __init early_init(unsigned long dt_ptr);
void __init machine_init(u64 dt_ptr);
#endif
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index 078608ec2e92..70b0eb5bedfd 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -71,31 +71,6 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
return do_mmap2(addr, len, prot, flags, fd, offset, PAGE_SHIFT);
}
-#ifdef CONFIG_PPC32
-/*
- * Due to some executables calling the wrong select we sometimes
- * get wrong args. This determines how the args are being passed
- * (a single ptr to them all args passed) then calls
- * sys_select() with the appropriate args. -- Cort
- */
-int
-ppc_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp, struct __kernel_old_timeval __user *tvp)
-{
- if ( (unsigned long)n >= 4096 )
- {
- unsigned long __user *buffer = (unsigned long __user *)n;
- if (!access_ok(buffer, 5*sizeof(unsigned long))
- || __get_user(n, buffer)
- || __get_user(inp, ((fd_set __user * __user *)(buffer+1)))
- || __get_user(outp, ((fd_set __user * __user *)(buffer+2)))
- || __get_user(exp, ((fd_set __user * __user *)(buffer+3)))
- || __get_user(tvp, ((struct __kernel_old_timeval __user * __user *)(buffer+4))))
- return -EFAULT;
- }
- return sys_select(n, inp, outp, exp, tvp);
-}
-#endif
-
#ifdef CONFIG_PPC64
long ppc64_personality(unsigned long personality)
{
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 0b2480cf3e47..5bb0e90e502e 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -110,9 +110,7 @@
79 common settimeofday sys_settimeofday compat_sys_settimeofday
80 common getgroups sys_getgroups
81 common setgroups sys_setgroups
-82 32 select ppc_select sys_ni_syscall
-82 64 select sys_ni_syscall
-82 spu select sys_ni_syscall
+82 common select sys_ni_syscall
83 common symlink sys_symlink
84 32 oldlstat sys_lstat sys_ni_syscall
84 64 oldlstat sys_ni_syscall
--
2.25.0
^ permalink raw reply related
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Christophe Leroy @ 2021-03-05 8:23 UTC (permalink / raw)
To: Marco Elver, Michael Ellerman
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <YEHiq1ALdPn2crvP@elver.google.com>
Le 05/03/2021 à 08:50, Marco Elver a écrit :
> On Fri, Mar 05, 2021 at 04:01PM +1100, Michael Ellerman wrote:
>> Marco Elver <elver@google.com> writes:
>>> On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
>>>> Le 04/03/2021 à 12:31, Marco Elver a écrit :
>>>>> On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
>>>>> <christophe.leroy@csgroup.eu> wrote:
>>>>>> Le 03/03/2021 à 11:56, Marco Elver a écrit :
>>>>>>>
>>>>>>> Somewhat tangentially, I also note that e.g. show_regs(regs) (which
>>>>>>> was printed along the KFENCE report above) didn't include the top
>>>>>>> frame in the "Call Trace", so this assumption is definitely not
>>>>>>> isolated to KFENCE.
>>>>>>>
>>>>>>
>>>>>> Now, I have tested PPC64 (with the patch I sent yesterday to modify save_stack_trace_regs()
>>>>>> applied), and I get many failures. Any idea ?
>>>>>>
>>>>>> [ 17.653751][ T58] ==================================================================
>>>>>> [ 17.654379][ T58] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530
>>>>>> [ 17.654379][ T58]
>>>>>> [ 17.654831][ T58] Invalid free of 0xc00000003c9c0000 (in kfence-#77):
>>>>>> [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
>>>>>> [ 17.655775][ T58] .__slab_free+0x320/0x5a0
>>>>>> [ 17.656039][ T58] .test_double_free+0xe0/0x198
>>>>>> [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
>>>>>> [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
>>>>>> [ 17.657161][ T58] .kthread+0x18c/0x1a0
>>>>>> [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
>>>>>> [ 17.659869][ T58]
>>> [...]
>>>>>
>>>>> Looks like something is prepending '.' to function names. We expect
>>>>> the function name to appear as-is, e.g. "kfence_guarded_free",
>>>>> "test_double_free", etc.
>>>>>
>>>>> Is there something special on ppc64, where the '.' is some convention?
>>>>>
>>>>
>>>> I think so, see https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
>>>>
>>>> Also see commit https://github.com/linuxppc/linux/commit/02424d896
>>>
>>> Thanks -- could you try the below patch? You'll need to define
>>> ARCH_FUNC_PREFIX accordingly.
>>>
>>> We think, since there are only very few architectures that add a prefix,
>>> requiring <asm/kfence.h> to define something like ARCH_FUNC_PREFIX is
>>> the simplest option. Let me know if this works for you.
>>>
>>> There an alternative option, which is to dynamically figure out the
>>> prefix, but if this simpler option is fine with you, we'd prefer it.
>>
>> We have rediscovered this problem in basically every tracing / debugging
>> feature added in the last 20 years :)
>>
>> I think the simplest solution is the one tools/perf/util/symbol.c uses,
>> which is to just skip a leading '.'.
>>
>> Does that work?
>>
>> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
>> index ab83d5a59bb1..67b49dc54b38 100644
>> --- a/mm/kfence/report.c
>> +++ b/mm/kfence/report.c
>> @@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
>> for (skipnr = 0; skipnr < num_entries; skipnr++) {
>> int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
>>
>> + if (buf[0] == '.')
>> + buf++;
>> +
>
> Unfortunately this does not work, since buf is an array. We'd need an
> offset, and it should be determined outside the loop. I had a solution
> like this, but it turned out quite complex (see below). And since most
> architectures do not require this, decided that the safest option is to
> use the macro approach with ARCH_FUNC_PREFIX, for which Christophe
> already prepared a patch and tested:
> https://lore.kernel.org/linux-mm/20210304144000.1148590-1-elver@google.com/
> https://lkml.kernel.org/r/afaec81a551ef15345cb7d7563b3fac3d7041c3a.1614868445.git.christophe.leroy@csgroup.eu
>
> Since KFENCE requires <asm/kfence.h> anyway, we'd prefer this approach
> (vs. dynamically detecting).
>
> Thanks,
> -- Marco
>
What about
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index 519f037720f5..5e196625fb34 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -43,7 +43,7 @@ static void seq_con_printf(struct seq_file *seq, const char *fmt, ...)
static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries,
const enum kfence_error_type *type)
{
- char buf[64];
+ char _buf[64];
int skipnr, fallback = 0;
if (type) {
@@ -65,7 +65,11 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
}
for (skipnr = 0; skipnr < num_entries; skipnr++) {
- int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
+ char *buf = _buf;
+ int len = scnprintf(_buf, sizeof(_buf), "%ps", (void *)stack_entries[skipnr]);
+
+ if (_buf[0] == '.')
+ buf++, len--;
if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf, "__kfence_") ||
!strncmp(buf, "__slab_free", len)) {
---
Christophe
^ permalink raw reply related
* Re: [PATCH] powerpc/pseries: export LPAR security flavor in lparcfg
From: Laurent Dufour @ 2021-03-05 8:04 UTC (permalink / raw)
To: Michael Ellerman, benh, paulus, linuxppc-dev
Cc: nathanl, cheloha, linux-kernel
In-Reply-To: <871rcuruee.fsf@mpe.ellerman.id.au>
Le 05/03/2021 à 07:23, Michael Ellerman a écrit :
> Laurent Dufour <ldufour@linux.ibm.com> writes:
>> This is helpful to read the security flavor from inside the LPAR.
>
> We already have /sys/kernel/debug/powerpc/security_features.
>
> Is that not sufficient?
Not really, it only reports that security mitigation are on or off but not the
level set through the ASMI menu. Furthermore, reporting it through
/proc/powerpc/lparcfg allows an easy processing by the lparstat command (see below).
>
>> Export it like this in /proc/powerpc/lparcfg:
>>
>> $ grep security_flavor /proc/powerpc/lparcfg
>> security_flavor=1
>>
>> Value means:
>> 0 Speculative execution fully enabled
>> 1 Speculative execution controls to mitigate user-to-kernel attacks
>> 2 Speculative execution controls to mitigate user-to-kernel and
>> user-to-user side-channel attacks
>
> Those strings come from the FSP help, but we have no guarantee it won't
> mean something different in future.
I think this is nailed down, those strings came from:
https://www.ibm.com/support/pages/node/715841
Where it is written (regarding AIX):
On an LPAR, one can use lparstat -x to display the current mitigation mode:
0 = Speculative execution fully enabled
1 = Speculative execution controls to mitigate user-to-kernel side-channel attacks
2 = Speculative execution controls to mitigate user-to-kernel and user-to-user
side-channel attacks
We have been requested to provide almost the same, which I proposed in
powerpc-utils:
https://groups.google.com/g/powerpc-utils-devel/c/NaKXvdyl_UI/m/wa2stpIDAQAJ
Thanks,
Laurent.
^ permalink raw reply
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Marco Elver @ 2021-03-05 7:50 UTC (permalink / raw)
To: Michael Ellerman
Cc: LKML, Paul Mackerras, kasan-dev, Alexander Potapenko,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <874khqry78.fsf@mpe.ellerman.id.au>
On Fri, Mar 05, 2021 at 04:01PM +1100, Michael Ellerman wrote:
> Marco Elver <elver@google.com> writes:
> > On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
> >> Le 04/03/2021 à 12:31, Marco Elver a écrit :
> >> > On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
> >> > <christophe.leroy@csgroup.eu> wrote:
> >> > > Le 03/03/2021 à 11:56, Marco Elver a écrit :
> >> > > >
> >> > > > Somewhat tangentially, I also note that e.g. show_regs(regs) (which
> >> > > > was printed along the KFENCE report above) didn't include the top
> >> > > > frame in the "Call Trace", so this assumption is definitely not
> >> > > > isolated to KFENCE.
> >> > > >
> >> > >
> >> > > Now, I have tested PPC64 (with the patch I sent yesterday to modify save_stack_trace_regs()
> >> > > applied), and I get many failures. Any idea ?
> >> > >
> >> > > [ 17.653751][ T58] ==================================================================
> >> > > [ 17.654379][ T58] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530
> >> > > [ 17.654379][ T58]
> >> > > [ 17.654831][ T58] Invalid free of 0xc00000003c9c0000 (in kfence-#77):
> >> > > [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
> >> > > [ 17.655775][ T58] .__slab_free+0x320/0x5a0
> >> > > [ 17.656039][ T58] .test_double_free+0xe0/0x198
> >> > > [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
> >> > > [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
> >> > > [ 17.657161][ T58] .kthread+0x18c/0x1a0
> >> > > [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
> >> > > [ 17.659869][ T58]
> > [...]
> >> >
> >> > Looks like something is prepending '.' to function names. We expect
> >> > the function name to appear as-is, e.g. "kfence_guarded_free",
> >> > "test_double_free", etc.
> >> >
> >> > Is there something special on ppc64, where the '.' is some convention?
> >> >
> >>
> >> I think so, see https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
> >>
> >> Also see commit https://github.com/linuxppc/linux/commit/02424d896
> >
> > Thanks -- could you try the below patch? You'll need to define
> > ARCH_FUNC_PREFIX accordingly.
> >
> > We think, since there are only very few architectures that add a prefix,
> > requiring <asm/kfence.h> to define something like ARCH_FUNC_PREFIX is
> > the simplest option. Let me know if this works for you.
> >
> > There an alternative option, which is to dynamically figure out the
> > prefix, but if this simpler option is fine with you, we'd prefer it.
>
> We have rediscovered this problem in basically every tracing / debugging
> feature added in the last 20 years :)
>
> I think the simplest solution is the one tools/perf/util/symbol.c uses,
> which is to just skip a leading '.'.
>
> Does that work?
>
> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> index ab83d5a59bb1..67b49dc54b38 100644
> --- a/mm/kfence/report.c
> +++ b/mm/kfence/report.c
> @@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
> for (skipnr = 0; skipnr < num_entries; skipnr++) {
> int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
>
> + if (buf[0] == '.')
> + buf++;
> +
Unfortunately this does not work, since buf is an array. We'd need an
offset, and it should be determined outside the loop. I had a solution
like this, but it turned out quite complex (see below). And since most
architectures do not require this, decided that the safest option is to
use the macro approach with ARCH_FUNC_PREFIX, for which Christophe
already prepared a patch and tested:
https://lore.kernel.org/linux-mm/20210304144000.1148590-1-elver@google.com/
https://lkml.kernel.org/r/afaec81a551ef15345cb7d7563b3fac3d7041c3a.1614868445.git.christophe.leroy@csgroup.eu
Since KFENCE requires <asm/kfence.h> anyway, we'd prefer this approach
(vs. dynamically detecting).
Thanks,
-- Marco
------ >8 ------
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index 519f037720f5..b0590199b039 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -43,8 +43,8 @@ static void seq_con_printf(struct seq_file *seq, const char *fmt, ...)
static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries,
const enum kfence_error_type *type)
{
+ int skipnr, fallback = 0, fprefix_chars = 0;
char buf[64];
- int skipnr, fallback = 0;
if (type) {
/* Depending on error type, find different stack entries. */
@@ -64,11 +64,24 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
}
}
+ if (scnprintf(buf, sizeof(buf), "%ps", (void *)kfree)) {
+ /*
+ * Some architectures (e.g. ppc64) add a constant prefix to
+ * function names. Determine if such a prefix exists.
+ */
+ const char *str = strstr(buf, "kfree");
+
+ if (str)
+ fprefix_chars = str - buf;
+ }
+
for (skipnr = 0; skipnr < num_entries; skipnr++) {
- int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
+ int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]) -
+ fprefix_chars;
- if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf, "__kfence_") ||
- !strncmp(buf, "__slab_free", len)) {
+ if (str_has_prefix(buf + fprefix_chars, "kfence_") ||
+ str_has_prefix(buf + fprefix_chars, "__kfence_") ||
+ !strncmp(buf + fprefix_chars, "__slab_free", len)) {
/*
* In case of tail calls from any of the below
* to any of the above.
@@ -77,10 +90,10 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
}
/* Also the *_bulk() variants by only checking prefixes. */
- if (str_has_prefix(buf, "kfree") ||
- str_has_prefix(buf, "kmem_cache_free") ||
- str_has_prefix(buf, "__kmalloc") ||
- str_has_prefix(buf, "kmem_cache_alloc"))
+ if (str_has_prefix(buf + fprefix_chars, "kfree") ||
+ str_has_prefix(buf + fprefix_chars, "kmem_cache_free") ||
+ str_has_prefix(buf + fprefix_chars, "__kmalloc") ||
+ str_has_prefix(buf + fprefix_chars, "kmem_cache_alloc"))
goto found;
}
if (fallback < num_entries)
^ permalink raw reply related
* Re: [PATCH] ibmvnic: remove excessive irqsave
From: angkery @ 2021-03-05 7:03 UTC (permalink / raw)
To: Christophe Leroy
Cc: Junlin Yang, linux-kernel, kuba, netdev, ljp, drt, paulus,
sukadev, linuxppc-dev, davem
In-Reply-To: <67215668-0850-a0f3-06e1-49db590b8fcc@csgroup.eu>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=GB18030, Size: 2197 bytes --]
On Fri, 5 Mar 2021 06:49:14 +0100
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> Le 05/03/2021 ¨¤ 02:43, angkery a ¨¦crit0„2:
> > From: Junlin Yang <yangjunlin@yulong.com>
> >
> > ibmvnic_remove locks multiple spinlocks while disabling interrupts:
> > spin_lock_irqsave(&adapter->state_lock, flags);
> > spin_lock_irqsave(&adapter->rwi_lock, flags);
> >
> > there is no need for the second irqsave,since interrupts are
> > disabled at that point, so remove the second irqsave:
>
> The probl¨¨me is not that there is no need. The problem is a lot more
> serious: As reported by coccinella, the second _irqsave() overwrites
> the value saved in 'flags' by the first _irqsave, therefore when the
> second _irqrestore comes, the value in 'flags' is not valid, the
> value saved by the first _irqsave has been lost. This likely leads to
> IRQs remaining disabled, which is _THE_ problem really.
>
Thank you for explaining the real problem.
I will update the commit information with your description.
> > spin_lock_irqsave(&adapter->state_lock, flags);
> > spin_lock(&adapter->rwi_lock);
> >
> > Generated by: ./scripts/coccinelle/locks/flags.cocci
> > ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
> > ERROR: nested lock+irqsave that reuses flags from line 5404.
> >
> > Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
> > ---
> > drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> > b/drivers/net/ethernet/ibm/ibmvnic.c index 2464c8a..a52668d 100644
> > --- a/drivers/net/ethernet/ibm/ibmvnic.c
> > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> > @@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev
> > *dev)
> > * after setting state, so __ibmvnic_reset() which is
> > called
> > * from the flush_work() below, can make progress.
> > */
> > - spin_lock_irqsave(&adapter->rwi_lock, flags);
> > + spin_lock(&adapter->rwi_lock);
> > adapter->state = VNIC_REMOVING;
> > - spin_unlock_irqrestore(&adapter->rwi_lock, flags);
> > + spin_unlock(&adapter->rwi_lock);
> >
> > spin_unlock_irqrestore(&adapter->state_lock, flags);
> >
> >
^ permalink raw reply
* Re: [PATCH v2 09/37] KVM: PPC: Book3S 64: Move hcall early register setup to KVM
From: Daniel Axtens @ 2021-03-05 6:48 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210225134652.2127648-10-npiggin@gmail.com>
Nicholas Piggin <npiggin@gmail.com> writes:
> System calls / hcalls have a different calling convention than
> other interrupts, so there is code in the KVMTEST to massage these
> into the same form as other interrupt handlers.
>
> Move this work into the KVM hcall handler. This means teaching KVM
> a little more about the low level interrupt handler setup, PACA save
> areas, etc., although that's not obviously worse than the current
> approach of coming up with an entirely different interrupt register
> / save convention.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/exception-64s.h | 13 +++++++
> arch/powerpc/kernel/exceptions-64s.S | 44 ++----------------------
> arch/powerpc/kvm/book3s_64_entry.S | 17 +++++++++
> 3 files changed, 32 insertions(+), 42 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> index c1a8aac01cf9..bb6f78fcf981 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -35,6 +35,19 @@
> /* PACA save area size in u64 units (exgen, exmc, etc) */
> #define EX_SIZE 10
>
> +/* PACA save area offsets */
> +#define EX_R9 0
> +#define EX_R10 8
> +#define EX_R11 16
> +#define EX_R12 24
> +#define EX_R13 32
> +#define EX_DAR 40
> +#define EX_DSISR 48
> +#define EX_CCR 52
> +#define EX_CFAR 56
> +#define EX_PPR 64
> +#define EX_CTR 72
I wonder - now that EX_blah and EX_SIZE both live in the one header
file, if we could calculate EX_SIZE based on the last offset... or have
a BUILD_BUG_ON or something? ... just trying not to lose the sanity
checking afforded to us by the .if macro we lose when moving these out
of exceptions-64s.S...
> +
> /*
> * maximum recursive depth of MCE exceptions
> */
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 9ae463e8522b..b7092ba87da8 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -21,22 +21,6 @@
> #include <asm/feature-fixups.h>
> #include <asm/kup.h>
>
> -/* PACA save area offsets (exgen, exmc, etc) */
> -#define EX_R9 0
> -#define EX_R10 8
> -#define EX_R11 16
> -#define EX_R12 24
> -#define EX_R13 32
> -#define EX_DAR 40
> -#define EX_DSISR 48
> -#define EX_CCR 52
> -#define EX_CFAR 56
> -#define EX_PPR 64
> -#define EX_CTR 72
> -.if EX_SIZE != 10
> - .error "EX_SIZE is wrong"
> -.endif
> -
> /*
> * Following are fixed section helper macros.
> *
> @@ -1964,45 +1948,21 @@ EXC_VIRT_END(system_call, 0x4c00, 0x100)
>
> #ifdef CONFIG_KVM_BOOK3S_64_HANDLER
> TRAMP_REAL_BEGIN(system_call_kvm)
> - /*
> - * This is a hcall, so register convention is as above, with these
> - * differences:
> - * r13 = PACA
> - * ctr = orig r13
> - * orig r10 saved in PACA
> - */
> - /*
> - * Save the PPR (on systems that support it) before changing to
> - * HMT_MEDIUM. That allows the KVM code to save that value into the
> - * guest state (it is the guest's PPR value).
> - */
> -BEGIN_FTR_SECTION
> - mfspr r10,SPRN_PPR
> - std r10,HSTATE_PPR(r13)
> -END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> - HMT_MEDIUM
> mfctr r10
> - SET_SCRATCH0(r10)
> - mfcr r10
> - std r12,HSTATE_SCRATCH0(r13)
> - sldi r12,r10,32
> - ori r12,r12,0xc00
> + SET_SCRATCH0(r10) /* Save r13 in SCRATCH0 */
> #ifdef CONFIG_RELOCATABLE
> /*
> - * Requires __LOAD_FAR_HANDLER beause kvmppc_interrupt lives
> + * Requires __LOAD_FAR_HANDLER beause kvmppc_hcall lives
That change probably belongs in the previous patch :)
Beyond that, I'm still wrapping my head around the deep magic going on
here! Hopefully I'll get a bit a bit further next time I look.
Kind regards,
Daniel
> * outside the head section.
> */
> __LOAD_FAR_HANDLER(r10, kvmppc_hcall)
> mtctr r10
> - ld r10,PACA_EXGEN+EX_R10(r13)
> bctr
> #else
> - ld r10,PACA_EXGEN+EX_R10(r13)
> b kvmppc_hcall
> #endif
> #endif
>
> -
> /**
> * Interrupt 0xd00 - Trace Interrupt.
> * This is a synchronous interrupt in response to instruction step or
> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
> index 9572f759255c..1c9518ab7d96 100644
> --- a/arch/powerpc/kvm/book3s_64_entry.S
> +++ b/arch/powerpc/kvm/book3s_64_entry.S
> @@ -13,6 +13,23 @@
> .global kvmppc_hcall
> .balign IFETCH_ALIGN_BYTES
> kvmppc_hcall:
> + /*
> + * This is a hcall, so register convention is as
> + * Documentation/powerpc/papr_hcalls.rst, with these additions:
> + * R13 = PACA
> + * guest R13 saved in SPRN_SCRATCH0
> + * R10 = free
> + */
> +BEGIN_FTR_SECTION
> + mfspr r10,SPRN_PPR
> + std r10,HSTATE_PPR(r13)
> +END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> + HMT_MEDIUM
> + mfcr r10
> + std r12,HSTATE_SCRATCH0(r13)
> + sldi r12,r10,32
> + ori r12,r12,0xc00
> + ld r10,PACA_EXGEN+EX_R10(r13)
>
> .global kvmppc_interrupt
> .balign IFETCH_ALIGN_BYTES
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Christophe Leroy @ 2021-03-05 6:38 UTC (permalink / raw)
To: Segher Boessenkool, Nick Desaulniers
Cc: Mark Rutland, Marco Elver, Will Deacon, Catalin Marinas, LKML,
kasan-dev, Mark Brown, Paul Mackerras, linux-toolchains,
linuxppc-dev, Linux ARM
In-Reply-To: <20210304192447.GT29191@gate.crashing.org>
Le 04/03/2021 à 20:24, Segher Boessenkool a écrit :
> On Thu, Mar 04, 2021 at 09:54:44AM -0800, Nick Desaulniers wrote:
>> On Thu, Mar 4, 2021 at 9:42 AM Marco Elver <elver@google.com> wrote:
>> include/linux/compiler.h:246:
>> prevent_tail_call_optimization
>>
>> commit a9a3ed1eff36 ("x86: Fix early boot crash on gcc-10, third try")
https://github.com/linuxppc/linux/commit/a9a3ed1eff36
>
> That is much heavier than needed (an mb()). You can just put an empty
> inline asm after a call before a return, and that call cannot be
> optimised to a sibling call: (the end of a function is an implicit
> return:)
>
> Instead of:
>
> void g(void);
> void f(int x)
> if (x)
> g();
> }
>
> Do:
>
> void g(void);
> void f(int x)
> if (x)
> g();
> asm("");
> }
>
> This costs no extra instructions, and certainly not something as heavy
> as an mb()! It works without the "if" as well, of course, but with it
> it is a more interesting example of a tail call.
In the commit mentionned at the top, it is said:
The next attempt to prevent compilers from tail-call optimizing
the last function call cpu_startup_entry(), ... , was to add an empty asm("").
This current solution was short and sweet, and reportedly, is supported
by both compilers but we didn't get very far this time: future (LTO?)
optimization passes could potentially eliminate this, which leads us
to the third attempt: having an actual memory barrier there which the
compiler cannot ignore or move around etc.
Christophe
^ permalink raw reply
* Re: linux-next: manual merge of the tty tree with the powerpc-fixes tree
From: Greg KH @ 2021-03-05 6:30 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Uwe Kleine-König, Jiri Slaby, PowerPC,
Linux Kernel Mailing List, Linux Next Mailing List, Jiri Slaby
In-Reply-To: <20210305120523.0cb114b9@canb.auug.org.au>
On Fri, Mar 05, 2021 at 12:05:23PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the tty tree got a conflict in:
>
> drivers/tty/hvc/hvcs.c
>
> between commit:
>
> 386a966f5ce7 ("vio: make remove callback return void")
>
> from the powerpc-fixes tree and commit:
>
> fb8d350c291c ("tty: hvc, drop unneeded forward declarations")
>
> from the tty tree.
>
> I fixed it up (they both removed the forward decalrartion of
> hvcs_remove(), but the latter removed more) and can carry the fix as
> necessary. This is now fixed as far as linux-next is concerned, but any
> non trivial conflicts should be mentioned to your upstream maintainer
> when your tree is submitted for merging. You may also want to consider
> cooperating with the maintainer of the conflicting tree to minimise any
> particularly complex conflicts.
Thanks, that's the correct fix, we knew this was going to happen :(
greg k-h
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: export LPAR security flavor in lparcfg
From: Michael Ellerman @ 2021-03-05 6:23 UTC (permalink / raw)
To: Laurent Dufour, benh, paulus, linuxppc-dev; +Cc: nathanl, cheloha, linux-kernel
In-Reply-To: <20210304114240.54112-1-ldufour@linux.ibm.com>
Laurent Dufour <ldufour@linux.ibm.com> writes:
> This is helpful to read the security flavor from inside the LPAR.
We already have /sys/kernel/debug/powerpc/security_features.
Is that not sufficient?
> Export it like this in /proc/powerpc/lparcfg:
>
> $ grep security_flavor /proc/powerpc/lparcfg
> security_flavor=1
>
> Value means:
> 0 Speculative execution fully enabled
> 1 Speculative execution controls to mitigate user-to-kernel attacks
> 2 Speculative execution controls to mitigate user-to-kernel and
> user-to-user side-channel attacks
Those strings come from the FSP help, but we have no guarantee it won't
mean something different in future.
cheers
^ permalink raw reply
* Re: [PATCH] ibmvnic: remove excessive irqsave
From: Lijun Pan @ 2021-03-05 6:04 UTC (permalink / raw)
To: Christophe Leroy
Cc: Junlin Yang, angkery, linux-kernel, Jakub Kicinski, netdev,
Lijun Pan, Dany Madden, paulus, Sukadev Bhattiprolu, linuxppc-dev,
David Miller
In-Reply-To: <67215668-0850-a0f3-06e1-49db590b8fcc@csgroup.eu>
> On Mar 4, 2021, at 11:49 PM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 05/03/2021 à 02:43, angkery a écrit :
>> From: Junlin Yang <yangjunlin@yulong.com>
>> ibmvnic_remove locks multiple spinlocks while disabling interrupts:
>> spin_lock_irqsave(&adapter->state_lock, flags);
>> spin_lock_irqsave(&adapter->rwi_lock, flags);
>> there is no need for the second irqsave,since interrupts are disabled
>> at that point, so remove the second irqsave:
>
> The problème is not that there is no need. The problem is a lot more serious:
> As reported by coccinella, the second _irqsave() overwrites the value saved in 'flags' by the first _irqsave, therefore when the second _irqrestore comes, the value in 'flags' is not valid, the value saved by the first _irqsave has been lost. This likely leads to IRQs remaining disabled, which is _THE_ problem really.
That does sounds like a more serious functional problem than coccinella check.
Thanks for your explanation.
>
>> spin_lock_irqsave(&adapter->state_lock, flags);
>> spin_lock(&adapter->rwi_lock);
>> Generated by: ./scripts/coccinelle/locks/flags.cocci
>> ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
>> ERROR: nested lock+irqsave that reuses flags from line 5404.
>> Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
>> ---
>> drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>> index 2464c8a..a52668d 100644
>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> @@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev *dev)
>> * after setting state, so __ibmvnic_reset() which is called
>> * from the flush_work() below, can make progress.
>> */
>> - spin_lock_irqsave(&adapter->rwi_lock, flags);
>> + spin_lock(&adapter->rwi_lock);
>> adapter->state = VNIC_REMOVING;
>> - spin_unlock_irqrestore(&adapter->rwi_lock, flags);
>> + spin_unlock(&adapter->rwi_lock);
>> spin_unlock_irqrestore(&adapter->state_lock, flags);
>>
^ permalink raw reply
* Re: [PATCH v2 08/37] KVM: PPC: Book3S 64: add hcall interrupt handler
From: Daniel Axtens @ 2021-03-05 6:03 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin, Fabiano Rosas
In-Reply-To: <20210225134652.2127648-9-npiggin@gmail.com>
Hi Nick,
> Add a separate hcall entry point. This can be used to deal with the
> different calling convention.
Looks good to me, makes sense, passes checkpatch.
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
> Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kernel/exceptions-64s.S | 4 ++--
> arch/powerpc/kvm/book3s_64_entry.S | 6 +++++-
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index d956dd9ed61f..9ae463e8522b 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -1992,13 +1992,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> * Requires __LOAD_FAR_HANDLER beause kvmppc_interrupt lives
> * outside the head section.
> */
> - __LOAD_FAR_HANDLER(r10, kvmppc_interrupt)
> + __LOAD_FAR_HANDLER(r10, kvmppc_hcall)
> mtctr r10
> ld r10,PACA_EXGEN+EX_R10(r13)
> bctr
> #else
> ld r10,PACA_EXGEN+EX_R10(r13)
> - b kvmppc_interrupt
> + b kvmppc_hcall
> #endif
> #endif
>
> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
> index c1276f616af4..9572f759255c 100644
> --- a/arch/powerpc/kvm/book3s_64_entry.S
> +++ b/arch/powerpc/kvm/book3s_64_entry.S
> @@ -7,9 +7,13 @@
> #include <asm/reg.h>
>
> /*
> - * This is branched to from interrupt handlers in exception-64s.S which set
> + * These are branched to from interrupt handlers in exception-64s.S which set
> * IKVM_REAL or IKVM_VIRT, if HSTATE_IN_GUEST was found to be non-zero.
> */
> +.global kvmppc_hcall
> +.balign IFETCH_ALIGN_BYTES
> +kvmppc_hcall:
> +
> .global kvmppc_interrupt
> .balign IFETCH_ALIGN_BYTES
> kvmppc_interrupt:
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH v2 07/37] KVM: PPC: Book3S 64: Move GUEST_MODE_SKIP test into KVM
From: Daniel Axtens @ 2021-03-05 5:54 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin, Fabiano Rosas
In-Reply-To: <20210225134652.2127648-8-npiggin@gmail.com>
Hi Nick,
> - .if IKVM_SKIP
> -89: mtocrf 0x80,r9
> - ld r10,IAREA+EX_CTR(r13)
> - mtctr r10
> - ld r9,IAREA+EX_R9(r13)
> - ld r10,IAREA+EX_R10(r13)
> - ld r11,IAREA+EX_R11(r13)
> - ld r12,IAREA+EX_R12(r13)
> - .if IHSRR_IF_HVMODE
> - BEGIN_FTR_SECTION
> - b kvmppc_skip_Hinterrupt
> - FTR_SECTION_ELSE
> - b kvmppc_skip_interrupt
> - ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
I was initially concerned that you were pulling out the complexities
around IHSRR_IF_HVMODE, but I checked exceptions-64s.S and the only
exception handler that sets IHSRR_IF_HVMODE is hardware_interrupt and
that does not set IKVM_SKIP, so we are indeed fine to not keep this
complex little asm section.
> - .elseif IHSRR
> - b kvmppc_skip_Hinterrupt
> - .else
> - b kvmppc_skip_interrupt
> - .endif
> - .endif
> .endm
> +/*
> + * KVM uses a trick where it is running in MSR[HV]=1 mode in real-mode with the
> + * guest MMU context loaded, and it sets KVM_GUEST_MODE_SKIP and enables
> + * MSR[DR]=1 while leaving MSR[IR]=0, so it continues to fetch HV instructions
> + * but loads and stores will access the guest context. This is used to load
> + * the faulting instruction without walking page tables.
> + *
> + * However the guest context may not be able to translate, or it may cause a
> + * machine check or other issue, which will result in a fault in the host
> + * (even with KVM-HV).
> + *
> + * These faults are caught here and if the fault was (or was likely) due to
> + * that load, then we just return with the PC advanced +4 and skip the load,
> + * which then goes via the slow path.
> + */
This is a really helpful comment! Thanks!
> +.Lmaybe_skip:
> + cmpwi r12,BOOK3S_INTERRUPT_MACHINE_CHECK
> + beq 1f
> + cmpwi r12,BOOK3S_INTERRUPT_DATA_STORAGE
> + beq 1f
> + cmpwi r12,BOOK3S_INTERRUPT_DATA_SEGMENT
> + beq 1f
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> + /* HSRR interrupts have 2 added to trap vector */
> + cmpwi r12,BOOK3S_INTERRUPT_H_DATA_STORAGE | 0x2
This took me by surprise for a while, but I checked how it works in
exceptions-64s.S and indeed the GEN_KVM macro will add 0x2 to the IVEC
if IHSRR is set, and it is set for h_data_storage.
So this checks out to me.
I have checked, to the best of my limited assembler capabilities that
the logic before and after matches. It seems good to me.
On that limited basis,
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
> + beq 2f
> +#endif
> + b .Lno_skip
> +1: mfspr r9,SPRN_SRR0
> + addi r9,r9,4
> + mtspr SPRN_SRR0,r9
> + ld r12,HSTATE_SCRATCH0(r13)
> + ld r9,HSTATE_SCRATCH2(r13)
> + GET_SCRATCH0(r13)
> + RFI_TO_KERNEL
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> +2: mfspr r9,SPRN_HSRR0
> + addi r9,r9,4
> + mtspr SPRN_HSRR0,r9
> + ld r12,HSTATE_SCRATCH0(r13)
> + ld r9,HSTATE_SCRATCH2(r13)
> + GET_SCRATCH0(r13)
> + HRFI_TO_KERNEL
> +#endif
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH] powerpc/perf: prevent mixed EBB and non-EBB events
From: Athira Rajeev @ 2021-03-05 5:50 UTC (permalink / raw)
To: Thadeu Lima de Souza Cascardo; +Cc: Peter Zijlstra, linuxppc-dev, linux-kernel
In-Reply-To: <20210224122116.221120-1-cascardo@canonical.com>
> On 24-Feb-2021, at 5:51 PM, Thadeu Lima de Souza Cascardo <cascardo@canonical.com> wrote:
>
> EBB events must be under exclusive groups, so there is no mix of EBB and
> non-EBB events on the same PMU. This requirement worked fine as perf core
> would not allow other pinned events to be scheduled together with exclusive
> events.
>
> This assumption was broken by commit 1908dc911792 ("perf: Tweak
> perf_event_attr::exclusive semantics").
>
> After that, the test cpu_event_pinned_vs_ebb_test started succeeding after
> read_events, but worse, the task would not have given access to PMC1, so
> when it tried to write to it, it was killed with "illegal instruction".
>
> Preventing mixed EBB and non-EBB events from being add to the same PMU will
> just revert to the previous behavior and the test will succeed.
Hi,
Thanks for checking this. I checked your patch which is fixing “check_excludes” to make
sure all events must agree on EBB. But in the PMU group constraints, we already have check for
EBB events. This is in arch/powerpc/perf/isa207-common.c ( isa207_get_constraint function ).
<<>>
mask |= CNST_EBB_VAL(ebb);
value |= CNST_EBB_MASK;
<<>>
But the above setting for mask and value is interchanged. We actually need to fix here.
Below patch should fix this:
diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c
index e4f577da33d8..8b5eeb6fb2fb 100644
--- a/arch/powerpc/perf/isa207-common.c
+++ b/arch/powerpc/perf/isa207-common.c
@@ -447,8 +447,8 @@ int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp,
* EBB events are pinned & exclusive, so this should never actually
* hit, but we leave it as a fallback in case.
*/
- mask |= CNST_EBB_VAL(ebb);
- value |= CNST_EBB_MASK;
+ mask |= CNST_EBB_MASK;
+ value |= CNST_EBB_VAL(ebb);
*maskp = mask;
*valp = value;
Can you please try with this patch.
Thanks
Athira
>
> Fixes: 1908dc911792 (perf: Tweak perf_event_attr::exclusive semantics)
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
> arch/powerpc/perf/core-book3s.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 43599e671d38..d767f7944f85 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -1010,9 +1010,25 @@ static int check_excludes(struct perf_event **ctrs, unsigned int cflags[],
> int n_prev, int n_new)
> {
> int eu = 0, ek = 0, eh = 0;
> + bool ebb = false;
> int i, n, first;
> struct perf_event *event;
>
> + n = n_prev + n_new;
> + if (n <= 1)
> + return 0;
> +
> + first = 1;
> + for (i = 0; i < n; ++i) {
> + event = ctrs[i];
> + if (first) {
> + ebb = is_ebb_event(event);
> + first = 0;
> + } else if (is_ebb_event(event) != ebb) {
> + return -EAGAIN;
> + }
> + }
> +
> /*
> * If the PMU we're on supports per event exclude settings then we
> * don't need to do any of this logic. NB. This assumes no PMU has both
> @@ -1021,10 +1037,6 @@ static int check_excludes(struct perf_event **ctrs, unsigned int cflags[],
> if (ppmu->flags & PPMU_ARCH_207S)
> return 0;
>
> - n = n_prev + n_new;
> - if (n <= 1)
> - return 0;
> -
> first = 1;
> for (i = 0; i < n; ++i) {
> if (cflags[i] & PPMU_LIMITED_PMC_OK) {
> --
> 2.27.0
>
^ permalink raw reply related
* Re: [PATCH] ibmvnic: remove excessive irqsave
From: Christophe Leroy @ 2021-03-05 5:49 UTC (permalink / raw)
To: angkery, mpe, benh, paulus, drt, ljp, sukadev, davem, kuba
Cc: netdev, linuxppc-dev, linux-kernel, Junlin Yang
In-Reply-To: <20210305014350.1460-1-angkery@163.com>
Le 05/03/2021 à 02:43, angkery a écrit :
> From: Junlin Yang <yangjunlin@yulong.com>
>
> ibmvnic_remove locks multiple spinlocks while disabling interrupts:
> spin_lock_irqsave(&adapter->state_lock, flags);
> spin_lock_irqsave(&adapter->rwi_lock, flags);
>
> there is no need for the second irqsave,since interrupts are disabled
> at that point, so remove the second irqsave:
The problème is not that there is no need. The problem is a lot more serious:
As reported by coccinella, the second _irqsave() overwrites the value saved in 'flags' by the first
_irqsave, therefore when the second _irqrestore comes, the value in 'flags' is not valid, the value
saved by the first _irqsave has been lost. This likely leads to IRQs remaining disabled, which is
_THE_ problem really.
> spin_lock_irqsave(&adapter->state_lock, flags);
> spin_lock(&adapter->rwi_lock);
>
> Generated by: ./scripts/coccinelle/locks/flags.cocci
> ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
> ERROR: nested lock+irqsave that reuses flags from line 5404.
>
> Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
> ---
> drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 2464c8a..a52668d 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev *dev)
> * after setting state, so __ibmvnic_reset() which is called
> * from the flush_work() below, can make progress.
> */
> - spin_lock_irqsave(&adapter->rwi_lock, flags);
> + spin_lock(&adapter->rwi_lock);
> adapter->state = VNIC_REMOVING;
> - spin_unlock_irqrestore(&adapter->rwi_lock, flags);
> + spin_unlock(&adapter->rwi_lock);
>
> spin_unlock_irqrestore(&adapter->state_lock, flags);
>
>
^ permalink raw reply
* Re: [PATCH v2 06/37] KVM: PPC: Book3S 64: move KVM interrupt entry to a common entry point
From: Daniel Axtens @ 2021-03-05 5:03 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin, Fabiano Rosas
In-Reply-To: <20210225134652.2127648-7-npiggin@gmail.com>
Hi Nick,
> Rather than bifurcate the call depending on whether or not HV is
> possible, and have the HV entry test for PR, just make a single
> common point which does the demultiplexing. This makes it simpler
> to add another type of exit handler.
>
> Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>
I checked this against the RFC which I was happy with and there are no
changes that I am concerned about. The expanded comment is also nice.
As such:
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kernel/exceptions-64s.S | 8 +-----
> arch/powerpc/kvm/Makefile | 3 +++
> arch/powerpc/kvm/book3s_64_entry.S | 35 +++++++++++++++++++++++++
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 11 ++------
> 4 files changed, 41 insertions(+), 16 deletions(-)
> create mode 100644 arch/powerpc/kvm/book3s_64_entry.S
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 0097e0676ed7..ba13d749d203 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -208,7 +208,6 @@ do_define_int n
> .endm
>
> #ifdef CONFIG_KVM_BOOK3S_64_HANDLER
> -#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> /*
> * All interrupts which set HSRR registers, as well as SRESET and MCE and
> * syscall when invoked with "sc 1" switch to MSR[HV]=1 (HVMODE) to be taken,
> @@ -238,13 +237,8 @@ do_define_int n
>
> /*
> * If an interrupt is taken while a guest is running, it is immediately routed
> - * to KVM to handle. If both HV and PR KVM arepossible, KVM interrupts go first
> - * to kvmppc_interrupt_hv, which handles the PR guest case.
> + * to KVM to handle.
> */
> -#define kvmppc_interrupt kvmppc_interrupt_hv
> -#else
> -#define kvmppc_interrupt kvmppc_interrupt_pr
> -#endif
>
> .macro KVMTEST name
> lbz r10,HSTATE_IN_GUEST(r13)
> diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
> index 2bfeaa13befb..cdd119028f64 100644
> --- a/arch/powerpc/kvm/Makefile
> +++ b/arch/powerpc/kvm/Makefile
> @@ -59,6 +59,9 @@ kvm-pr-y := \
> kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) += \
> tm.o
>
> +kvm-book3s_64-builtin-objs-y += \
> + book3s_64_entry.o
> +
> ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
> kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) += \
> book3s_rmhandlers.o
> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
> new file mode 100644
> index 000000000000..e9a6a8fbb164
> --- /dev/null
> +++ b/arch/powerpc/kvm/book3s_64_entry.S
> @@ -0,0 +1,35 @@
> +#include <asm/asm-offsets.h>
> +#include <asm/cache.h>
> +#include <asm/kvm_asm.h>
> +#include <asm/kvm_book3s_asm.h>
> +#include <asm/ppc_asm.h>
> +#include <asm/reg.h>
> +
> +/*
> + * This is branched to from interrupt handlers in exception-64s.S which set
> + * IKVM_REAL or IKVM_VIRT, if HSTATE_IN_GUEST was found to be non-zero.
> + */
> +.global kvmppc_interrupt
> +.balign IFETCH_ALIGN_BYTES
> +kvmppc_interrupt:
> + /*
> + * Register contents:
> + * R12 = (guest CR << 32) | interrupt vector
> + * R13 = PACA
> + * guest R12 saved in shadow VCPU SCRATCH0
> + * guest R13 saved in SPRN_SCRATCH0
> + */
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> + std r9,HSTATE_SCRATCH2(r13)
> + lbz r9,HSTATE_IN_GUEST(r13)
> + cmpwi r9,KVM_GUEST_MODE_HOST_HV
> + beq kvmppc_bad_host_intr
> +#ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
> + cmpwi r9,KVM_GUEST_MODE_GUEST
> + ld r9,HSTATE_SCRATCH2(r13)
> + beq kvmppc_interrupt_pr
> +#endif
> + b kvmppc_interrupt_hv
> +#else
> + b kvmppc_interrupt_pr
> +#endif
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index 5e634db4809b..f976efb7e4a9 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -1269,16 +1269,8 @@ kvmppc_interrupt_hv:
> * R13 = PACA
> * guest R12 saved in shadow VCPU SCRATCH0
> * guest R13 saved in SPRN_SCRATCH0
> + * guest R9 saved in HSTATE_SCRATCH2
> */
> - std r9, HSTATE_SCRATCH2(r13)
> - lbz r9, HSTATE_IN_GUEST(r13)
> - cmpwi r9, KVM_GUEST_MODE_HOST_HV
> - beq kvmppc_bad_host_intr
> -#ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
> - cmpwi r9, KVM_GUEST_MODE_GUEST
> - ld r9, HSTATE_SCRATCH2(r13)
> - beq kvmppc_interrupt_pr
> -#endif
> /* We're now back in the host but in guest MMU context */
> li r9, KVM_GUEST_MODE_HOST_HV
> stb r9, HSTATE_IN_GUEST(r13)
> @@ -3280,6 +3272,7 @@ END_FTR_SECTION_IFCLR(CPU_FTR_P9_TM_HV_ASSIST)
> * cfar is saved in HSTATE_CFAR(r13)
> * ppr is saved in HSTATE_PPR(r13)
> */
> +.global kvmppc_bad_host_intr
> kvmppc_bad_host_intr:
> /*
> * Switch to the emergency stack, but start half-way down in
> --
> 2.23.0
^ permalink raw reply
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Michael Ellerman @ 2021-03-05 5:01 UTC (permalink / raw)
To: Marco Elver, Christophe Leroy
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <YEDXJ5JNkgvDFehc@elver.google.com>
Marco Elver <elver@google.com> writes:
> On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
>> Le 04/03/2021 à 12:31, Marco Elver a écrit :
>> > On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
>> > <christophe.leroy@csgroup.eu> wrote:
>> > > Le 03/03/2021 à 11:56, Marco Elver a écrit :
>> > > >
>> > > > Somewhat tangentially, I also note that e.g. show_regs(regs) (which
>> > > > was printed along the KFENCE report above) didn't include the top
>> > > > frame in the "Call Trace", so this assumption is definitely not
>> > > > isolated to KFENCE.
>> > > >
>> > >
>> > > Now, I have tested PPC64 (with the patch I sent yesterday to modify save_stack_trace_regs()
>> > > applied), and I get many failures. Any idea ?
>> > >
>> > > [ 17.653751][ T58] ==================================================================
>> > > [ 17.654379][ T58] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530
>> > > [ 17.654379][ T58]
>> > > [ 17.654831][ T58] Invalid free of 0xc00000003c9c0000 (in kfence-#77):
>> > > [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
>> > > [ 17.655775][ T58] .__slab_free+0x320/0x5a0
>> > > [ 17.656039][ T58] .test_double_free+0xe0/0x198
>> > > [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
>> > > [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
>> > > [ 17.657161][ T58] .kthread+0x18c/0x1a0
>> > > [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
>> > > [ 17.659869][ T58]
> [...]
>> >
>> > Looks like something is prepending '.' to function names. We expect
>> > the function name to appear as-is, e.g. "kfence_guarded_free",
>> > "test_double_free", etc.
>> >
>> > Is there something special on ppc64, where the '.' is some convention?
>> >
>>
>> I think so, see https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
>>
>> Also see commit https://github.com/linuxppc/linux/commit/02424d896
>
> Thanks -- could you try the below patch? You'll need to define
> ARCH_FUNC_PREFIX accordingly.
>
> We think, since there are only very few architectures that add a prefix,
> requiring <asm/kfence.h> to define something like ARCH_FUNC_PREFIX is
> the simplest option. Let me know if this works for you.
>
> There an alternative option, which is to dynamically figure out the
> prefix, but if this simpler option is fine with you, we'd prefer it.
We have rediscovered this problem in basically every tracing / debugging
feature added in the last 20 years :)
I think the simplest solution is the one tools/perf/util/symbol.c uses,
which is to just skip a leading '.'.
Does that work?
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index ab83d5a59bb1..67b49dc54b38 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
for (skipnr = 0; skipnr < num_entries; skipnr++) {
int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
+ if (buf[0] == '.')
+ buf++;
+
if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf, "__kfence_") ||
!strncmp(buf, "__slab_free", len)) {
/*
cheers
^ permalink raw reply related
* Re: [PATCH v2 05/37] KVM: PPC: Book3S HV: Ensure MSR[ME] is always set in guest MSR
From: Daniel Axtens @ 2021-03-05 4:50 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Fabiano Rosas
In-Reply-To: <1614383549.rxe6rxw7w8.astroid@bobo.none>
>> This 'if' is technically redundant but you mention a future patch warning
>> on !(msr & MSR_ME) so I'm holding off on any judgement about the 'if' until
>> I get to that patch :)
>
> That's true. The warning is actually further down when we're setting up
> the msr to run in guest mode. At this point the MSR I think comes from
> qemu (and arguably the guest setup code shouldn't need to know about HV
> specific MSR bits) so a warning here wouldn't be appropriate.
>
> I could remove the if, although the compiler might already do that.
Yes, I think the compiler is almost certainly going to remove that.
I'd probably not include the 'if' statement even though it probably gets
removed by the compiler but I think that's more a matter of taste than
anything else.
Kind regards,
Daniel
>
>>
>> The patch seems sane to me, I agree that we don't want guests running with
>> MSR_ME=0 and kvmppc_set_msr_hv already ensures that the transactional state is
>> sane so this is another sanity-enforcement in the same sort of vein.
>>
>> All up:
>> Reviewed-by: Daniel Axtens <dja@axtens.net>
>
> Thanks,
> Nick
^ permalink raw reply
* Re: [PATCH v2 01/37] KVM: PPC: Book3S 64: remove unused kvmppc_h_protect argument
From: Daniel Axtens @ 2021-03-05 4:45 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <1614383256.cikqwycx8o.astroid@bobo.none>
Hi Nick,
>> ERROR: code indent should use tabs where possible
>> #25: FILE: arch/powerpc/include/asm/kvm_ppc.h:770:
>> + unsigned long pte_index, unsigned long avpn);$
>>
>> WARNING: please, no spaces at the start of a line
>> #25: FILE: arch/powerpc/include/asm/kvm_ppc.h:770:
>> + unsigned long pte_index, unsigned long avpn);$
>
> All the declarations are using the same style in this file so I think
> I'll leave it for someone to do a cleanup patch on. Okay?
Huh, right you are. In that case:
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
^ permalink raw reply
* Re: [PATCH] ibmvnic: remove excessive irqsave
From: Sukadev Bhattiprolu @ 2021-03-05 4:19 UTC (permalink / raw)
To: angkery
Cc: Junlin Yang, linux-kernel, kuba, netdev, ljp, drt, paulus,
linuxppc-dev, davem
In-Reply-To: <20210305014350.1460-1-angkery@163.com>
angkery [angkery@163.com] wrote:
> From: Junlin Yang <yangjunlin@yulong.com>
>
> ibmvnic_remove locks multiple spinlocks while disabling interrupts:
> spin_lock_irqsave(&adapter->state_lock, flags);
> spin_lock_irqsave(&adapter->rwi_lock, flags);
>
> there is no need for the second irqsave,since interrupts are disabled
> at that point, so remove the second irqsave:
> spin_lock_irqsave(&adapter->state_lock, flags);
> spin_lock(&adapter->rwi_lock);
>
> Generated by: ./scripts/coccinelle/locks/flags.cocci
> ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
> ERROR: nested lock+irqsave that reuses flags from line 5404.
>
Thanks. Please add
Fixes: 4a41c421f367 ("ibmvnic: serialize access to work queue on remove")
> Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
Reviewed-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
^ permalink raw reply
* [PATCH V3] mm: Generalize HUGETLB_PAGE_SIZE_VARIABLE
From: Anshuman Khandual @ 2021-03-05 3:28 UTC (permalink / raw)
To: linux-mm
Cc: linux-ia64, Anshuman Khandual, linux-kernel, Paul Mackerras,
Andrew Morton, linuxppc-dev, Christoph Hellwig
HUGETLB_PAGE_SIZE_VARIABLE need not be defined for each individual
platform subscribing it. Instead just make it generic.
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: linux-ia64@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
This change was originally suggested in an earilier discussion. This
applies on v5.12-rc1 and has been build tested on all applicable
platforms i.e ia64 and powerpc.
https://patchwork.kernel.org/project/linux-mm/patch/1613024531-19040-3-git-send-email-anshuman.khandual@arm.com/
Changes in V3:
- Dropped the bool desciption that enabled user selection
- Dropped the dependency on HUGETLB_PAGE for HUGETLB_PAGE_SIZE_VARIABLE
Changes in V2:
https://patchwork.kernel.org/project/linux-mm/patch/1614661987-23881-1-git-send-email-anshuman.khandual@arm.com/
- Added a description for HUGETLB_PAGE_SIZE_VARIABLE
- Added HUGETLB_PAGE dependency while selecting HUGETLB_PAGE_SIZE_VARIABLE
Changes in V1:
https://patchwork.kernel.org/project/linux-mm/patch/1614577853-7452-1-git-send-email-anshuman.khandual@arm.com/
arch/ia64/Kconfig | 6 +-----
arch/powerpc/Kconfig | 6 +-----
mm/Kconfig | 7 +++++++
3 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index 2ad7a8d29fcc..dccf5bfebf48 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -32,6 +32,7 @@ config IA64
select TTY
select HAVE_ARCH_TRACEHOOK
select HAVE_VIRT_CPU_ACCOUNTING
+ select HUGETLB_PAGE_SIZE_VARIABLE if HUGETLB_PAGE
select VIRT_TO_BUS
select GENERIC_IRQ_PROBE
select GENERIC_PENDING_IRQ if SMP
@@ -82,11 +83,6 @@ config STACKTRACE_SUPPORT
config GENERIC_LOCKBREAK
def_bool n
-config HUGETLB_PAGE_SIZE_VARIABLE
- bool
- depends on HUGETLB_PAGE
- default y
-
config GENERIC_CALIBRATE_DELAY
bool
default y
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 386ae12d8523..11fea95a1f2c 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -231,6 +231,7 @@ config PPC
select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !HAVE_HARDLOCKUP_DETECTOR_ARCH
select HAVE_PERF_REGS
select HAVE_PERF_USER_STACK_DUMP
+ select HUGETLB_PAGE_SIZE_VARIABLE if PPC_BOOK3S_64 && HUGETLB_PAGE
select MMU_GATHER_RCU_TABLE_FREE
select MMU_GATHER_PAGE_SIZE
select HAVE_REGS_AND_STACK_ACCESS_API
@@ -415,11 +416,6 @@ config HIGHMEM
source "kernel/Kconfig.hz"
-config HUGETLB_PAGE_SIZE_VARIABLE
- bool
- depends on HUGETLB_PAGE && PPC_BOOK3S_64
- default y
-
config MATH_EMULATION
bool "Math emulation"
depends on 4xx || PPC_8xx || PPC_MPC832x || BOOKE
diff --git a/mm/Kconfig b/mm/Kconfig
index 24c045b24b95..4413a69e7850 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -274,6 +274,13 @@ config ARCH_ENABLE_HUGEPAGE_MIGRATION
config ARCH_ENABLE_THP_MIGRATION
bool
+config HUGETLB_PAGE_SIZE_VARIABLE
+ def_bool n
+ help
+ Allows the pageblock_order value to be dynamic instead of just standard
+ HUGETLB_PAGE_ORDER when there are multiple HugeTLB page sizes available
+ on a platform.
+
config CONTIG_ALLOC
def_bool (MEMORY_ISOLATION && COMPACTION) || CMA
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 7/7] ASoC: dt-bindings: imx-rpmsg: Add binding doc for rpmsg machine driver
From: Shengjiu Wang @ 2021-03-05 2:55 UTC (permalink / raw)
To: Rob Herring
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, linux-kernel, Nicolin Chen,
Mark Brown, linuxppc-dev
In-Reply-To: <CAL_JsqK1uc82hfdE4yj0ye-D6vygiqWkDVW96NOb-8kEFVqHMg@mail.gmail.com>
Hi
On Fri, Mar 5, 2021 at 4:05 AM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Feb 18, 2021 at 1:23 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> >
> > On Thu, Feb 11, 2021 at 6:18 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Sun, Feb 07, 2021 at 06:23:55PM +0800, Shengjiu Wang wrote:
> > > > Imx-rpmsg is a new added machine driver for supporting audio on Cortex-M
> > > > core. The Cortex-M core will control the audio interface, DMA and audio
> > > > codec, setup the pipeline, the audio driver on Cortex-A core side is just
> > > > to communitcate with M core, it is a virtual sound card and don't touch
> > > > the hardware.
> > >
> > > I don't understand why there are 2 nodes for this other than you happen
> > > to want to split this into 2 Linux drivers. It's 1 h/w thing.
> >
> > This one is for the sound card machine driver. Another one is
> > for the sound card cpu dai driver. so there are 2 nodes.
>
> You are explaining this to me in terms of drivers. Explain it in terms
> of h/w blocks.
>
Yes, there is only 1 h/w block, which is (MU) message unit
As the sound card needs a cpu dai node and sound card node,
so from the driver's perspective, I use two nodes.
Seems It is hard to only use one node for my case.
or do you have any suggestions?
Best regards
Wang shengjiu
^ 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