* [PATCH] powerpc/head_check: use stdout for error messages
From: Randy Dunlap @ 2021-08-15 22:23 UTC (permalink / raw)
To: linux-kernel
Cc: Chen, Rong A, Randy Dunlap, Nicholas Piggin, Paul Mackerras,
linuxppc-dev
Prefer stderr instead of stdout for error messages.
This is a good practice and can help CI error detecting and
reporting (0day in this case).
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: "Chen, Rong A" <rong.a.chen@intel.com>
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
Cc: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/tools/head_check.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
--- linux-next-20210813.orig/arch/powerpc/tools/head_check.sh
+++ linux-next-20210813/arch/powerpc/tools/head_check.sh
@@ -56,9 +56,9 @@ expected_start_head_addr=$vma
start_head_addr=$(cat .tmp_symbols.txt | grep " t start_first_256B$" | cut -d' ' -f1)
if [ "$start_head_addr" != "$expected_start_head_addr" ]; then
- echo "ERROR: head code starts at $start_head_addr, should be $expected_start_head_addr"
- echo "ERROR: try to enable LD_HEAD_STUB_CATCH config option"
- echo "ERROR: see comments in arch/powerpc/tools/head_check.sh"
+ echo "ERROR: head code starts at $start_head_addr, should be $expected_start_head_addr" 1>&2
+ echo "ERROR: try to enable LD_HEAD_STUB_CATCH config option" 1>&2
+ echo "ERROR: see comments in arch/powerpc/tools/head_check.sh" 1>&2
exit 1
fi
@@ -70,9 +70,9 @@ expected_start_text_addr=$(cat .tmp_symb
start_text_addr=$(cat .tmp_symbols.txt | grep " t start_text$" | cut -d' ' -f1)
if [ "$start_text_addr" != "$expected_start_text_addr" ]; then
- echo "ERROR: start_text address is $start_text_addr, should be $expected_start_text_addr"
- echo "ERROR: try to enable LD_HEAD_STUB_CATCH config option"
- echo "ERROR: see comments in arch/powerpc/tools/head_check.sh"
+ echo "ERROR: start_text address is $start_text_addr, should be $expected_start_text_addr" 1>&2
+ echo "ERROR: try to enable LD_HEAD_STUB_CATCH config option" 1>&2
+ echo "ERROR: see comments in arch/powerpc/tools/head_check.sh" 1>&2
exit 1
fi
^ permalink raw reply
* Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.14-5 tag
From: pr-tracker-bot @ 2021-08-15 17:00 UTC (permalink / raw)
To: Michael Ellerman
Cc: Linus Torvalds, linux-kernel, clg, pulehui, ldufour, linuxppc-dev
In-Reply-To: <87tujqlvpo.fsf@mpe.ellerman.id.au>
The pull request you sent on Sun, 15 Aug 2021 22:48:51 +1000:
> https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git tags/powerpc-5.14-5
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/ecf93431963a95c0f475921101bedc0dd62ec96d
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
^ permalink raw reply
* Re: [PATCH v2 03/12] x86/sev: Add an x86 version of prot_guest_has()
From: Borislav Petkov @ 2021-08-15 14:39 UTC (permalink / raw)
To: Tom Lendacky
Cc: Sathyanarayanan Kuppuswamy, linux-efi, Brijesh Singh, kvm,
Peter Zijlstra, Dave Hansen, dri-devel, platform-driver-x86,
linux-s390, Andi Kleen, Joerg Roedel, x86, amd-gfx, Ingo Molnar,
linux-graphics-maintainer, Joerg Roedel, Tianyu Lan,
Andy Lutomirski, Thomas Gleixner, kexec, linux-kernel, iommu,
linux-fsdevel, linuxppc-dev
In-Reply-To: <4710eb91-d054-7b31-5106-09e3e54bba9e@amd.com>
On Sun, Aug 15, 2021 at 08:53:31AM -0500, Tom Lendacky wrote:
> It's not a cross-vendor thing as opposed to a KVM or other hypervisor
> thing where the family doesn't have to be reported as AMD or HYGON.
What would be the use case? A HV starts a guest which is supposed to be
encrypted using the AMD's confidential guest technology but the HV tells
the guest that it is not running on an AMD SVM HV but something else?
Is that even an actual use case?
Or am I way off?
I know we have talked about this in the past but this still sounds
insane.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v2 03/12] x86/sev: Add an x86 version of prot_guest_has()
From: Tom Lendacky @ 2021-08-15 13:53 UTC (permalink / raw)
To: Borislav Petkov
Cc: Sathyanarayanan Kuppuswamy, linux-efi, Brijesh Singh, kvm,
Peter Zijlstra, Dave Hansen, dri-devel, platform-driver-x86,
linux-s390, Andi Kleen, Joerg Roedel, x86, amd-gfx, Ingo Molnar,
linux-graphics-maintainer, Joerg Roedel, Tianyu Lan,
Andy Lutomirski, Thomas Gleixner, kexec, linux-kernel, iommu,
linux-fsdevel, linuxppc-dev
In-Reply-To: <YRgUxyhoqVJ0Kxvt@zn.tnic>
On 8/14/21 2:08 PM, Borislav Petkov wrote:
> On Fri, Aug 13, 2021 at 11:59:22AM -0500, Tom Lendacky wrote:
>> diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h
>> new file mode 100644
>> index 000000000000..51e4eefd9542
>> --- /dev/null
>> +++ b/arch/x86/include/asm/protected_guest.h
>> @@ -0,0 +1,29 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Protected Guest (and Host) Capability checks
>> + *
>> + * Copyright (C) 2021 Advanced Micro Devices, Inc.
>> + *
>> + * Author: Tom Lendacky <thomas.lendacky@amd.com>
>> + */
>> +
>> +#ifndef _X86_PROTECTED_GUEST_H
>> +#define _X86_PROTECTED_GUEST_H
>> +
>> +#include <linux/mem_encrypt.h>
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +static inline bool prot_guest_has(unsigned int attr)
>> +{
>> +#ifdef CONFIG_AMD_MEM_ENCRYPT
>> + if (sme_me_mask)
>> + return amd_prot_guest_has(attr);
>> +#endif
>> +
>> + return false;
>> +}
>> +
>> +#endif /* __ASSEMBLY__ */
>> +
>> +#endif /* _X86_PROTECTED_GUEST_H */
>
> I think this can be simplified more, diff ontop below:
>
> - no need for the ifdeffery as amd_prot_guest_has() has versions for
> both when CONFIG_AMD_MEM_ENCRYPT is set or not.
Ugh, yeah, not sure why I put that in for this version since I have the
static inline for when CONFIG_AMD_MEM_ENCRYPT is not set.
>
> - the sme_me_mask check is pushed there too.
>
> - and since this is vendor-specific, I'm checking the vendor bit. Yeah,
> yeah, cross-vendor but I don't really believe that.
It's not a cross-vendor thing as opposed to a KVM or other hypervisor
thing where the family doesn't have to be reported as AMD or HYGON. That's
why I made the if check be for sme_me_mask. I think that is the safer way
to go.
Thanks,
Tom
>
> ---
> diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h
> index 51e4eefd9542..8541c76d5da4 100644
> --- a/arch/x86/include/asm/protected_guest.h
> +++ b/arch/x86/include/asm/protected_guest.h
> @@ -12,18 +12,13 @@
>
> #include <linux/mem_encrypt.h>
>
> -#ifndef __ASSEMBLY__
> -
> static inline bool prot_guest_has(unsigned int attr)
> {
> -#ifdef CONFIG_AMD_MEM_ENCRYPT
> - if (sme_me_mask)
> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> + boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
> return amd_prot_guest_has(attr);
> -#endif
>
> return false;
> }
>
> -#endif /* __ASSEMBLY__ */
> -
> #endif /* _X86_PROTECTED_GUEST_H */
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index edc67ddf065d..5a0442a6f072 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -392,6 +392,9 @@ bool noinstr sev_es_active(void)
>
> bool amd_prot_guest_has(unsigned int attr)
> {
> + if (!sme_me_mask)
> + return false;
> +
> switch (attr) {
> case PATTR_MEM_ENCRYPT:
> return sme_me_mask != 0;
>
^ permalink raw reply
* [GIT PULL] Please pull powerpc/linux.git powerpc-5.14-5 tag
From: Michael Ellerman @ 2021-08-15 12:48 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, clg, pulehui, ldufour, linuxppc-dev
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Hi Linus,
Please pull some more powerpc fixes for 5.14:
The following changes since commit a88603f4b92ecef9e2359e40bcb99ad399d85dd7:
powerpc/vdso: Don't use r30 to avoid breaking Go lang (2021-07-29 23:13:12 +1000)
are available in the git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git tags/powerpc-5.14-5
for you to fetch changes up to cbc06f051c524dcfe52ef0d1f30647828e226d30:
powerpc/xive: Do not skip CPU-less nodes when creating the IPIs (2021-08-12 22:31:41 +1000)
- ------------------------------------------------------------------
powerpc fixes for 5.14 #5
- Fix crashes coming out of nap on 32-bit Book3s (eg. powerbooks).
- Fix critical and debug interrupts on BookE, seen as crashes when using ptrace.
- Fix an oops when running an SMP kernel on a UP system.
- Update pseries LPAR security flavor after partition migration.
- Fix an oops when using kprobes on BookE.
- Fix oops on 32-bit pmac by not calling do_IRQ() from timer_interrupt().
- Fix softlockups on CPU hotplug into a CPU-less node with xive (P9).
Thanks to: Cédric Le Goater, Christophe Leroy, Finn Thain, Geetika Moolchandani, Laurent
Dufour, Laurent Vivier, Nicholas Piggin, Pu Lehui, Radu Rendec, Srikar Dronamraju, Stan
Johnson.
- ------------------------------------------------------------------
Christophe Leroy (5):
powerpc/32s: Fix napping restore in data storage interrupt (DSI)
powerpc/32: Fix critical and debug interrupts on BOOKE
powerpc/smp: Fix OOPS in topology_init()
powerpc/interrupt: Fix OOPS by not calling do_IRQ() from timer_interrupt()
powerpc/interrupt: Do not call single_step_exception() from other exceptions
Cédric Le Goater (1):
powerpc/xive: Do not skip CPU-less nodes when creating the IPIs
Laurent Dufour (1):
powerpc/pseries: Fix update of LPAR security flavor after LPM
Pu Lehui (1):
powerpc/kprobes: Fix kprobe Oops happens in booke
arch/powerpc/include/asm/interrupt.h | 3 ++
arch/powerpc/include/asm/irq.h | 2 +-
arch/powerpc/include/asm/ptrace.h | 16 +++++++++
arch/powerpc/kernel/asm-offsets.c | 31 ++++++++---------
arch/powerpc/kernel/head_book3s_32.S | 2 +-
arch/powerpc/kernel/head_booke.h | 27 ++-------------
arch/powerpc/kernel/irq.c | 7 +++-
arch/powerpc/kernel/kprobes.c | 3 +-
arch/powerpc/kernel/sysfs.c | 2 +-
arch/powerpc/kernel/time.c | 2 +-
arch/powerpc/kernel/traps.c | 9 +++--
arch/powerpc/platforms/pseries/setup.c | 5 +--
arch/powerpc/sysdev/xive/common.c | 35 ++++++++++++++------
13 files changed, 82 insertions(+), 62 deletions(-)
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEEJFGtCPCthwEv2Y/bUevqPMjhpYAFAmEZDQQACgkQUevqPMjh
pYDVNQ//cV/JSIm1THeqbfjZFrx2iO5hcwgO0lXaRK6Lf4gXGAcQAGr6Co+JjsNy
A5vz3myCKHiTlCe7862Dm7X54FKeJKwhQsdzpWVfw8aeQwxdhkc1Wg4WtgKMT3/j
woN6je4+2/I9LctFgnRMqMV1ode4t91jgjX4wGCxqLYQxPtBXgkZ4jwwIvTh5xR8
DmkYr5ZFaHzVX+wHY/58aT4rtej5f88RXVdR1ClCw9APzgPzSP+t/arZm1B7TRGN
PFetXlHtyOzHcsf5OhfHXWJ4tjUxLsuHccPmnJKJsPAnfNP2fUEZv8fqHqJma+bj
YVG8wzCI4/OTdaoEzmgwdyKXsE0g5GSh0V2ok4DEUaRWrWCbSmsKvLLqW9gVn06K
9iLMne+MoxOmUh4z2ZI2PI+Ko31IEOlaRudsdeJGCYAYAqTRrbBdrrUK55Qjozv0
oGccwMZrqyWwlNfyx4kgD03x1N0zITr1/CstAIssfQilnrlrM6n4vWjc3tvMI1R7
Lgl5Ja8UYkLQsOmH5FD0iqEwmjaJgNuc7+Vt93rWvxk1Pk/s8Kx5dEer+/q0tLku
u7QabZMOd3DsVsgU3oNNNUDGA3RbvhlOj3H2J/RoHftmXg2UG0T4MBIVk9r5N+DP
MOgk1GP1zxLQ2B6FazRG3J5W/Ct9gHTMSFUpWGaz313VWp6gpJ4=
=NuwF
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
From: Michael Ellerman @ 2021-08-15 3:49 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <389962b1b702e3c78d169e59bcfac56282889173.1618331882.git.christophe.leroy@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 24725e50c7b4..34745f239208 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -926,7 +926,7 @@ static void check_section(const char *modname, struct elf_info *elf,
> ".kprobes.text", ".cpuidle.text", ".noinstr.text"
> #define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \
> ".fixup", ".entry.text", ".exception.text", ".text.*", \
> - ".coldtext"
> + ".coldtext .softirqentry.text"
This wasn't working, I updated it to:
".coldtext", ".softirqentry.text"
Which works.
cheers
^ permalink raw reply
* Re: [PATCH] ppc: add "-z notext" flag to disable diagnostic
From: Fāng-ruì Sòng @ 2021-08-14 19:34 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Nick Desaulniers, linux-kernel, Nathan Chancellor,
clang-built-linux, Paul Mackerras, Bill Wendling, linuxppc-dev,
Daniel Axtens
In-Reply-To: <20210814125812.GC1583@gate.crashing.org>
On Sat, Aug 14, 2021 at 5:59 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Fri, Aug 13, 2021 at 01:05:08PM -0700, Fangrui Song wrote:
> > Text relocations are considered very awful by linker developers.
>
> By very few linker developers.
https://groups.google.com/g/generic-abi/c/Ckq19PfLxyk/m/uW29sgkoAgAJ
Ali Bahrami: "My opinion is that no one wants text relocations, but
not labeling them if they do exist doesn't seem right. I find the
presence of DF_TEXTREL very interesting when diagnosing various
issues."
https://gcc.gnu.org/legacy-ml/gcc/2016-04/msg00138.html
( "So why not simply create 'dynamic text relocations' then? Is that
possible with a pure linker change?" )
Cary Coutant: "Ugh. Besides being a bad idea from a performance point
of view, it's not even always possible to do. Depending on the
architecture, a direct reference from an executable to a variable in a
shared library may not have the necessary reach."
binutils-gdb commit "Add linker option: --warn-shared-textrel to
produce warnings when adding a DT_TEXTREL to a shared object."
Nick Clifton
https://www.openwall.com/lists/musl/2020/09/26/3
Szabolcs Nagy: "nice. and gcc passes -z text for static pie code so
that case should not end up with text rels."
Someone wrote "Overall, the overhead of processing text relocations
can cause serious performance degradation." in Solaris' Linker and
Libraries Guide.
Me :)
(I wrote lld/ELF 9.0.0~13.0.0 release notes and filed dozen of GNU
ld/gold bugs/feature requests)
> > binutils 2.35 added --enable-textrel-check={no,warn,error}
> > https://sourceware.org/bugzilla/show_bug.cgi?id=20824
>
> Yes, some people wanted the default to be configurable. So now we have
> a default default that is sane, so most people get to reap the benefits
> of having defaults at all, but we also allow other people to shoot
> themselves (and people who have to deal with them) in the foot.
> "Progress". Changing the defaults should be a one-time event, only done
> when the benefits strongly outweigh the costs. Defaults should never be
> configurable (by the user).
ld.lld has such a non-configurable model and thus caught the issue
(which the patch intends to address).
Future --enable-textrel-check={yes,error} configured GNU ld will be similar.
> > I can imagine that in the future some Linux distributions (especially those
> > focusing on security) will default their binutils to use
> > --enable-textrel-check={no,warn,error}.
>
> How would this be a benefit to security?
https://flameeyes.blog/2016/01/16/textrels-text-relocations-and-their-impact-on-hardening-techniques/
https://github.com/golang/go/issues/9210
Android: "libexample.so has text relocations. This is wasting memory
and prevents security hardening. Please fix."
FWIW I contributed a glibc patch allowing TEXTREL to co-exist with ifunc.
It requires temporary mapping the text segment W^X.
> > In -no-pie links, R_PPC64_ADDR64 relocations are link-time constants.
>
> Where "link" includes dynamic links as well. There are no constants.
"Link-time" usually refers to the processing of the static linker.
Dynamic links can use load-time or run-time.
> > There are no text relocations, therefore no need for -z notext.
>
> This is a choice by the compiler, nothing more. It saves some process
> startup time, and allows slightly more maps to be shared by processes
> that run the same images. But it is a tradeoff, so it might change; and
> of course it is not an ABI requirement.
>
>
> Segher
Text relocations are generally awful.
GNU ld and gold's traditional "add DF_TEXTREL on-demand" behavior made
such user errors easy to make.
I understand that kernels are special applications where we apply
relocations once and many user-space objection can be less of a
concern/ignored.
However, the Linux kernel is already in a position where many linker
options are controlled and thus should specify -z notext to make
the intention explicit, or fix the problems (I think x86-64 is good;
that said, powerpc
has a higher cost using PC-relative instructions so pay the oneshot relocation
time cost probably isn't a bad choice)
--
宋方睿
^ permalink raw reply
* Re: [PATCH v2 03/12] x86/sev: Add an x86 version of prot_guest_has()
From: Borislav Petkov @ 2021-08-14 19:08 UTC (permalink / raw)
To: Tom Lendacky
Cc: Sathyanarayanan Kuppuswamy, linux-efi, Brijesh Singh, kvm,
Peter Zijlstra, Dave Hansen, dri-devel, platform-driver-x86,
linux-s390, Andi Kleen, Joerg Roedel, x86, amd-gfx, Ingo Molnar,
linux-graphics-maintainer, Joerg Roedel, Tianyu Lan,
Andy Lutomirski, Thomas Gleixner, kexec, linux-kernel, iommu,
linux-fsdevel, linuxppc-dev
In-Reply-To: <7d55bac0cf2e73f53816bce3a3097877ed9663f3.1628873970.git.thomas.lendacky@amd.com>
On Fri, Aug 13, 2021 at 11:59:22AM -0500, Tom Lendacky wrote:
> diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h
> new file mode 100644
> index 000000000000..51e4eefd9542
> --- /dev/null
> +++ b/arch/x86/include/asm/protected_guest.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Protected Guest (and Host) Capability checks
> + *
> + * Copyright (C) 2021 Advanced Micro Devices, Inc.
> + *
> + * Author: Tom Lendacky <thomas.lendacky@amd.com>
> + */
> +
> +#ifndef _X86_PROTECTED_GUEST_H
> +#define _X86_PROTECTED_GUEST_H
> +
> +#include <linux/mem_encrypt.h>
> +
> +#ifndef __ASSEMBLY__
> +
> +static inline bool prot_guest_has(unsigned int attr)
> +{
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> + if (sme_me_mask)
> + return amd_prot_guest_has(attr);
> +#endif
> +
> + return false;
> +}
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* _X86_PROTECTED_GUEST_H */
I think this can be simplified more, diff ontop below:
- no need for the ifdeffery as amd_prot_guest_has() has versions for
both when CONFIG_AMD_MEM_ENCRYPT is set or not.
- the sme_me_mask check is pushed there too.
- and since this is vendor-specific, I'm checking the vendor bit. Yeah,
yeah, cross-vendor but I don't really believe that.
---
diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h
index 51e4eefd9542..8541c76d5da4 100644
--- a/arch/x86/include/asm/protected_guest.h
+++ b/arch/x86/include/asm/protected_guest.h
@@ -12,18 +12,13 @@
#include <linux/mem_encrypt.h>
-#ifndef __ASSEMBLY__
-
static inline bool prot_guest_has(unsigned int attr)
{
-#ifdef CONFIG_AMD_MEM_ENCRYPT
- if (sme_me_mask)
+ if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
+ boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
return amd_prot_guest_has(attr);
-#endif
return false;
}
-#endif /* __ASSEMBLY__ */
-
#endif /* _X86_PROTECTED_GUEST_H */
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index edc67ddf065d..5a0442a6f072 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -392,6 +392,9 @@ bool noinstr sev_es_active(void)
bool amd_prot_guest_has(unsigned int attr)
{
+ if (!sme_me_mask)
+ return false;
+
switch (attr) {
case PATTR_MEM_ENCRYPT:
return sme_me_mask != 0;
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply related
* Re: [PATCH v2 02/12] mm: Introduce a function to check for virtualization protection features
From: Tom Lendacky @ 2021-08-14 18:49 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-s390, Sathyanarayanan Kuppuswamy, linux-efi, Brijesh Singh,
kvm, Joerg Roedel, x86, kexec, linux-kernel, amd-gfx,
platform-driver-x86, iommu, Andi Kleen, linux-graphics-maintainer,
dri-devel, Joerg Roedel, linux-fsdevel, Tianyu Lan, linuxppc-dev
In-Reply-To: <YRgMUHqdH60jDB06@zn.tnic>
On 8/14/21 1:32 PM, Borislav Petkov wrote:
> On Fri, Aug 13, 2021 at 11:59:21AM -0500, Tom Lendacky wrote:
>> diff --git a/include/linux/protected_guest.h b/include/linux/protected_guest.h
>> new file mode 100644
>> index 000000000000..43d4dde94793
>> --- /dev/null
>> +++ b/include/linux/protected_guest.h
>> @@ -0,0 +1,35 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Protected Guest (and Host) Capability checks
>> + *
>> + * Copyright (C) 2021 Advanced Micro Devices, Inc.
>> + *
>> + * Author: Tom Lendacky <thomas.lendacky@amd.com>
>> + */
>> +
>> +#ifndef _PROTECTED_GUEST_H
>> +#define _PROTECTED_GUEST_H
>> +
>> +#ifndef __ASSEMBLY__
> ^^^^^^^^^^^^^
>
> Do you really need that guard? It builds fine without it too. Or
> something coming later does need it...?
No, I probably did it out of habit. I can remove it in the next version.
Thanks,
Tom
>
^ permalink raw reply
* Re: [PATCH v2 02/12] mm: Introduce a function to check for virtualization protection features
From: Borislav Petkov @ 2021-08-14 18:32 UTC (permalink / raw)
To: Tom Lendacky
Cc: linux-s390, Sathyanarayanan Kuppuswamy, linux-efi, Brijesh Singh,
kvm, Joerg Roedel, x86, kexec, linux-kernel, amd-gfx,
platform-driver-x86, iommu, Andi Kleen, linux-graphics-maintainer,
dri-devel, Joerg Roedel, linux-fsdevel, Tianyu Lan, linuxppc-dev
In-Reply-To: <482fe51f1671c1cd081039801b03db7ec0036332.1628873970.git.thomas.lendacky@amd.com>
On Fri, Aug 13, 2021 at 11:59:21AM -0500, Tom Lendacky wrote:
> In prep for other protected virtualization technologies, introduce a
> generic helper function, prot_guest_has(), that can be used to check
> for specific protection attributes, like memory encryption. This is
> intended to eliminate having to add multiple technology-specific checks
> to the code (e.g. if (sev_active() || tdx_active())).
>
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> Co-developed-by: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> Co-developed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> arch/Kconfig | 3 +++
> include/linux/protected_guest.h | 35 +++++++++++++++++++++++++++++++++
> 2 files changed, 38 insertions(+)
> create mode 100644 include/linux/protected_guest.h
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 98db63496bab..bd4f60c581f1 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -1231,6 +1231,9 @@ config RELR
> config ARCH_HAS_MEM_ENCRYPT
> bool
>
> +config ARCH_HAS_PROTECTED_GUEST
> + bool
> +
> config HAVE_SPARSE_SYSCALL_NR
> bool
> help
> diff --git a/include/linux/protected_guest.h b/include/linux/protected_guest.h
> new file mode 100644
> index 000000000000..43d4dde94793
> --- /dev/null
> +++ b/include/linux/protected_guest.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Protected Guest (and Host) Capability checks
> + *
> + * Copyright (C) 2021 Advanced Micro Devices, Inc.
> + *
> + * Author: Tom Lendacky <thomas.lendacky@amd.com>
> + */
> +
> +#ifndef _PROTECTED_GUEST_H
> +#define _PROTECTED_GUEST_H
> +
> +#ifndef __ASSEMBLY__
^^^^^^^^^^^^^
Do you really need that guard? It builds fine without it too. Or
something coming later does need it...?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH] crypto: DRBG - select SHA512
From: Borislav Petkov @ 2021-08-14 16:23 UTC (permalink / raw)
To: Herbert Xu; +Cc: Stephan Mueller, sachinp, linuxppc-dev, linux-crypto, lkml
In-Reply-To: <20210716081411.GA2062@gondor.apana.org.au>
On Fri, Jul 16, 2021 at 04:14:12PM +0800, Herbert Xu wrote:
> Stephan Mueller <smueller@chronox.de> wrote:
> > With the swtich to use HMAC(SHA-512) as the default DRBG type, the
> > configuration must now also select SHA-512.
> >
> > Fixes: 9b7b94683a9b "crypto: DRBG - switch to HMAC SHA512 DRBG as default
> > DRBG"
> > Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> > Signed-off-by: Stephan Mueller <smueller@chronox.com>
> > ---
> > crypto/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Patch applied. Thanks.
Is that patch going to Linus anytime soon?
I still see it on latest rc5+:
DRBG: could not allocate digest TFM handle: hmac(sha512)
alg: drbg: Failed to reset rng
alg: drbg: Test 0 failed for drbg_nopr_hmac_sha512
------------[ cut here ]------------
alg: self-tests for drbg_nopr_hmac_sha512 (stdrng) failed (rc=-22)
WARNING: CPU: 3 PID: 76 at crypto/testmgr.c:5652 alg_test.part.0+0x132/0x3c0
Modules linked in:
CPU: 3 PID: 76 Comm: cryptomgr_test Not tainted 5.14.0-rc5+ #1
Hardware name: LENOVO 2320CTO/2320CTO, BIOS G2ET86WW (2.06 ) 11/13/2012
RIP: 0010:alg_test.part.0+0x132/0x3c0
Code: c0 74 2e 80 3d 7f 61 ad 02 00 0f 85 c0 64 5f 00 44 89 c1 4c 89 f2 4c 89 ee 44 89 44 24 04 48 c7 c7 f8 0a 11 82 e8 8c 57 5e 00 <0f> 0b 44 8b 44 24 04 48 8b 84 24 98 00 00 00 65 48 2b 04 25 28 00
RSP: 0000:ffffc9000078fe38 EFLAGS: 00010292
RAX: 0000000000000042 RBX: 00000000ffffffff RCX: 0000000000000000
RDX: 0000000000000001 RSI: ffffffff810f520f RDI: ffffffff810f520f
RBP: 0000000000000053 R08: 0000000000000001 R09: 0000000000000001
R10: ffff888219df9000 R11: 3fffffffffffffff R12: 0000000000000053
R13: ffff888100c0ee00 R14: ffff888100c0ee80 R15: 00000000000014c0
FS: 0000000000000000(0000) GS:ffff888211f80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000002412001 CR4: 00000000001706e0
Call Trace:
? lock_is_held_type+0xd5/0x130
? find_held_lock+0x2b/0x80
? preempt_count_sub+0x9b/0xd0
? crypto_acomp_scomp_free_ctx+0x30/0x30
cryptomgr_test+0x27/0x50
kthread+0x144/0x170
? set_kthread_struct+0x40/0x40
ret_from_fork+0x22/0x30
irq event stamp: 411
hardirqs last enabled at (419): [<ffffffff810f6972>] console_unlock+0x332/0x570
hardirqs last disabled at (426): [<ffffffff810f6a1f>] console_unlock+0x3df/0x570
softirqs last enabled at (234): [<ffffffff81c00329>] __do_softirq+0x329/0x496
softirqs last disabled at (151): [<ffffffff8108248d>] irq_exit_rcu+0xdd/0x130
---[ end trace edfdfd51982deb2d ]---
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v2 01/12] x86/ioremap: Selectively build arch override encryption functions
From: Borislav Petkov @ 2021-08-14 15:25 UTC (permalink / raw)
To: Tom Lendacky
Cc: Sathyanarayanan Kuppuswamy, linux-efi, Brijesh Singh, kvm,
Peter Zijlstra, Dave Hansen, dri-devel, platform-driver-x86,
linux-s390, Andi Kleen, Joerg Roedel, x86, amd-gfx, Ingo Molnar,
linux-graphics-maintainer, Tianyu Lan, Andy Lutomirski,
Thomas Gleixner, kexec, linux-kernel, iommu, linux-fsdevel,
linuxppc-dev
In-Reply-To: <a4338245609a6be63b162e3516d3f6614db782a4.1628873970.git.thomas.lendacky@amd.com>
On Fri, Aug 13, 2021 at 11:59:20AM -0500, Tom Lendacky wrote:
> In prep for other uses of the prot_guest_has() function besides AMD's
> memory encryption support, selectively build the AMD memory encryption
> architecture override functions only when CONFIG_AMD_MEM_ENCRYPT=y. These
> functions are:
> - early_memremap_pgprot_adjust()
> - arch_memremap_can_ram_remap()
>
> Additionally, routines that are only invoked by these architecture
> override functions can also be conditionally built. These functions are:
> - memremap_should_map_decrypted()
> - memremap_is_efi_data()
> - memremap_is_setup_data()
> - early_memremap_is_setup_data()
>
> And finally, phys_mem_access_encrypted() is conditionally built as well,
> but requires a static inline version of it when CONFIG_AMD_MEM_ENCRYPT is
> not set.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> arch/x86/include/asm/io.h | 8 ++++++++
> arch/x86/mm/ioremap.c | 2 +-
> 2 files changed, 9 insertions(+), 1 deletion(-)
LGTM.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH] ppc: add "-z notext" flag to disable diagnostic
From: Segher Boessenkool @ 2021-08-14 12:58 UTC (permalink / raw)
To: Fangrui Song
Cc: Nick Desaulniers, linux-kernel, Nathan Chancellor,
clang-built-linux, Paul Mackerras, Bill Wendling, linuxppc-dev,
Daniel Axtens
In-Reply-To: <20210813200508.7bqehxgd6ruerds5@google.com>
On Fri, Aug 13, 2021 at 01:05:08PM -0700, Fangrui Song wrote:
> Text relocations are considered very awful by linker developers.
By very few linker developers.
> binutils 2.35 added --enable-textrel-check={no,warn,error}
> https://sourceware.org/bugzilla/show_bug.cgi?id=20824
Yes, some people wanted the default to be configurable. So now we have
a default default that is sane, so most people get to reap the benefits
of having defaults at all, but we also allow other people to shoot
themselves (and people who have to deal with them) in the foot.
"Progress". Changing the defaults should be a one-time event, only done
when the benefits strongly outweigh the costs. Defaults should never be
configurable (by the user).
> I can imagine that in the future some Linux distributions (especially those
> focusing on security) will default their binutils to use
> --enable-textrel-check={no,warn,error}.
How would this be a benefit to security?
> In -no-pie links, R_PPC64_ADDR64 relocations are link-time constants.
Where "link" includes dynamic links as well. There are no constants.
> There are no text relocations, therefore no need for -z notext.
This is a choice by the compiler, nothing more. It saves some process
startup time, and allows slightly more maps to be shared by processes
that run the same images. But it is a tradeoff, so it might change; and
of course it is not an ABI requirement.
Segher
^ permalink raw reply
* Re: [PATCH v2 2/2] powerpc/perf: Return regs->nip as instruction pointer value when SIAR is 0
From: Michael Ellerman @ 2021-08-14 12:44 UTC (permalink / raw)
To: Christophe Leroy, Kajol Jain, linuxppc-dev; +Cc: atrajeev, maddy, rnsastry
In-Reply-To: <c6110aa1-90e2-77aa-1ab5-355975037227@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 13/08/2021 à 10:24, Kajol Jain a écrit :
>> Incase of random sampling, there can be scenarios where SIAR is not
>> latching sample address and results in 0 value. Since current code
>> directly returning the siar value, we could see multiple instruction
>> pointer values as 0 in perf report.
Can you please give more detail on that? What scenarios? On what CPUs?
>> Patch resolves this issue by adding a ternary condition to return
>> regs->nip incase SIAR is 0.
>
> Your description seems rather similar to
> https://github.com/linuxppc/linux/commit/2ca13a4cc56c920a6c9fc8ee45d02bccacd7f46c
>
> Does it mean that the problem occurs on more than the power10 DD1 ?
>
> In that case, can the solution be common instead of doing something for power10 DD1 and something
> for others ?
Agreed.
This change would seem to make that P10 DD1 logic superfluous.
Also we already have a fallback to regs->nip in the else case of the if,
so we should just use that rather than adding a ternary condition.
eg.
if (use_siar && siar_valid(regs) && siar)
return siar + perf_ip_adjust(regs);
else if (use_siar)
return 0; // no valid instruction pointer
else
return regs->nip;
I'm also not sure why we have that return 0 case, I can't think of why
we'd ever want to do that rather than using nip. So maybe we should do
another patch to drop that case.
cheers
^ permalink raw reply
* Re: [PATCH v2 1/2] powerpc/perf: Use stack siar instead of mfspr
From: Michael Ellerman @ 2021-08-14 12:30 UTC (permalink / raw)
To: Christophe Leroy, kajoljain, linuxppc-dev; +Cc: atrajeev, maddy, rnsastry
In-Reply-To: <9662dfbf-d163-c313-745e-aeda0f638e98@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 13/08/2021 à 10:29, kajoljain a écrit :
>>
>> On 8/13/21 1:54 PM, Kajol Jain wrote:
>>> Minor optimization in the 'perf_instruction_pointer' function code by
>>> making use of stack siar instead of mfspr.
>>>
>>> Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
>>> into perf_read_regs")
>>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>>
>> Please ignore this patch-set as I mentioned wrong version number. I will resend
>> this patch-set again with correct version. Sorry for the confusion.
>
> I fear you are creating even more confusion by sending a v1 after sending a v2 ...
Yeah in future just reply to the v2 saying "oops I sent v2 instead of
v1" and leave it at that.
cheers
^ permalink raw reply
* Re: [PATCH v2 2/2] powerpc/perf: Return regs->nip as instruction pointer value when SIAR is 0
From: Michael Ellerman @ 2021-08-14 12:25 UTC (permalink / raw)
To: Christophe Leroy, Kajol Jain, linuxppc-dev; +Cc: atrajeev, maddy, rnsastry
In-Reply-To: <95fc0f78-51d0-82f6-c9d6-de101fec445c@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 13/08/2021 à 10:24, Kajol Jain a écrit :
>> Incase of random sampling, there can be scenarios where SIAR is not
>> latching sample address and results in 0 value. Since current code
>> directly returning the siar value, we could see multiple instruction
>> pointer values as 0 in perf report.
>> Patch resolves this issue by adding a ternary condition to return
>> regs->nip incase SIAR is 0.
>>
>> Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
>> into perf_read_regs")
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>> arch/powerpc/perf/core-book3s.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index 1b464aad29c4..aeecaaf6810f 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -2260,7 +2260,7 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
>> else
>> return regs->nip;
>> } else if (use_siar && siar_valid(regs))
>> - return siar + perf_ip_adjust(regs);
>> + return siar ? siar + perf_ip_adjust(regs) : regs->nip;
>
> Why bother about returning SIAR at all if regs->nip is ok ? Why not just return regs->nip all the time ?
Same answer as last time :)
https://lore.kernel.org/linuxppc-dev/87r1prxd9e.fsf@mpe.ellerman.id.au/
ie. SIAR can point into interrupts-off code, whereas regs->nip will
point to where we re-enabled interrupts.
cheers
^ permalink raw reply
* Re: [PATCH] ppc: add "-z notext" flag to disable diagnostic
From: Michael Ellerman @ 2021-08-14 11:59 UTC (permalink / raw)
To: Bill Wendling, Daniel Axtens, Fangrui Song
Cc: Nick Desaulniers, LKML, Nathan Chancellor, clang-built-linux,
Paul Mackerras, linuxppc-dev
In-Reply-To: <CAGG=3QUz2LNgC8Hn6rU68ejjv4=J9Uidef0oH9A7=sKTs+vf7g@mail.gmail.com>
Bill Wendling <morbo@google.com> writes:
> On Fri, Aug 13, 2021 at 7:13 AM Daniel Axtens <dja@axtens.net> wrote:
>> Bill Wendling <morbo@google.com> writes:
...
>> > diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
>> > index 6505d66f1193..17a9fbf9b789 100644
>> > --- a/arch/powerpc/Makefile
>> > +++ b/arch/powerpc/Makefile
>> > @@ -122,6 +122,7 @@ endif
>> >
>> > LDFLAGS_vmlinux-y := -Bstatic
>> > LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
>> > +LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) += -z notext
...
>
> Unrelated question: Should the "-pie" flag be added with "+= -pie"
> (note the plus sign)?
I noticed that too.
It's been like that since the original relocatable support was added in
2008, commit 549e8152de80 ("powerpc: Make the 64-bit kernel as a
position-independent executable"), which did:
-LDFLAGS_vmlinux := -Bstatic
+LDFLAGS_vmlinux-yy := -Bstatic
+LDFLAGS_vmlinux-$(CONFIG_PPC64)$(CONFIG_RELOCATABLE) := -pie
+LDFLAGS_vmlinux := $(LDFLAGS_vmlinux-yy)
There's no mention of those flags in the change log. But the way it's
written suggests the intention was to not pass -Bstatic for relocatable
builds, otherwise it could have been more simply:
+LDFLAGS_vmlinux-$(CONFIG_PPC64)$(CONFIG_RELOCATABLE) := -pie
+LDFLAGS_vmlinux := -Bstatic $(LDFLAGS_vmlinux-yy)
So I think it was deliberate to not use +=, but whether that's actually
correct I can't say. Maybe in the past -Bstatic and -pie were
incompatible?
cheers
^ permalink raw reply
* Re: [PATCH] ppc: add "-z notext" flag to disable diagnostic
From: Segher Boessenkool @ 2021-08-14 11:01 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Fangrui Song, LKML, Nathan Chancellor, clang-built-linux,
Paul Mackerras, Bill Wendling, linuxppc-dev, Daniel Axtens
In-Reply-To: <CAKwvOd=rN9s5YBtt-AMnaqXhYCsAT=6yp29_oomRvTaev6Q6zw@mail.gmail.com>
On Fri, Aug 13, 2021 at 11:59:21AM -0700, Nick Desaulniers wrote:
> Or we can dig through why there are relocations in read only sections,
> fix those, then enable `-z text` for all linkers. My recommendation
> would be get the thing building, then go digging time permitting.
It is not always a bug. You can get much more efficient code if you
have text relocations than if you don't. This "read-only" memory is
perfectly writable until after relocation, a la relro.
But you no doubt will find some non-optimalities (or even straight out
bugs) if you build with -ztext sometimes :-)
Segher
^ permalink raw reply
* [powerpc:merge] BUILD SUCCESS 01dc10da827c1725c0f5491c78d700a4478aae08
From: kernel test robot @ 2021-08-14 10:00 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git merge
branch HEAD: 01dc10da827c1725c0f5491c78d700a4478aae08 Automatic merge of 'fixes' into merge (2021-08-12 23:46)
elapsed time: 2610m
configs tested: 231
configs skipped: 4
The following configs have been built successfully.
More configs may be tested in the coming days.
gcc tested configs:
arm defconfig
arm64 allyesconfig
arm64 defconfig
arm allyesconfig
arm allmodconfig
i386 randconfig-c001-20210814
i386 randconfig-c001-20210812
i386 randconfig-c001-20210813
powerpc mpc8540_ads_defconfig
ia64 defconfig
arm imx_v6_v7_defconfig
mips rs90_defconfig
parisc generic-32bit_defconfig
arm am200epdkit_defconfig
openrisc or1klitex_defconfig
m68k m5407c3_defconfig
powerpc klondike_defconfig
mips loongson1c_defconfig
arm ep93xx_defconfig
arm iop32x_defconfig
powerpc tqm8xx_defconfig
arm lpc18xx_defconfig
sh edosk7760_defconfig
x86_64 allnoconfig
sh shx3_defconfig
mips maltasmvp_eva_defconfig
mips bigsur_defconfig
powerpc64 defconfig
powerpc cell_defconfig
arm davinci_all_defconfig
mips workpad_defconfig
arm omap2plus_defconfig
powerpc pq2fads_defconfig
h8300 alldefconfig
i386 alldefconfig
mips fuloong2e_defconfig
mips mtx1_defconfig
arm u8500_defconfig
nios2 3c120_defconfig
ia64 generic_defconfig
powerpc tqm8555_defconfig
powerpc acadia_defconfig
arm simpad_defconfig
mips cavium_octeon_defconfig
nds32 defconfig
parisc alldefconfig
arm tct_hammer_defconfig
powerpc obs600_defconfig
powerpc makalu_defconfig
arm shannon_defconfig
arm clps711x_defconfig
powerpc ppc64e_defconfig
sh ul2_defconfig
arm orion5x_defconfig
xtensa allyesconfig
arm imx_v4_v5_defconfig
powerpc ep88xc_defconfig
powerpc rainier_defconfig
sh shmin_defconfig
h8300 defconfig
powerpc mpc834x_itxgp_defconfig
mips maltaup_defconfig
mips ip22_defconfig
sh se7721_defconfig
m68k m5208evb_defconfig
x86_64 alldefconfig
powerpc pseries_defconfig
mips gpr_defconfig
mips maltaaprp_defconfig
powerpc ppc6xx_defconfig
powerpc mpc837x_mds_defconfig
ia64 alldefconfig
sh edosk7705_defconfig
sh se7750_defconfig
powerpc socrates_defconfig
riscv allnoconfig
powerpc ksi8560_defconfig
powerpc mpc837x_rdb_defconfig
sh landisk_defconfig
powerpc mpc832x_rdb_defconfig
powerpc ebony_defconfig
arm h5000_defconfig
powerpc ppc40x_defconfig
h8300 allyesconfig
h8300 h8s-sim_defconfig
arm aspeed_g4_defconfig
sh sh7785lcr_32bit_defconfig
mips lemote2f_defconfig
mips rm200_defconfig
arm stm32_defconfig
powerpc ppc64_defconfig
xtensa audio_kc705_defconfig
mips loongson3_defconfig
mips ath79_defconfig
arc haps_hs_smp_defconfig
sh se7712_defconfig
powerpc microwatt_defconfig
sh r7780mp_defconfig
arm cns3420vb_defconfig
mips ath25_defconfig
m68k allyesconfig
powerpc amigaone_defconfig
arm s5pv210_defconfig
arm viper_defconfig
sh lboxre2_defconfig
sh urquell_defconfig
microblaze mmu_defconfig
arm nhk8815_defconfig
sh ap325rxa_defconfig
arm gemini_defconfig
sh alldefconfig
sh apsh4a3a_defconfig
powerpc cm5200_defconfig
arm pleb_defconfig
powerpc fsp2_defconfig
arm multi_v4t_defconfig
ia64 allmodconfig
ia64 allyesconfig
m68k allmodconfig
m68k defconfig
nios2 defconfig
arc allyesconfig
nds32 allnoconfig
nios2 allyesconfig
csky defconfig
alpha defconfig
alpha allyesconfig
arc defconfig
sh allmodconfig
parisc defconfig
s390 allyesconfig
s390 allmodconfig
parisc allyesconfig
s390 defconfig
i386 allyesconfig
sparc allyesconfig
sparc defconfig
i386 defconfig
mips allyesconfig
mips allmodconfig
powerpc allyesconfig
powerpc allmodconfig
powerpc allnoconfig
x86_64 randconfig-a006-20210812
x86_64 randconfig-a004-20210812
x86_64 randconfig-a003-20210812
x86_64 randconfig-a005-20210812
x86_64 randconfig-a002-20210812
x86_64 randconfig-a001-20210812
x86_64 randconfig-a004-20210814
x86_64 randconfig-a006-20210814
x86_64 randconfig-a003-20210814
x86_64 randconfig-a001-20210814
x86_64 randconfig-a005-20210814
x86_64 randconfig-a002-20210814
i386 randconfig-a004-20210814
i386 randconfig-a002-20210814
i386 randconfig-a001-20210814
i386 randconfig-a003-20210814
i386 randconfig-a006-20210814
i386 randconfig-a005-20210814
i386 randconfig-a004-20210812
i386 randconfig-a003-20210812
i386 randconfig-a002-20210812
i386 randconfig-a001-20210812
i386 randconfig-a006-20210812
i386 randconfig-a005-20210812
i386 randconfig-a004-20210813
i386 randconfig-a003-20210813
i386 randconfig-a001-20210813
i386 randconfig-a002-20210813
i386 randconfig-a006-20210813
i386 randconfig-a005-20210813
x86_64 randconfig-a011-20210813
x86_64 randconfig-a013-20210813
x86_64 randconfig-a012-20210813
x86_64 randconfig-a016-20210813
x86_64 randconfig-a015-20210813
x86_64 randconfig-a014-20210813
i386 randconfig-a011-20210814
i386 randconfig-a015-20210814
i386 randconfig-a013-20210814
i386 randconfig-a014-20210814
i386 randconfig-a016-20210814
i386 randconfig-a012-20210814
i386 randconfig-a011-20210812
i386 randconfig-a015-20210812
i386 randconfig-a013-20210812
i386 randconfig-a014-20210812
i386 randconfig-a016-20210812
i386 randconfig-a012-20210812
i386 randconfig-a011-20210813
i386 randconfig-a015-20210813
i386 randconfig-a014-20210813
i386 randconfig-a013-20210813
i386 randconfig-a016-20210813
i386 randconfig-a012-20210813
riscv nommu_k210_defconfig
riscv allyesconfig
riscv nommu_virt_defconfig
riscv defconfig
riscv rv32_defconfig
riscv allmodconfig
x86_64 rhel-8.3-kselftests
um x86_64_defconfig
um i386_defconfig
x86_64 allyesconfig
x86_64 defconfig
x86_64 rhel-8.3
x86_64 kexec
clang tested configs:
x86_64 randconfig-c001-20210812
x86_64 randconfig-c001-20210813
x86_64 randconfig-c001-20210814
x86_64 randconfig-a006-20210813
x86_64 randconfig-a004-20210813
x86_64 randconfig-a003-20210813
x86_64 randconfig-a002-20210813
x86_64 randconfig-a005-20210813
x86_64 randconfig-a001-20210813
x86_64 randconfig-a011-20210812
x86_64 randconfig-a013-20210812
x86_64 randconfig-a012-20210812
x86_64 randconfig-a016-20210812
x86_64 randconfig-a015-20210812
x86_64 randconfig-a014-20210812
x86_64 randconfig-a013-20210814
x86_64 randconfig-a011-20210814
x86_64 randconfig-a016-20210814
x86_64 randconfig-a012-20210814
x86_64 randconfig-a014-20210814
x86_64 randconfig-a015-20210814
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply
* Re: [PATCH v3 4/8] PCI: replace pci_dev::driver usage that gets the driver name
From: Christoph Hellwig @ 2021-08-14 8:38 UTC (permalink / raw)
To: Uwe Kleine-K??nig
Cc: Alexander Duyck, oss-drivers, Paul Mackerras, Herbert Xu,
Ido Schimmel, Rafa?? Mi??ecki, Jesse Brandeburg,
Christoph Hellwig, Bjorn Helgaas, linux-pci, Jakub Kicinski,
Yisen Zhuang, Vadym Kochan, Michael Buesch, Jiri Pirko,
Salil Mehta, Greg Kroah-Hartman, linux-wireless, linux-kernel,
Taras Chornyi, Zhou Wang, linux-crypto, kernel, netdev,
Simon Horman, Oliver O'Halloran, linuxppc-dev,
David S. Miller
In-Reply-To: <20210812081425.7pjy4a25e2ehkr3x@pengutronix.de>
On Thu, Aug 12, 2021 at 10:14:25AM +0200, Uwe Kleine-K??nig wrote:
> dev_driver_string() might return "" (via dev_bus_name()). If that happens
> *drvstr == '\0' becomes true.
>
> Would the following be better?:
>
> const char *drvstr;
>
> if (pdev)
> return "<null>";
>
> drvstr = dev_driver_string(&pdev->dev);
>
> if (!strcmp(drvstr, ""))
> return "<null>";
>
> return drvstr;
>
> When I thought about this hunk I considered it ugly to have "<null>" in
> it twice.
Well, if you want to avoid that you can do:
if (pdev) {
const char *name = dev_driver_string(&pdev->dev);
if (strcmp(drvstr, ""))
return name;
}
return "<null>";
Which would be a lot more readable.
^ permalink raw reply
* Re: [PATCH v1 17/55] KVM: PPC: Book3S HV P9: Implement PMU save/restore in C
From: Athira Rajeev @ 2021-08-14 7:12 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev, kvm-ppc
In-Reply-To: <1628827731.ai2zz7xxwa.astroid@bobo.none>
> On 13-Aug-2021, at 9:54 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Excerpts from Athira Rajeev's message of August 9, 2021 1:03 pm:
>>
>>
>>> On 26-Jul-2021, at 9:19 AM, Nicholas Piggin <npiggin@gmail.com> wrote:
>
>
>>> +static void freeze_pmu(unsigned long mmcr0, unsigned long mmcra)
>>> +{
>>> + if (!(mmcr0 & MMCR0_FC))
>>> + goto do_freeze;
>>> + if (mmcra & MMCRA_SAMPLE_ENABLE)
>>> + goto do_freeze;
>>> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>>> + if (!(mmcr0 & MMCR0_PMCCEXT))
>>> + goto do_freeze;
>>> + if (!(mmcra & MMCRA_BHRB_DISABLE))
>>> + goto do_freeze;
>>> + }
>>> + return;
>>> +
>>> +do_freeze:
>>> + mmcr0 = MMCR0_FC;
>>> + mmcra = 0;
>>> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>>> + mmcr0 |= MMCR0_PMCCEXT;
>>> + mmcra = MMCRA_BHRB_DISABLE;
>>> + }
>>> +
>>> + mtspr(SPRN_MMCR0, mmcr0);
>>> + mtspr(SPRN_MMCRA, mmcra);
>>> + isync();
>>> +}
>>> +
>> Hi Nick,
>>
>> After feezing pmu, do we need to clear “pmcregs_in_use” as well?
>
> Not until we save the values out of the registers. pmcregs_in_use = 0
> means our hypervisor is free to clear our PMU register contents.
>
>> Also can’t we unconditionally do the MMCR0/MMCRA/ freeze settings in here ? do we need the if conditions for FC/PMCCEXT/BHRB ?
>
> I think it's possible, but pretty minimal advantage. I would prefer to
> set them the way perf does for now.
Sure Nick,
Other changes looks good to me.
Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Thanks
Athira
> If we can move this code into perf/
> it should become easier for you to tweak things.
>
> Thanks,
> Nick
^ permalink raw reply
* Re: [PATCH v15 7/9] powerpc: Set ARCH_HAS_STRICT_MODULE_RWX
From: Jordan Niethe @ 2021-08-14 1:23 UTC (permalink / raw)
To: Fabiano Rosas
Cc: Laurent Vivier, ajd, Aneesh Kumar K.V, muriloo, Greg Kurz,
Nicholas Piggin, cmr, kvm-ppc, naveen.n.rao, David Gibson,
linuxppc-dev, Daniel Axtens
In-Reply-To: <8735rdt0i4.fsf@linux.ibm.com>
On Sat, Aug 14, 2021 at 8:59 AM Fabiano Rosas <farosas@linux.ibm.com> wrote:
>
> Laurent Vivier <lvivier@redhat.com> writes:
>
> >
> > since this patch is merged my VM is experiencing a crash at boot (20% of the time):
> >
> > [ 8.496850] kernel tried to execute exec-protected page (c008000004073278) - exploit
> > attempt? (uid: 0)
> > [ 8.496921] BUG: Unable to handle kernel instruction fetch
> > [ 8.496954] Faulting instruction address: 0xc008000004073278
> > [ 8.496994] Oops: Kernel access of bad area, sig: 11 [#1]
> > [ 8.497028] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
> > [ 8.497071] Modules linked in: drm virtio_console fuse drm_panel_orientation_quirks xfs
> > libcrc32c virtio_net net_failover virtio_blk vmx_crypto failover dm_mirror dm_region_hash
> > dm_log dm_mod
> > [ 8.497186] CPU: 3 PID: 44 Comm: kworker/3:1 Not tainted 5.14.0-rc4+ #12
> > [ 8.497228] Workqueue: events control_work_handler [virtio_console]
> > [ 8.497272] NIP: c008000004073278 LR: c008000004073278 CTR: c0000000001e9de0
> > [ 8.497320] REGS: c00000002e4ef7e0 TRAP: 0400 Not tainted (5.14.0-rc4+)
> > [ 8.497361] MSR: 800000004280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24002822
> > XER: 200400cf
> > [ 8.497426] CFAR: c0000000001e9e44 IRQMASK: 1
> > [ 8.497426] GPR00: c008000004073278 c00000002e4efa80 c000000002a26b00 c000000042c39520
> > [ 8.497426] GPR04: 0000000000000001 0000000000000000 0000000000000000 00000000000000ff
> > [ 8.497426] GPR08: 0000000000000001 c000000042c39520 0000000000000001 c008000004076008
> > [ 8.497426] GPR12: c0000000001e9de0 c0000001fffccb00 c00000000018ba88 c00000002c91d400
> > [ 8.497426] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > [ 8.497426] GPR20: 0000000000000000 0000000000000000 0000000000000000 c008000004080340
> > [ 8.497426] GPR24: c0080000040a01e8 0000000000000000 0000000000000000 c00000002e0975c0
> > [ 8.497426] GPR28: c00000002ce72940 c000000042c39520 0000000000000048 0000000000000038
> > [ 8.497891] NIP [c008000004073278] fill_queue+0xf0/0x210 [virtio_console]
> > [ 8.497934] LR [c008000004073278] fill_queue+0xf0/0x210 [virtio_console]
> > [ 8.497976] Call Trace:
> > [ 8.497993] [c00000002e4efa80] [c00800000407323c] fill_queue+0xb4/0x210
> > [virtio_console] (unreliable)
> > [ 8.498052] [c00000002e4efae0] [c008000004073a90] add_port+0x1a8/0x470 [virtio_console]
> > [ 8.498102] [c00000002e4efbb0] [c0080000040750f4] control_work_handler+0xbc/0x1e8
> > [virtio_console]
> > [ 8.498160] [c00000002e4efc60] [c00000000017f4f0] process_one_work+0x290/0x590
> > [ 8.498212] [c00000002e4efd00] [c00000000017f878] worker_thread+0x88/0x620
> > [ 8.498256] [c00000002e4efda0] [c00000000018bc14] kthread+0x194/0x1a0
> > [ 8.498299] [c00000002e4efe10] [c00000000000cf54] ret_from_kernel_thread+0x5c/0x64
> > [ 8.498349] Instruction dump:
> > [ 8.498374] 7da96b78 a14d0c8a 419c00b0 2f8a0000 419eff88 b32d0c8a 7c0004ac 4bffff7c
> > [ 8.498430] 60000000 60000000 7fa3eb78 48002d95 <e8410018> 38600000 480025e1 e8410018
> > [ 8.498485] ---[ end trace 16ee10903290b647 ]---
> > [ 8.501433]
> > [ 9.502601] Kernel panic - not syncing: Fatal exception
> >
> > add_port+0x1a8/0x470 :
> >
> > 1420
> > 1421 /* We can safely ignore ENOSPC because it means
> > 1422 * the queue already has buffers. Buffers are removed
> > 1423 * only by virtcons_remove(), not by unplug_port()
> > 1424 */
> > ->1425 err = fill_queue(port->in_vq, &port->inbuf_lock);
> > 1426 if (err < 0 && err != -ENOSPC) {
> > 1427 dev_err(port->dev, "Error allocating inbufs\n");
> > 1428 goto free_device;
> > 1429 }
> >
> > fill_queue+0x90/0x210 :
> >
> > 1326 static int fill_queue(struct virtqueue *vq, spinlock_t *lock)
> > 1327 {
> > 1328 struct port_buffer *buf;
> > 1329 int nr_added_bufs;
> > 1330 int ret;
> > 1331
> > 1332 nr_added_bufs = 0;
> > 1333 do {
> > 1334 buf = alloc_buf(vq->vdev, PAGE_SIZE, 0);
> > 1335 if (!buf)
> > 1336 return -ENOMEM;
> > 1337
> > ->1338 spin_lock_irq(lock);
> >
> > I'm using an upstream kernel (5.14-rc4, 251a1524293d) in the VM.
> >
> > My host is a RHEL 8.5/POWER9: qemu-kvm-6.0.0-21 and kernel-4.18.0-325
> >
> > My qemu command line is:
> >
> > /usr/libexec/qemu-kvm \
> > -M pseries,accel=kvm \
> > -nographic -nodefaults \
> > -device virtio-serial-pci \
> > -device virtconsole \
> > -device virtio-net-pci,mac=9a:2b:2c:2d:2e:2f,netdev=hostnet0 \
> > -blockdev
> > node-name=disk1,file.driver=file,driver=qcow2,file.driver=file,file.filename=disk.qcow2 \
> > -netdev bridge,id=hostnet0,br=virbr0,helper=/usr/libexec/qemu-bridge-helper \
> > -device virtio-blk-pci,id=image1,drive=disk1 \
> > -m 8192 \
> > -smp 4 \
> > -serial mon:stdio
> >
> >
> > Do we need something in qemu/kvm to support STRICT_MODULE_RWX ?
> >
> > Thanks,
> > Laurent
>
> This patch survived 300 consecutive runs without the issue:
>
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> index 0876216ceee6..7aeb2b62e5dc 100644
> --- a/arch/powerpc/mm/pageattr.c
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -35,10 +35,7 @@ static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
> pte_t pte;
>
> spin_lock(&init_mm.page_table_lock);
> -
> - /* invalidate the PTE so it's safe to modify */
> - pte = ptep_get_and_clear(&init_mm, addr, ptep);
> - flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> + pte = *ptep;
>
> /* modify the PTE bits as desired, then apply */
> switch (action) {
> @@ -60,10 +57,7 @@ static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
> }
>
> set_pte_at(&init_mm, addr, ptep, pte);
> -
> - /* See ptesync comment in radix__set_pte_at() */
> - if (radix_enabled())
> - asm volatile("ptesync": : :"memory");
> + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> spin_unlock(&init_mm.page_table_lock);
>
> return 0;
> ---
>
> What I think is happening is that the virtio_console code is running
> at the same time we are doing the `module_enable_ro` at the end of
> `do_init_module` due to the async nature of the work handler. There is
> a window after the TLB flush when the PTE has its permission bits
> cleared, so any translations of the module code page attempted during
> that window will fault.
Thanks for looking at this. I agree that this is the problem.
This avoids the crash (not a proper solution):
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2011,13 +2011,15 @@ static void module_enable_ro(const struct
module *mod, bool after_init)
if (!rodata_enabled)
return;
- set_vm_flush_reset_perms(mod->core_layout.base);
- set_vm_flush_reset_perms(mod->init_layout.base);
- frob_text(&mod->core_layout, set_memory_ro);
-
- frob_rodata(&mod->core_layout, set_memory_ro);
- frob_text(&mod->init_layout, set_memory_ro);
- frob_rodata(&mod->init_layout, set_memory_ro);
+ if (!after_init) {
+ set_vm_flush_reset_perms(mod->core_layout.base);
+ set_vm_flush_reset_perms(mod->init_layout.base);
+ frob_text(&mod->core_layout, set_memory_ro);
+
+ frob_rodata(&mod->core_layout, set_memory_ro);
+ frob_text(&mod->init_layout, set_memory_ro);
+ frob_rodata(&mod->init_layout, set_memory_ro);
+ }
>
> I'm ignorant of strict rwx in general so I don't see why we need to
> clear the bits before setting them to their final value, but as I
> understand it, the set_pte_at + flush_tlb_kernel_range satisfy the ISA
> requirement of [ptesync; tlbie; eieio; tlbsync; ptesync;] so it seems
> like the patch should work.
I believe it's done like that because ISA 6.10.1.2 Modifying a
Translation Table Entry -
"The sequence is equivalent to deleting the PTE and then adding a new one".
In that sequence, I think it is [set pte to 0; ptesync; tlbie; eieio;
tlbsync; ptesync;]
But it does seem like we need to do something like your patch for
change_page_attr() to work as expected.
>
> Now, I cannot explain why the crash always happens around the code
> that does the module's symbols relocation (the NIP in Laurent's trace
> is the TOC reload from module_64.c:restore_r2). Maybe because
> instructions are already in icache until the first branch into the
> stub?
Yeah, mpe thought it might be some cache effect of a branch instruction.
From time to time I did see Oopses like this (not "kernel tried to
execute exec-protected page")
[ 124.986964][ T52] Oops: Kernel access of bad area, sig: 11 [#1]
[ 124.987043][ T52] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
[ 124.987095][ T52] Modules linked in: virtio_console binfmt_misc
virtiofs fuse virtio_net virtio_blk virtio_pci virtio_pci_modern_dev
virtio_ring virtio crc32c_vpmsum [last unloaded: virtio_console]
[ 124.987209][ T52] CPU: 2 PID: 52 Comm: kworker/2:1 Not tainted
5.14.0-rc4 #4
[ 124.987259][ T52] Workqueue: events control_work_handler [virtio_console]
[ 124.987307][ T52] NIP: c008000001a86044 LR: c008000001a82cf8
CTR: c00000000042a6c0
[ 124.987358][ T52] REGS: c00000000b273770 TRAP: 0300 Not tainted
(5.14.0-rc4)
[ 124.987406][ T52] MSR: 800000000280b033
<SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 82002882 XER: 00000004
[ 124.987475][ T52] CFAR: c008000001a82cf4 DAR: c008000001a86058
DSISR: 40000000 IRQMASK: 0
[ 124.987475][ T52] GPR00: c008000001a82ce8 c00000000b273a10
c008000001ab8000 c000000010731320
[ 124.987475][ T52] GPR04: 0000000000000001 0000000000000000
0000000000000000 0000000000000000
[ 124.987475][ T52] GPR08: 0000000000000000 0000000000000000
0000000000000000 c008000001a86038
[ 124.987475][ T52] GPR12: 0000000022002884 c0000001ffffdf00
c000000000169ae8 c0000000037700c0
[ 124.987475][ T52] GPR16: 0000000000000000 0000000000000000
c000000021af0000 c000000003313900
[ 124.987475][ T52] GPR20: 0000000000000001 0000000000000000
0000000000000000 c000000003316780
[ 124.987475][ T52] GPR24: c008000001a90288 0000000000000000
0000000000000000 c000000010731320
[ 124.987475][ T52] GPR28: c000000003316900 0000000000000018
0000000000000068 c00000000379c5a0
[ 124.988028][ T52] NIP [c008000001a86044] fini+0x824/0xa7e0 [virtio_console]
[ 124.988085][ T52] LR [c008000001a82cf8] fill_queue+0xb0/0x230
[virtio_console]
[ 124.988141][ T52] Call Trace:
[ 124.988171][ T52] [c00000000b273a10] [c008000001a82ce8]
fill_queue+0xa0/0x230 [virtio_console] (unreliable)
[ 124.988246][ T52] [c00000000b273aa0] [c008000001a83208]
add_port.isra.0+0x1a0/0x4b0 [virtio_console]
[ 124.988312][ T52] [c00000000b273b70] [c008000001a85474]
control_work_handler+0x46c/0x654 [virtio_console]
[ 124.988403][ T52] [c00000000b273c70] [c00000000015ce60]
process_one_work+0x2a0/0x570
[ 124.988586][ T52] [c00000000b273d10] [c00000000015d1d8]
worker_thread+0xa8/0x660
[ 124.988684][ T52] [c00000000b273da0] [c000000000169c5c] kthread+0x17c/0x190
[ 124.988762][ T52] [c00000000b273e10] [c00000000000cf54]
ret_from_kernel_thread+0x5c/0x64
[ 124.988916][ T52] Instruction dump:
[ 124.988965][ T52] 396be010 f8410018 e98b0020 7d8903a6 4e800420
00000000 73747562 003a0300
[ 124.989152][ T52] c0000000 3d62fffd 396be038 f8410018 <e98b0020>
7d8903a6 4e800420 00000000
[ 124.989298][ T52] ---[ end trace ab9046c024eb3154 ]---
I guess it depends on the timing of which pte is invalidated.
>
> Anyway, this is what Murilo and I found out over our debugging session
> in the past couple of days. I hope it helps. =)
Yes it does, thanks heaps.
>
^ permalink raw reply
* Re: [PATCH v15 7/9] powerpc: Set ARCH_HAS_STRICT_MODULE_RWX
From: Fabiano Rosas @ 2021-08-13 22:58 UTC (permalink / raw)
To: Laurent Vivier, Jordan Niethe, linuxppc-dev
Cc: ajd, aneesh.kumar, muriloo, Greg Kurz, npiggin, cmr, kvm-ppc,
naveen.n.rao, David Gibson, dja
In-Reply-To: <f7624d58-80e1-6912-1867-7874f1a569f5@redhat.com>
Laurent Vivier <lvivier@redhat.com> writes:
>
> since this patch is merged my VM is experiencing a crash at boot (20% of the time):
>
> [ 8.496850] kernel tried to execute exec-protected page (c008000004073278) - exploit
> attempt? (uid: 0)
> [ 8.496921] BUG: Unable to handle kernel instruction fetch
> [ 8.496954] Faulting instruction address: 0xc008000004073278
> [ 8.496994] Oops: Kernel access of bad area, sig: 11 [#1]
> [ 8.497028] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
> [ 8.497071] Modules linked in: drm virtio_console fuse drm_panel_orientation_quirks xfs
> libcrc32c virtio_net net_failover virtio_blk vmx_crypto failover dm_mirror dm_region_hash
> dm_log dm_mod
> [ 8.497186] CPU: 3 PID: 44 Comm: kworker/3:1 Not tainted 5.14.0-rc4+ #12
> [ 8.497228] Workqueue: events control_work_handler [virtio_console]
> [ 8.497272] NIP: c008000004073278 LR: c008000004073278 CTR: c0000000001e9de0
> [ 8.497320] REGS: c00000002e4ef7e0 TRAP: 0400 Not tainted (5.14.0-rc4+)
> [ 8.497361] MSR: 800000004280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24002822
> XER: 200400cf
> [ 8.497426] CFAR: c0000000001e9e44 IRQMASK: 1
> [ 8.497426] GPR00: c008000004073278 c00000002e4efa80 c000000002a26b00 c000000042c39520
> [ 8.497426] GPR04: 0000000000000001 0000000000000000 0000000000000000 00000000000000ff
> [ 8.497426] GPR08: 0000000000000001 c000000042c39520 0000000000000001 c008000004076008
> [ 8.497426] GPR12: c0000000001e9de0 c0000001fffccb00 c00000000018ba88 c00000002c91d400
> [ 8.497426] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 8.497426] GPR20: 0000000000000000 0000000000000000 0000000000000000 c008000004080340
> [ 8.497426] GPR24: c0080000040a01e8 0000000000000000 0000000000000000 c00000002e0975c0
> [ 8.497426] GPR28: c00000002ce72940 c000000042c39520 0000000000000048 0000000000000038
> [ 8.497891] NIP [c008000004073278] fill_queue+0xf0/0x210 [virtio_console]
> [ 8.497934] LR [c008000004073278] fill_queue+0xf0/0x210 [virtio_console]
> [ 8.497976] Call Trace:
> [ 8.497993] [c00000002e4efa80] [c00800000407323c] fill_queue+0xb4/0x210
> [virtio_console] (unreliable)
> [ 8.498052] [c00000002e4efae0] [c008000004073a90] add_port+0x1a8/0x470 [virtio_console]
> [ 8.498102] [c00000002e4efbb0] [c0080000040750f4] control_work_handler+0xbc/0x1e8
> [virtio_console]
> [ 8.498160] [c00000002e4efc60] [c00000000017f4f0] process_one_work+0x290/0x590
> [ 8.498212] [c00000002e4efd00] [c00000000017f878] worker_thread+0x88/0x620
> [ 8.498256] [c00000002e4efda0] [c00000000018bc14] kthread+0x194/0x1a0
> [ 8.498299] [c00000002e4efe10] [c00000000000cf54] ret_from_kernel_thread+0x5c/0x64
> [ 8.498349] Instruction dump:
> [ 8.498374] 7da96b78 a14d0c8a 419c00b0 2f8a0000 419eff88 b32d0c8a 7c0004ac 4bffff7c
> [ 8.498430] 60000000 60000000 7fa3eb78 48002d95 <e8410018> 38600000 480025e1 e8410018
> [ 8.498485] ---[ end trace 16ee10903290b647 ]---
> [ 8.501433]
> [ 9.502601] Kernel panic - not syncing: Fatal exception
>
> add_port+0x1a8/0x470 :
>
> 1420
> 1421 /* We can safely ignore ENOSPC because it means
> 1422 * the queue already has buffers. Buffers are removed
> 1423 * only by virtcons_remove(), not by unplug_port()
> 1424 */
> ->1425 err = fill_queue(port->in_vq, &port->inbuf_lock);
> 1426 if (err < 0 && err != -ENOSPC) {
> 1427 dev_err(port->dev, "Error allocating inbufs\n");
> 1428 goto free_device;
> 1429 }
>
> fill_queue+0x90/0x210 :
>
> 1326 static int fill_queue(struct virtqueue *vq, spinlock_t *lock)
> 1327 {
> 1328 struct port_buffer *buf;
> 1329 int nr_added_bufs;
> 1330 int ret;
> 1331
> 1332 nr_added_bufs = 0;
> 1333 do {
> 1334 buf = alloc_buf(vq->vdev, PAGE_SIZE, 0);
> 1335 if (!buf)
> 1336 return -ENOMEM;
> 1337
> ->1338 spin_lock_irq(lock);
>
> I'm using an upstream kernel (5.14-rc4, 251a1524293d) in the VM.
>
> My host is a RHEL 8.5/POWER9: qemu-kvm-6.0.0-21 and kernel-4.18.0-325
>
> My qemu command line is:
>
> /usr/libexec/qemu-kvm \
> -M pseries,accel=kvm \
> -nographic -nodefaults \
> -device virtio-serial-pci \
> -device virtconsole \
> -device virtio-net-pci,mac=9a:2b:2c:2d:2e:2f,netdev=hostnet0 \
> -blockdev
> node-name=disk1,file.driver=file,driver=qcow2,file.driver=file,file.filename=disk.qcow2 \
> -netdev bridge,id=hostnet0,br=virbr0,helper=/usr/libexec/qemu-bridge-helper \
> -device virtio-blk-pci,id=image1,drive=disk1 \
> -m 8192 \
> -smp 4 \
> -serial mon:stdio
>
>
> Do we need something in qemu/kvm to support STRICT_MODULE_RWX ?
>
> Thanks,
> Laurent
This patch survived 300 consecutive runs without the issue:
diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
index 0876216ceee6..7aeb2b62e5dc 100644
--- a/arch/powerpc/mm/pageattr.c
+++ b/arch/powerpc/mm/pageattr.c
@@ -35,10 +35,7 @@ static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
pte_t pte;
spin_lock(&init_mm.page_table_lock);
-
- /* invalidate the PTE so it's safe to modify */
- pte = ptep_get_and_clear(&init_mm, addr, ptep);
- flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+ pte = *ptep;
/* modify the PTE bits as desired, then apply */
switch (action) {
@@ -60,10 +57,7 @@ static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
}
set_pte_at(&init_mm, addr, ptep, pte);
-
- /* See ptesync comment in radix__set_pte_at() */
- if (radix_enabled())
- asm volatile("ptesync": : :"memory");
+ flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
spin_unlock(&init_mm.page_table_lock);
return 0;
---
What I think is happening is that the virtio_console code is running
at the same time we are doing the `module_enable_ro` at the end of
`do_init_module` due to the async nature of the work handler. There is
a window after the TLB flush when the PTE has its permission bits
cleared, so any translations of the module code page attempted during
that window will fault.
I'm ignorant of strict rwx in general so I don't see why we need to
clear the bits before setting them to their final value, but as I
understand it, the set_pte_at + flush_tlb_kernel_range satisfy the ISA
requirement of [ptesync; tlbie; eieio; tlbsync; ptesync;] so it seems
like the patch should work.
Now, I cannot explain why the crash always happens around the code
that does the module's symbols relocation (the NIP in Laurent's trace
is the TOC reload from module_64.c:restore_r2). Maybe because
instructions are already in icache until the first branch into the
stub?
Anyway, this is what Murilo and I found out over our debugging session
in the past couple of days. I hope it helps. =)
^ permalink raw reply related
* Re: [PATCH] soc: fsl: qe: fix static checker warning
From: Li Yang @ 2021-08-13 21:39 UTC (permalink / raw)
To: Maxim Kochetkov
Cc: Saravana Kannan, lkml,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linuxppc-dev, Dan Carpenter, Qiang Zhao
In-Reply-To: <20210811071036.44658-1-fido_max@inbox.ru>
On Wed, Aug 11, 2021 at 2:10 AM Maxim Kochetkov <fido_max@inbox.ru> wrote:
>
> The patch be7ecbd240b2: "soc: fsl: qe: convert QE interrupt
> controller to platform_device" from Aug 3, 2021, leads to the
> following static checker warning:
>
> drivers/soc/fsl/qe/qe_ic.c:438 qe_ic_init()
> warn: unsigned 'qe_ic->virq_low' is never less than zero.
>
> In old variant irq_of_parse_and_map() returns zero if failed so
> unsigned int for virq_high/virq_low was ok.
> In new variant platform_get_irq() returns negative error codes
> if failed so we need to use int for virq_high/virq_low.
>
> Also simplify high_handler checking and remove the curly braces
> to make checkpatch happy.
>
> Fixes: be7ecbd240b2 ("soc: fsl: qe: convert QE interrupt controller to platform_device")
> Signed-off-by: Maxim Kochetkov <fido_max@inbox.ru>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> drivers/soc/fsl/qe/qe_ic.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/fsl/qe/qe_ic.c b/drivers/soc/fsl/qe/qe_ic.c
> index e710d554425d..bff34ee2150a 100644
> --- a/drivers/soc/fsl/qe/qe_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ic.c
> @@ -54,8 +54,8 @@ struct qe_ic {
> struct irq_chip hc_irq;
>
> /* VIRQ numbers of QE high/low irqs */
> - unsigned int virq_high;
> - unsigned int virq_low;
> + int virq_high;
> + int virq_low;
> };
>
> /*
> @@ -435,9 +435,8 @@ static int qe_ic_init(struct platform_device *pdev)
> qe_ic->virq_high = platform_get_irq(pdev, 0);
> qe_ic->virq_low = platform_get_irq(pdev, 1);
>
> - if (qe_ic->virq_low < 0) {
> + if (qe_ic->virq_low < 0)
Probably should be <= 0 here.
> return -ENODEV;
> - }
>
> if (qe_ic->virq_high != qe_ic->virq_low) {
Probably we should check if qe_ic->virq_high > 0 here if we rely on
this to decide whether to set the handler later.
Applied with the above changes. Thanks
> low_handler = qe_ic_cascade_low;
> @@ -459,7 +458,7 @@ static int qe_ic_init(struct platform_device *pdev)
> irq_set_handler_data(qe_ic->virq_low, qe_ic);
> irq_set_chained_handler(qe_ic->virq_low, low_handler);
>
> - if (qe_ic->virq_high && qe_ic->virq_high != qe_ic->virq_low) {
> + if (high_handler) {
> irq_set_handler_data(qe_ic->virq_high, qe_ic);
> irq_set_chained_handler(qe_ic->virq_high, high_handler);
> }
> --
> 2.31.1
>
^ permalink raw reply
* Re: [PATCH 07/11] treewide: Replace the use of mem_encrypt_active() with prot_guest_has()
From: Tom Lendacky @ 2021-08-13 20:17 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: Kuppuswamy, Sathyanarayanan, linux-efi, Brijesh Singh, kvm,
Peter Zijlstra, Dave Hansen, dri-devel, platform-driver-x86,
Will Deacon, linux-s390, Andi Kleen, Baoquan He, Joerg Roedel,
x86, amd-gfx, David Airlie, Ingo Molnar,
linux-graphics-maintainer, Dave Young, Tianyu Lan,
Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard,
Borislav Petkov, Andy Lutomirski, Thomas Gleixner, kexec,
linux-kernel, iommu, Daniel Vetter, linux-fsdevel, linuxppc-dev
In-Reply-To: <943223d5-5949-6aba-8a49-0b07078d68e1@amd.com>
On 8/13/21 12:08 PM, Tom Lendacky wrote:
> On 8/12/21 5:07 AM, Kirill A. Shutemov wrote:
>> On Wed, Aug 11, 2021 at 10:52:55AM -0500, Tom Lendacky wrote:
>>> On 8/11/21 7:19 AM, Kirill A. Shutemov wrote:
>>>> On Tue, Aug 10, 2021 at 02:48:54PM -0500, Tom Lendacky wrote:
>>>>> On 8/10/21 1:45 PM, Kuppuswamy, Sathyanarayanan wrote:
> ...
>>>> Looking at code agains, now I *think* the reason is accessing a global
>>>> variable from __startup_64() inside TDX version of prot_guest_has().
>>>>
>>>> __startup_64() is special. If you access any global variable you need to
>>>> use fixup_pointer(). See comment before __startup_64().
>>>>
>>>> I'm not sure how you get away with accessing sme_me_mask directly from
>>>> there. Any clues? Maybe just a luck and complier generates code just
>>>> right
>>>> for your case, I donno.
>>>
>>> Hmm... yeah, could be that the compiler is using rip-relative addressing
>>> for it because it lives in the .data section?
>>
>> I guess. It has to be fixed. It may break with complier upgrade or any
>> random change around the code.
>
> I'll look at doing that separate from this series.
>
>>
>> BTW, does it work with clang for you?
>
> I haven't tried with clang, I'll check on that.
Just as an fyi, clang also uses rip relative addressing for those
variables. No issues booting SME and SEV guests built with clang.
Thanks,
Tom
>
> Thanks,
> Tom
>
>>
^ 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