* [PATCH 2/5] entry: rseq: Call rseq_handle_notify_resume() in tracehook_notify_resume()
From: Sean Christopherson @ 2021-08-18 0:12 UTC (permalink / raw)
To: Russell King, Catalin Marinas, Will Deacon, Guo Ren,
Thomas Bogendoerfer, Michael Ellerman, Heiko Carstens,
Vasily Gorbik, Christian Borntraeger, Oleg Nesterov,
Steven Rostedt, Ingo Molnar, Thomas Gleixner, Peter Zijlstra,
Andy Lutomirski, Mathieu Desnoyers, Paul E. McKenney, Boqun Feng,
Paolo Bonzini, Shuah Khan
Cc: linux-s390, kvm, Ben Gardon, linux-kernel, linux-csky, linux-mips,
Peter Foley, Paul Mackerras, linux-kselftest, Sean Christopherson,
Shakeel Butt, linuxppc-dev, linux-arm-kernel
In-Reply-To: <20210818001210.4073390-1-seanjc@google.com>
Invoke rseq_handle_notify_resume() from tracehook_notify_resume() now
that the two function are always called back-to-back by architectures
that have rseq. The rseq helper is stubbed out for architectures that
don't support rseq, i.e. this is a nop across the board.
Note, tracehook_notify_resume() is horribly named and arguably does not
belong in tracehook.h as literally every line of code in it has nothing
to do with tracing. But, that's been true since commit a42c6ded827d
("move key_repace_session_keyring() into tracehook_notify_resume()")
first usurped tracehook_notify_resume() back in 2012. Punt cleaning that
mess up to future patches.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/arm/kernel/signal.c | 1 -
arch/arm64/kernel/signal.c | 1 -
arch/csky/kernel/signal.c | 4 +---
arch/mips/kernel/signal.c | 4 +---
arch/powerpc/kernel/signal.c | 4 +---
arch/s390/kernel/signal.c | 1 -
include/linux/tracehook.h | 2 ++
kernel/entry/common.c | 4 +---
kernel/entry/kvm.c | 4 +---
9 files changed, 7 insertions(+), 18 deletions(-)
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index a3a38d0a4c85..9df68d139965 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -670,7 +670,6 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
uprobe_notify_resume(regs);
} else {
tracehook_notify_resume(regs);
- rseq_handle_notify_resume(NULL, regs);
}
}
local_irq_disable();
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 23036334f4dc..22b55db13da6 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -951,7 +951,6 @@ asmlinkage void do_notify_resume(struct pt_regs *regs,
if (thread_flags & _TIF_NOTIFY_RESUME) {
tracehook_notify_resume(regs);
- rseq_handle_notify_resume(NULL, regs);
/*
* If we reschedule after checking the affinity
diff --git a/arch/csky/kernel/signal.c b/arch/csky/kernel/signal.c
index 312f046d452d..bc4238b9f709 100644
--- a/arch/csky/kernel/signal.c
+++ b/arch/csky/kernel/signal.c
@@ -260,8 +260,6 @@ asmlinkage void do_notify_resume(struct pt_regs *regs,
if (thread_info_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL))
do_signal(regs);
- if (thread_info_flags & _TIF_NOTIFY_RESUME) {
+ if (thread_info_flags & _TIF_NOTIFY_RESUME)
tracehook_notify_resume(regs);
- rseq_handle_notify_resume(NULL, regs);
- }
}
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index f1e985109da0..c9b2a75563e1 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -906,10 +906,8 @@ asmlinkage void do_notify_resume(struct pt_regs *regs, void *unused,
if (thread_info_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL))
do_signal(regs);
- if (thread_info_flags & _TIF_NOTIFY_RESUME) {
+ if (thread_info_flags & _TIF_NOTIFY_RESUME)
tracehook_notify_resume(regs);
- rseq_handle_notify_resume(NULL, regs);
- }
user_enter();
}
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index e600764a926c..b93b87df499d 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -293,10 +293,8 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
do_signal(current);
}
- if (thread_info_flags & _TIF_NOTIFY_RESUME) {
+ if (thread_info_flags & _TIF_NOTIFY_RESUME)
tracehook_notify_resume(regs);
- rseq_handle_notify_resume(NULL, regs);
- }
}
static unsigned long get_tm_stackpointer(struct task_struct *tsk)
diff --git a/arch/s390/kernel/signal.c b/arch/s390/kernel/signal.c
index 78ef53b29958..b307db26bf2d 100644
--- a/arch/s390/kernel/signal.c
+++ b/arch/s390/kernel/signal.c
@@ -537,5 +537,4 @@ void arch_do_signal_or_restart(struct pt_regs *regs, bool has_signal)
void do_notify_resume(struct pt_regs *regs)
{
tracehook_notify_resume(regs);
- rseq_handle_notify_resume(NULL, regs);
}
diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index 3e80c4bc66f7..2564b7434b4d 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -197,6 +197,8 @@ static inline void tracehook_notify_resume(struct pt_regs *regs)
mem_cgroup_handle_over_high();
blkcg_maybe_throttle_current();
+
+ rseq_handle_notify_resume(NULL, regs);
}
/*
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index bf16395b9e13..d5a61d565ad5 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -171,10 +171,8 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
if (ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL))
handle_signal_work(regs, ti_work);
- if (ti_work & _TIF_NOTIFY_RESUME) {
+ if (ti_work & _TIF_NOTIFY_RESUME)
tracehook_notify_resume(regs);
- rseq_handle_notify_resume(NULL, regs);
- }
/* Architecture specific TIF work */
arch_exit_to_user_mode_work(regs, ti_work);
diff --git a/kernel/entry/kvm.c b/kernel/entry/kvm.c
index 049fd06b4c3d..49972ee99aff 100644
--- a/kernel/entry/kvm.c
+++ b/kernel/entry/kvm.c
@@ -19,10 +19,8 @@ static int xfer_to_guest_mode_work(struct kvm_vcpu *vcpu, unsigned long ti_work)
if (ti_work & _TIF_NEED_RESCHED)
schedule();
- if (ti_work & _TIF_NOTIFY_RESUME) {
+ if (ti_work & _TIF_NOTIFY_RESUME)
tracehook_notify_resume(NULL);
- rseq_handle_notify_resume(NULL, NULL);
- }
ret = arch_xfer_to_guest_mode_handle_work(vcpu, ti_work);
if (ret)
--
2.33.0.rc1.237.g0d66db33f3-goog
^ permalink raw reply related
* [PATCH 0/5] KVM: rseq: Fix and a test for a KVM+rseq bug
From: Sean Christopherson @ 2021-08-18 0:12 UTC (permalink / raw)
To: Russell King, Catalin Marinas, Will Deacon, Guo Ren,
Thomas Bogendoerfer, Michael Ellerman, Heiko Carstens,
Vasily Gorbik, Christian Borntraeger, Oleg Nesterov,
Steven Rostedt, Ingo Molnar, Thomas Gleixner, Peter Zijlstra,
Andy Lutomirski, Mathieu Desnoyers, Paul E. McKenney, Boqun Feng,
Paolo Bonzini, Shuah Khan
Cc: linux-s390, kvm, Ben Gardon, linux-kernel, linux-csky, linux-mips,
Peter Foley, Paul Mackerras, linux-kselftest, Sean Christopherson,
Shakeel Butt, linuxppc-dev, linux-arm-kernel
Patch 1 fixes a KVM+rseq bug where KVM's handling of TIF_NOTIFY_RESUME,
e.g. for task migration, clears the flag without informing rseq and leads
to stale data in userspace's rseq struct.
Patch 2 is a cleanup to try and make future bugs less likely. It's also
a baby step towards moving and renaming tracehook_notify_resume() since
it has nothing to do with tracing. It kills me to not do the move/rename
as part of this series, but having a dedicated series/discussion seems
more appropriate given the sheer number of architectures that call
tracehook_notify_resume() and the lack of an obvious home for the code.
Patch 3 is a fix/cleanup to stop overriding x86's unistd_{32,64}.h when
the include path (intentionally) omits tools' uapi headers. KVM's
selftests do exactly that so that they can pick up the uapi headers from
the installed kernel headers, and still use various tools/ headers that
mirror kernel code, e.g. linux/types.h. This allows the new test in
patch 4 to reference __NR_rseq without having to manually define it.
Patch 4 is a regression test for the KVM+rseq bug.
Patch 5 is a cleanup made possible by patch 3.
Sean Christopherson (5):
KVM: rseq: Update rseq when processing NOTIFY_RESUME on xfer to KVM
guest
entry: rseq: Call rseq_handle_notify_resume() in
tracehook_notify_resume()
tools: Move x86 syscall number fallbacks to .../uapi/
KVM: selftests: Add a test for KVM_RUN+rseq to detect task migration
bugs
KVM: selftests: Remove __NR_userfaultfd syscall fallback
arch/arm/kernel/signal.c | 1 -
arch/arm64/kernel/signal.c | 1 -
arch/csky/kernel/signal.c | 4 +-
arch/mips/kernel/signal.c | 4 +-
arch/powerpc/kernel/signal.c | 4 +-
arch/s390/kernel/signal.c | 1 -
include/linux/tracehook.h | 2 +
kernel/entry/common.c | 4 +-
kernel/rseq.c | 4 +-
.../x86/include/{ => uapi}/asm/unistd_32.h | 0
.../x86/include/{ => uapi}/asm/unistd_64.h | 3 -
tools/testing/selftests/kvm/.gitignore | 1 +
tools/testing/selftests/kvm/Makefile | 3 +
tools/testing/selftests/kvm/rseq_test.c | 131 ++++++++++++++++++
14 files changed, 143 insertions(+), 20 deletions(-)
rename tools/arch/x86/include/{ => uapi}/asm/unistd_32.h (100%)
rename tools/arch/x86/include/{ => uapi}/asm/unistd_64.h (83%)
create mode 100644 tools/testing/selftests/kvm/rseq_test.c
--
2.33.0.rc1.237.g0d66db33f3-goog
^ permalink raw reply
* [PATCH 1/5] KVM: rseq: Update rseq when processing NOTIFY_RESUME on xfer to KVM guest
From: Sean Christopherson @ 2021-08-18 0:12 UTC (permalink / raw)
To: Russell King, Catalin Marinas, Will Deacon, Guo Ren,
Thomas Bogendoerfer, Michael Ellerman, Heiko Carstens,
Vasily Gorbik, Christian Borntraeger, Oleg Nesterov,
Steven Rostedt, Ingo Molnar, Thomas Gleixner, Peter Zijlstra,
Andy Lutomirski, Mathieu Desnoyers, Paul E. McKenney, Boqun Feng,
Paolo Bonzini, Shuah Khan
Cc: linux-s390, kvm, Ben Gardon, linux-kernel, linux-csky, linux-mips,
Peter Foley, Paul Mackerras, linux-kselftest, Sean Christopherson,
Shakeel Butt, linuxppc-dev, linux-arm-kernel
In-Reply-To: <20210818001210.4073390-1-seanjc@google.com>
Invoke rseq's NOTIFY_RESUME handler when processing the flag prior to
transferring to a KVM guest, which is roughly equivalent to an exit to
userspace and processes many of the same pending actions. While the task
cannot be in an rseq critical section as the KVM path is reachable only
via ioctl(KVM_RUN), the side effects that apply to rseq outside of a
critical section still apply, e.g. the CPU ID needs to be updated if the
task is migrated.
Clearing TIF_NOTIFY_RESUME without informing rseq can lead to segfaults
and other badness in userspace VMMs that use rseq in combination with KVM,
e.g. due to the CPU ID being stale after task migration.
Fixes: 72c3c0fe54a3 ("x86/kvm: Use generic xfer to guest work function")
Reported-by: Peter Foley <pefoley@google.com>
Bisected-by: Doug Evans <dje@google.com>
Cc: Shakeel Butt <shakeelb@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
kernel/entry/kvm.c | 4 +++-
kernel/rseq.c | 4 ++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/kernel/entry/kvm.c b/kernel/entry/kvm.c
index 49972ee99aff..049fd06b4c3d 100644
--- a/kernel/entry/kvm.c
+++ b/kernel/entry/kvm.c
@@ -19,8 +19,10 @@ static int xfer_to_guest_mode_work(struct kvm_vcpu *vcpu, unsigned long ti_work)
if (ti_work & _TIF_NEED_RESCHED)
schedule();
- if (ti_work & _TIF_NOTIFY_RESUME)
+ if (ti_work & _TIF_NOTIFY_RESUME) {
tracehook_notify_resume(NULL);
+ rseq_handle_notify_resume(NULL, NULL);
+ }
ret = arch_xfer_to_guest_mode_handle_work(vcpu, ti_work);
if (ret)
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 35f7bd0fced0..58c79a7918cd 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -236,7 +236,7 @@ static bool in_rseq_cs(unsigned long ip, struct rseq_cs *rseq_cs)
static int rseq_ip_fixup(struct pt_regs *regs)
{
- unsigned long ip = instruction_pointer(regs);
+ unsigned long ip = regs ? instruction_pointer(regs) : 0;
struct task_struct *t = current;
struct rseq_cs rseq_cs;
int ret;
@@ -250,7 +250,7 @@ static int rseq_ip_fixup(struct pt_regs *regs)
* If not nested over a rseq critical section, restart is useless.
* Clear the rseq_cs pointer and return.
*/
- if (!in_rseq_cs(ip, &rseq_cs))
+ if (!regs || !in_rseq_cs(ip, &rseq_cs))
return clear_rseq_cs(t);
ret = rseq_need_restart(t, rseq_cs.flags);
if (ret <= 0)
--
2.33.0.rc1.237.g0d66db33f3-goog
^ permalink raw reply related
* Re: [PATCH kernel] KVM: PPC: Book3S HV: Make unique debugfs nodename
From: Fabiano Rosas @ 2021-08-17 22:20 UTC (permalink / raw)
To: Alexey Kardashevskiy, Paul Mackerras; +Cc: linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <be02290c-60a0-48af-0491-61e8a6d5b7b7@ozlabs.ru>
Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> On 07/07/2021 14:13, Alexey Kardashevskiy wrote:
> alternatively move this debugfs stuff under the platform-independent
> directory, how about that?
That's a good idea. I only now realized we have two separate directories
for the same guest:
$ ls /sys/kernel/debug/kvm/ | grep $pid
19062-11
vm19062
Looks like we would have to implement kvm_arch_create_vcpu_debugfs for
the vcpu information and add a similar hook for the vm.
>> ---
>> arch/powerpc/kvm/book3s_hv.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 1d1fcc290fca..0223ddc0eed0 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -5227,7 +5227,7 @@ static int kvmppc_core_init_vm_hv(struct kvm *kvm)
>> /*
>> * Create a debugfs directory for the VM
>> */
>> - snprintf(buf, sizeof(buf), "vm%d", current->pid);
>> + snprintf(buf, sizeof(buf), "vm%d-lp%ld", current->pid, lpid);
>> kvm->arch.debugfs_dir = debugfs_create_dir(buf, kvm_debugfs_dir);
>> kvmppc_mmu_debugfs_init(kvm);
>> if (radix_enabled())
>>
^ permalink raw reply
* Re: [PATCH v2 06/12] x86/sev: Replace occurrences of sev_active() with prot_guest_has()
From: Borislav Petkov @ 2021-08-17 18:43 UTC (permalink / raw)
To: Tom Lendacky
Cc: Sathyanarayanan Kuppuswamy, linux-efi, Brijesh Singh, kvm,
Peter Zijlstra, Dave Hansen, dri-devel, platform-driver-x86,
Ard Biesheuvel, 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: <b346ae1b-dbd3-cdbd-b5cd-b5ab9c304737@amd.com>
On Tue, Aug 17, 2021 at 10:26:18AM -0500, Tom Lendacky wrote:
> >> /*
> >> - * If SME is active we need to be sure that kexec pages are
> >> - * not encrypted because when we boot to the new kernel the
> >> + * If host memory encryption is active we need to be sure that kexec
> >> + * pages are not encrypted because when we boot to the new kernel the
> >> * pages won't be accessed encrypted (initially).
> >> */
> >
> > That hunk belongs logically into the previous patch which removes
> > sme_active().
>
> I was trying to keep the sev_active() changes separate... so even though
> it's an SME thing, I kept it here. But I can move it to the previous
> patch, it just might look strange.
Oh I meant only the comment because it is a SME-related change. But not
too important so whatever.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v2 05/12] x86/sme: Replace occurrences of sme_active() with prot_guest_has()
From: Borislav Petkov @ 2021-08-17 18:41 UTC (permalink / raw)
To: Tom Lendacky
Cc: Sathyanarayanan Kuppuswamy, linux-efi, Brijesh Singh, kvm,
Peter Zijlstra, Dave Hansen, dri-devel, platform-driver-x86,
Will Deacon, 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: <2996b1c8-1ea1-0e56-3aad-08b46fc207f0@amd.com>
On Tue, Aug 17, 2021 at 09:46:58AM -0500, Tom Lendacky wrote:
> I'm ok with letting the TDX folks make changes to these calls to be SME or
> SEV specific, if necessary, later.
Yap, exactly. Let's add the specific stuff only when really needed.
Thx.
--
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: Borislav Petkov @ 2021-08-17 18: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: <f4b8bc3c-5158-cd04-6e12-77f08036ea19@amd.com>
On Tue, Aug 17, 2021 at 10:22:52AM -0500, Tom Lendacky wrote:
> I can change it to be an AMD/HYGON check... although, I'll have to check
> to see if any (very) early use of the function will work with that.
We can always change it later if really needed. It is just that I'm not
a fan of such "preemptive" changes.
> At a minimum, the check in arch/x86/kernel/head64.c will have to be
> changed or removed. I'll take a closer look.
Yeah, sme_me_mask, already discussed on IRC.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars()
From: kernel test robot @ 2021-08-17 18:24 UTC (permalink / raw)
To: Xianting Tian, gregkh, jirislaby, amit, arnd, osandov
Cc: kbuild-all, Xianting Tian, linux-kernel, virtualization,
clang-built-linux, linuxppc-dev
In-Reply-To: <20210817132300.165014-2-xianting.tian@linux.alibaba.com>
[-- Attachment #1: Type: text/plain, Size: 4844 bytes --]
Hi Xianting,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on char-misc/char-misc-testing soc/for-next v5.14-rc6 next-20210817]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Xianting-Tian/make-hvc-pass-dma-capable-memory-to-its-backend/20210817-212556
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: i386-randconfig-r021-20210816 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 2c6448cdc2f68f8c28fd0bd9404182b81306e6e6)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/f12c3bee9f2413ed7643d858b40ce2337329fdae
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Xianting-Tian/make-hvc-pass-dma-capable-memory-to-its-backend/20210817-212556
git checkout f12c3bee9f2413ed7643d858b40ce2337329fdae
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
clang-14: warning: optimization flag '-falign-jumps=0' is not supported [-Wignored-optimization-argument]
In file included from drivers/tty/hvc/hvc_console.c:15:
In file included from include/linux/kbd_kern.h:5:
In file included from include/linux/tty.h:5:
In file included from include/linux/fs.h:6:
In file included from include/linux/wait_bit.h:8:
In file included from include/linux/wait.h:9:
In file included from include/linux/spinlock.h:51:
In file included from include/linux/preempt.h:78:
In file included from arch/x86/include/asm/preempt.h:7:
In file included from include/linux/thread_info.h:60:
arch/x86/include/asm/thread_info.h:172:13: warning: calling '__builtin_frame_address' with a nonzero argument is unsafe [-Wframe-address]
oldframe = __builtin_frame_address(1);
^~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/include/asm/thread_info.h:174:11: warning: calling '__builtin_frame_address' with a nonzero argument is unsafe [-Wframe-address]
frame = __builtin_frame_address(2);
^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/tty/hvc/hvc_console.c:160:18: warning: address of array 'hp->c' will always evaluate to 'true' [-Wpointer-bool-conversion]
if (!hp || !hp->c)
~~~~~^
3 warnings generated.
vim +160 drivers/tty/hvc/hvc_console.c
136
137 /*
138 * Console APIs, NOT TTY. These APIs are available immediately when
139 * hvc_console_setup() finds adapters.
140 */
141
142 static void hvc_console_print(struct console *co, const char *b,
143 unsigned count)
144 {
145 char *c;
146 unsigned i = 0, n = 0;
147 int r, donecr = 0, index = co->index;
148 unsigned long flags;
149 struct hvc_struct *hp;
150
151 /* Console access attempt outside of acceptable console range. */
152 if (index >= MAX_NR_HVC_CONSOLES)
153 return;
154
155 /* This console adapter was removed so it is not usable. */
156 if (vtermnos[index] == -1)
157 return;
158
159 hp = cons_hvcs[index];
> 160 if (!hp || !hp->c)
161 return;
162
163 c = hp->c;
164
165 spin_lock_irqsave(&hp->c_lock, flags);
166 while (count > 0 || i > 0) {
167 if (count > 0 && i < sizeof(c)) {
168 if (b[n] == '\n' && !donecr) {
169 c[i++] = '\r';
170 donecr = 1;
171 } else {
172 c[i++] = b[n++];
173 donecr = 0;
174 --count;
175 }
176 } else {
177 r = cons_ops[index]->put_chars(vtermnos[index], c, i);
178 if (r <= 0) {
179 /* throw away characters on error
180 * but spin in case of -EAGAIN */
181 if (r != -EAGAIN) {
182 i = 0;
183 } else {
184 hvc_console_flush(cons_ops[index],
185 vtermnos[index]);
186 }
187 } else if (r > 0) {
188 i -= r;
189 if (i > 0)
190 memmove(c, c+r, i);
191 }
192 }
193 }
194 spin_unlock_irqrestore(&hp->c_lock, flags);
195 hvc_console_flush(cons_ops[index], vtermnos[index]);
196 }
197
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36668 bytes --]
^ permalink raw reply
* Re: [PATCH] macintosh: no need to initilise statics to 0
From: Joe Perches @ 2021-08-17 18:05 UTC (permalink / raw)
To: Christophe Leroy, Jason Wang, benh; +Cc: yukuai3, linuxppc-dev, linux-kernel
In-Reply-To: <2105ef52-b736-cc18-def9-02ac174d1922@csgroup.eu>
On Tue, 2021-08-17 at 13:59 +0200, Christophe Leroy wrote:
>
> Le 17/08/2021 à 13:51, Jason Wang a écrit :
> > Global static variables dont need to be initialised to 0. Because
> > the compiler will initilise them.
>
> It is not the compiler, it is the Kernel. It is done here:
>
> https://elixir.bootlin.com/linux/v5.14-rc6/source/arch/powerpc/kernel/early_32.c
I don't know why that's done generally.
From memory, it's also required by the c spec unless it's for a union
where the first union member is smaller in size than other members.
^ permalink raw reply
* Re: [PATCH] powerpc/32s: Fix random crashes by adding isync() after locking/unlocking KUEP
From: Segher Boessenkool @ 2021-08-17 18:03 UTC (permalink / raw)
To: Christophe Leroy
Cc: userm57, fthain, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <0426a0d3-bdc6-1a34-1018-71b34282a6c6@csgroup.eu>
Hi!
On Tue, Aug 17, 2021 at 07:13:44PM +0200, Christophe Leroy wrote:
> Le 17/08/2021 à 18:22, Segher Boessenkool a écrit :
> >On Tue, Aug 17, 2021 at 02:43:15PM +0000, Christophe Leroy wrote:
> >>Commit b5efec00b671 ("powerpc/32s: Move KUEP locking/unlocking in C")
> >>removed the 'isync' instruction after adding/removing NX bit in user
> >>segments. The reasoning behind this change was that when setting the
> >>NX bit we don't mind it taking effect with delay as the kernel never
> >>executes text from userspace, and when clearing the NX bit this is
> >>to return to userspace and then the 'rfi' should synchronise the
> >>context.
> >>
> >>However, it looks like on book3s/32 having a hash page table, at least
> >>on the G3 processor, we get an unexpected fault from userspace, then
> >>this is followed by something wrong in the verification of MSR_PR
> >>at end of another interrupt.
> >>
> >>This is fixed by adding back the removed isync() following update
> >>of NX bit in user segment registers. Only do it for cores with an
> >>hash table, as 603 cores don't exhibit that problem and the two isync
> >>increase ./null_syscall selftest by 6 cycles on an MPC 832x.
> >>
> >>First problem: unexpected PROTFAULT
> >>
> >> [ 62.896426] WARNING: CPU: 0 PID: 1660 at
> >> arch/powerpc/mm/fault.c:354 do_page_fault+0x6c/0x5b0
> >> [ 62.918111] Modules linked in:
> >> [ 62.923350] CPU: 0 PID: 1660 Comm: Xorg Not tainted
> >> 5.13.0-pmac-00028-gb3c15b60339a #40
> >> [ 62.943476] NIP: c001b5c8 LR: c001b6f8 CTR: 00000000
> >> [ 62.954714] REGS: e2d09e40 TRAP: 0700 Not tainted
> >> (5.13.0-pmac-00028-gb3c15b60339a)
> >
> >That is not a protection fault. What causes this?
>
> That's the WARN_ON(error_code & DSISR_PROTFAULT) at
>
> https://elixir.bootlin.com/linux/v5.13/source/arch/powerpc/mm/fault.c#L354
Ah okay. How confusing :-/
> >A CSI (like isync) is required both before and after mtsr. It may work
> >on some cores without -- what part of that is luck, if there is anything
> >that guarantees it, is anyone's guess :-/
>
> kuep_lock() is called when entering interrupts, it means we recently got an
> 'rfi' to re-enable MMU.
> kuep_unlock() is called when exit interrupts, it means we are soon going to
> call 'rfi' to go back to user.
>
> In between, nobody is going to exec any userspace code, so who minds that
> the 'mtsr' changing user segments is not completely finished ?
Hey, that is my question! :-)
So why does this not work on 750 then?
> >>@@ -28,6 +30,8 @@ static inline void kuep_lock(void)
> >> return;
> >>
> >> update_user_segments(mfsr(0) | SR_NX);
> >>+ if (mmu_has_feature(MMU_FTR_HPTE_TABLE))
> >>+ isync(); /* Context sync required after mtsr() */
> >> }
> >
> >This needs a comment why you are not doing this for systems without
> >hardware page table walk, at the least?
>
> Ok, will add a comment tomorrow.
Thanks!
Segher
^ permalink raw reply
* Re: [PATCH] macintosh: no need to initilise statics to 0
From: Segher Boessenkool @ 2021-08-17 17:44 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, linux-kernel, Jason Wang, yukuai3
In-Reply-To: <2105ef52-b736-cc18-def9-02ac174d1922@csgroup.eu>
On Tue, Aug 17, 2021 at 01:59:33PM +0200, Christophe Leroy wrote:
>
>
> Le 17/08/2021 à 13:51, Jason Wang a écrit :
> >Global static variables dont need to be initialised to 0. Because
> >the compiler will initilise them.
>
> It is not the compiler, it is the Kernel. It is done here:
>
> https://elixir.bootlin.com/linux/v5.14-rc6/source/arch/powerpc/kernel/early_32.c
This implements part of the C language: everything with static storage
duration (which includes all global objects, not just those marked
"static") is initialized.
> Among those 44 changes, only 2 are related to the commit's description.
Yeah. Don't do that please :-/
Segher
^ permalink raw reply
* Re: [PATCH] powerpc/32s: Fix random crashes by adding isync() after locking/unlocking KUEP
From: Christophe Leroy @ 2021-08-17 17:13 UTC (permalink / raw)
To: Segher Boessenkool
Cc: userm57, fthain, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <20210817162239.GF1583@gate.crashing.org>
Le 17/08/2021 à 18:22, Segher Boessenkool a écrit :
> On Tue, Aug 17, 2021 at 02:43:15PM +0000, Christophe Leroy wrote:
>> Commit b5efec00b671 ("powerpc/32s: Move KUEP locking/unlocking in C")
>> removed the 'isync' instruction after adding/removing NX bit in user
>> segments. The reasoning behind this change was that when setting the
>> NX bit we don't mind it taking effect with delay as the kernel never
>> executes text from userspace, and when clearing the NX bit this is
>> to return to userspace and then the 'rfi' should synchronise the
>> context.
>>
>> However, it looks like on book3s/32 having a hash page table, at least
>> on the G3 processor, we get an unexpected fault from userspace, then
>> this is followed by something wrong in the verification of MSR_PR
>> at end of another interrupt.
>>
>> This is fixed by adding back the removed isync() following update
>> of NX bit in user segment registers. Only do it for cores with an
>> hash table, as 603 cores don't exhibit that problem and the two isync
>> increase ./null_syscall selftest by 6 cycles on an MPC 832x.
>>
>> First problem: unexpected PROTFAULT
>>
>> [ 62.896426] WARNING: CPU: 0 PID: 1660 at arch/powerpc/mm/fault.c:354 do_page_fault+0x6c/0x5b0
>> [ 62.918111] Modules linked in:
>> [ 62.923350] CPU: 0 PID: 1660 Comm: Xorg Not tainted 5.13.0-pmac-00028-gb3c15b60339a #40
>> [ 62.943476] NIP: c001b5c8 LR: c001b6f8 CTR: 00000000
>> [ 62.954714] REGS: e2d09e40 TRAP: 0700 Not tainted (5.13.0-pmac-00028-gb3c15b60339a)
>
> That is not a protection fault. What causes this?
That's the WARN_ON(error_code & DSISR_PROTFAULT) at
https://elixir.bootlin.com/linux/v5.13/source/arch/powerpc/mm/fault.c#L354
>
> A CSI (like isync) is required both before and after mtsr. It may work
> on some cores without -- what part of that is luck, if there is anything
> that guarantees it, is anyone's guess :-/
kuep_lock() is called when entering interrupts, it means we recently got an 'rfi' to re-enable MMU.
kuep_unlock() is called when exit interrupts, it means we are soon going to call 'rfi' to go back to
user.
In between, nobody is going to exec any userspace code, so who minds that the 'mtsr' changing user
segments is not completely finished ?
>
>> @@ -28,6 +30,8 @@ static inline void kuep_lock(void)
>> return;
>>
>> update_user_segments(mfsr(0) | SR_NX);
>> + if (mmu_has_feature(MMU_FTR_HPTE_TABLE))
>> + isync(); /* Context sync required after mtsr() */
>> }
>
> This needs a comment why you are not doing this for systems without
> hardware page table walk, at the least?
Ok, will add a comment tomorrow.
Christophe
^ permalink raw reply
* Re: [PATCH] powerpc/32s: Fix random crashes by adding isync() after locking/unlocking KUEP
From: Segher Boessenkool @ 2021-08-17 16:22 UTC (permalink / raw)
To: Christophe Leroy
Cc: userm57, fthain, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <1d28441dd80845e6428d693c0724cb6457247466.1629211378.git.christophe.leroy@csgroup.eu>
On Tue, Aug 17, 2021 at 02:43:15PM +0000, Christophe Leroy wrote:
> Commit b5efec00b671 ("powerpc/32s: Move KUEP locking/unlocking in C")
> removed the 'isync' instruction after adding/removing NX bit in user
> segments. The reasoning behind this change was that when setting the
> NX bit we don't mind it taking effect with delay as the kernel never
> executes text from userspace, and when clearing the NX bit this is
> to return to userspace and then the 'rfi' should synchronise the
> context.
>
> However, it looks like on book3s/32 having a hash page table, at least
> on the G3 processor, we get an unexpected fault from userspace, then
> this is followed by something wrong in the verification of MSR_PR
> at end of another interrupt.
>
> This is fixed by adding back the removed isync() following update
> of NX bit in user segment registers. Only do it for cores with an
> hash table, as 603 cores don't exhibit that problem and the two isync
> increase ./null_syscall selftest by 6 cycles on an MPC 832x.
>
> First problem: unexpected PROTFAULT
>
> [ 62.896426] WARNING: CPU: 0 PID: 1660 at arch/powerpc/mm/fault.c:354 do_page_fault+0x6c/0x5b0
> [ 62.918111] Modules linked in:
> [ 62.923350] CPU: 0 PID: 1660 Comm: Xorg Not tainted 5.13.0-pmac-00028-gb3c15b60339a #40
> [ 62.943476] NIP: c001b5c8 LR: c001b6f8 CTR: 00000000
> [ 62.954714] REGS: e2d09e40 TRAP: 0700 Not tainted (5.13.0-pmac-00028-gb3c15b60339a)
That is not a protection fault. What causes this?
A CSI (like isync) is required both before and after mtsr. It may work
on some cores without -- what part of that is luck, if there is anything
that guarantees it, is anyone's guess :-/
> @@ -28,6 +30,8 @@ static inline void kuep_lock(void)
> return;
>
> update_user_segments(mfsr(0) | SR_NX);
> + if (mmu_has_feature(MMU_FTR_HPTE_TABLE))
> + isync(); /* Context sync required after mtsr() */
> }
This needs a comment why you are not doing this for systems without
hardware page table walk, at the least?
Segher
^ permalink raw reply
* [PATCH] powerpc/ptrace: Make user_mode() common to PPC32 and PPC64
From: Christophe Leroy @ 2021-08-17 16:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
Today we have:
#ifdef __powerpc64__
#define user_mode(regs) ((((regs)->msr) >> MSR_PR_LG) & 0x1)
#else
#define user_mode(regs) (((regs)->msr & MSR_PR) != 0)
#endif
With ppc64_defconfig, we get:
if (!user_mode(regs))
14b4: e9 3e 01 08 ld r9,264(r30)
14b8: 71 29 40 00 andi. r9,r9,16384
14bc: 41 82 07 a4 beq 1c60 <.emulate_instruction+0x7d0>
If taking the ppc32 definition of user_mode(), the exact same code
is generated for ppc64_defconfig.
So, only keep one version of user_mode(), preferably the one not
using MSR_PR_LG which should be kept internal to reg.h.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/ptrace.h | 4 ----
1 file changed, 4 deletions(-)
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 14422e851494..fd60538737a0 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -197,11 +197,7 @@ static inline unsigned long frame_pointer(struct pt_regs *regs)
return 0;
}
-#ifdef __powerpc64__
-#define user_mode(regs) ((((regs)->msr) >> MSR_PR_LG) & 0x1)
-#else
#define user_mode(regs) (((regs)->msr & MSR_PR) != 0)
-#endif
#define force_successful_syscall_return() \
do { \
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars()
From: Greg KH @ 2021-08-17 15:48 UTC (permalink / raw)
To: Xianting Tian
Cc: arnd, amit, jirislaby, linux-kernel, virtualization, linuxppc-dev,
osandov
In-Reply-To: <20210817132300.165014-2-xianting.tian@linux.alibaba.com>
On Tue, Aug 17, 2021 at 09:22:59PM +0800, Xianting Tian wrote:
> We tested the patch, it worked normally.
Who is "we"?
> Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
Like I said before, I need another developer from your company to review
and sign-off on this patch (and the other one), before I am willing to
look at it, based on the previous mistakes that have happened here.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2 09/12] mm: Remove the now unused mem_encrypt_active() function
From: Tom Lendacky @ 2021-08-17 15:30 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: <YRuOVOdxOZm0S86j@zn.tnic>
On 8/17/21 5:24 AM, Borislav Petkov wrote:
> On Tue, Aug 17, 2021 at 12:22:33PM +0200, Borislav Petkov wrote:
>> This one wants to be part of the previous patch.
>
> ... and the three following patches too - the treewide patch does a
> single atomic :) replacement and that's it.
Ok, I'll squash those all together.
Thanks,
Tom
>
^ permalink raw reply
* Re: [PATCH v2 06/12] x86/sev: Replace occurrences of sev_active() with prot_guest_has()
From: Tom Lendacky @ 2021-08-17 15:26 UTC (permalink / raw)
To: Borislav Petkov
Cc: Sathyanarayanan Kuppuswamy, linux-efi, Brijesh Singh, kvm,
Peter Zijlstra, Dave Hansen, dri-devel, platform-driver-x86,
Ard Biesheuvel, 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: <YRuJPqxFZ6ItZd++@zn.tnic>
On 8/17/21 5:02 AM, Borislav Petkov wrote:
> On Fri, Aug 13, 2021 at 11:59:25AM -0500, Tom Lendacky wrote:
>> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
>> index 8e7b517ad738..66ff788b79c9 100644
>> --- a/arch/x86/kernel/machine_kexec_64.c
>> +++ b/arch/x86/kernel/machine_kexec_64.c
>> @@ -167,7 +167,7 @@ static int init_transition_pgtable(struct kimage *image, pgd_t *pgd)
>> }
>> pte = pte_offset_kernel(pmd, vaddr);
>>
>> - if (sev_active())
>> + if (prot_guest_has(PATTR_GUEST_MEM_ENCRYPT))
>> prot = PAGE_KERNEL_EXEC;
>>
>> set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, prot));
>> @@ -207,7 +207,7 @@ static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
>> level4p = (pgd_t *)__va(start_pgtable);
>> clear_page(level4p);
>>
>> - if (sev_active()) {
>> + if (prot_guest_has(PATTR_GUEST_MEM_ENCRYPT)) {
>> info.page_flag |= _PAGE_ENC;
>> info.kernpg_flag |= _PAGE_ENC;
>> }
>> @@ -570,12 +570,12 @@ void arch_kexec_unprotect_crashkres(void)
>> */
>> int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, gfp_t gfp)
>> {
>> - if (sev_active())
>> + if (!prot_guest_has(PATTR_HOST_MEM_ENCRYPT))
>> return 0;
>>
>> /*
>> - * If SME is active we need to be sure that kexec pages are
>> - * not encrypted because when we boot to the new kernel the
>> + * If host memory encryption is active we need to be sure that kexec
>> + * pages are not encrypted because when we boot to the new kernel the
>> * pages won't be accessed encrypted (initially).
>> */
>
> That hunk belongs logically into the previous patch which removes
> sme_active().
I was trying to keep the sev_active() changes separate... so even though
it's an SME thing, I kept it here. But I can move it to the previous
patch, it just might look strange.
>
>> return set_memory_decrypted((unsigned long)vaddr, pages);
>> @@ -583,12 +583,12 @@ int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, gfp_t gfp)
>>
>> void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages)
>> {
>> - if (sev_active())
>> + if (!prot_guest_has(PATTR_HOST_MEM_ENCRYPT))
>> return;
>>
>> /*
>> - * If SME is active we need to reset the pages back to being
>> - * an encrypted mapping before freeing them.
>> + * If host memory encryption is active we need to reset the pages back
>> + * to being an encrypted mapping before freeing them.
>> */
>> set_memory_encrypted((unsigned long)vaddr, pages);
>> }
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index e8ccab50ebf6..b69f5ac622d5 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -25,6 +25,7 @@
>> #include <linux/pagemap.h>
>> #include <linux/swap.h>
>> #include <linux/rwsem.h>
>> +#include <linux/protected_guest.h>
>>
>> #include <asm/apic.h>
>> #include <asm/perf_event.h>
>> @@ -457,7 +458,7 @@ static int has_svm(void)
>> return 0;
>> }
>>
>> - if (sev_active()) {
>> + if (prot_guest_has(PATTR_SEV)) {
>> pr_info("KVM is unsupported when running as an SEV guest\n");
>> return 0;
>
> Same question as for PATTR_SME. PATTR_GUEST_MEM_ENCRYPT should be enough.
Yup, I'll change them all.
>
>> @@ -373,7 +373,7 @@ int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size)
>> * up under SME the trampoline area cannot be encrypted, whereas under SEV
>> * the trampoline area must be encrypted.
>> */
>> -bool sev_active(void)
>> +static bool sev_active(void)
>> {
>> return sev_status & MSR_AMD64_SEV_ENABLED;
>> }
>> @@ -382,7 +382,6 @@ static bool sme_active(void)
>> {
>> return sme_me_mask && !sev_active();
>> }
>> -EXPORT_SYMBOL_GPL(sev_active);
>
> Just get rid of it altogether.
Ok.
Thanks,
Tom
>
> Thx.
>
^ permalink raw reply
* Re: [PATCH v2 03/12] x86/sev: Add an x86 version of prot_guest_has()
From: Tom Lendacky @ 2021-08-17 15:22 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: <YRknDQGUJJ/j9pth@zn.tnic>
On 8/15/21 9:39 AM, Borislav Petkov wrote:
> 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.
Maybe the KVM folks have a better understanding of it...
I can change it to be an AMD/HYGON check... although, I'll have to check
to see if any (very) early use of the function will work with that.
At a minimum, the check in arch/x86/kernel/head64.c will have to be
changed or removed. I'll take a closer look.
Thanks,
Tom
>
^ permalink raw reply
* Re: [PATCH v2 05/12] x86/sme: Replace occurrences of sme_active() with prot_guest_has()
From: Tom Lendacky @ 2021-08-17 14:46 UTC (permalink / raw)
To: Borislav Petkov
Cc: Sathyanarayanan Kuppuswamy, linux-efi, Brijesh Singh, kvm,
Peter Zijlstra, Dave Hansen, dri-devel, platform-driver-x86,
Will Deacon, 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: <YRt6yCNCBLwyyx5X@zn.tnic>
On 8/17/21 4:00 AM, Borislav Petkov wrote:
> On Fri, Aug 13, 2021 at 11:59:24AM -0500, Tom Lendacky wrote:
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index edc67ddf065d..5635ca9a1fbe 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -144,7 +144,7 @@ void __init sme_unmap_bootdata(char *real_mode_data)
>> struct boot_params *boot_data;
>> unsigned long cmdline_paddr;
>>
>> - if (!sme_active())
>> + if (!amd_prot_guest_has(PATTR_SME))
>> return;
>>
>> /* Get the command line address before unmapping the real_mode_data */
>> @@ -164,7 +164,7 @@ void __init sme_map_bootdata(char *real_mode_data)
>> struct boot_params *boot_data;
>> unsigned long cmdline_paddr;
>>
>> - if (!sme_active())
>> + if (!amd_prot_guest_has(PATTR_SME))
>> return;
>>
>> __sme_early_map_unmap_mem(real_mode_data, sizeof(boot_params), true);
>> @@ -378,7 +378,7 @@ bool sev_active(void)
>> return sev_status & MSR_AMD64_SEV_ENABLED;
>> }
>>
>> -bool sme_active(void)
>> +static bool sme_active(void)
>
> Just get rid of it altogether. Also, there's an
>
> EXPORT_SYMBOL_GPL(sev_active);
> > which needs to go under the actual function. Here's a diff ontop:
Will do.
>
> ---
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 5635ca9a1fbe..a3a2396362a5 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -364,8 +364,9 @@ int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size)
> /*
> * SME and SEV are very similar but they are not the same, so there are
> * times that the kernel will need to distinguish between SME and SEV. The
> - * sme_active() and sev_active() functions are used for this. When a
> - * distinction isn't needed, the mem_encrypt_active() function can be used.
> + * PATTR_HOST_MEM_ENCRYPT and PATTR_GUEST_MEM_ENCRYPT flags to
> + * amd_prot_guest_has() are used for this. When a distinction isn't needed,
> + * the mem_encrypt_active() function can be used.
> *
> * The trampoline code is a good example for this requirement. Before
> * paging is activated, SME will access all memory as decrypted, but SEV
> @@ -377,11 +378,6 @@ bool sev_active(void)
> {
> return sev_status & MSR_AMD64_SEV_ENABLED;
> }
> -
> -static bool sme_active(void)
> -{
> - return sme_me_mask && !sev_active();
> -}
> EXPORT_SYMBOL_GPL(sev_active);
>
> /* Needs to be called from non-instrumentable code */
> @@ -398,7 +394,7 @@ bool amd_prot_guest_has(unsigned int attr)
>
> case PATTR_SME:
> case PATTR_HOST_MEM_ENCRYPT:
> - return sme_active();
> + return sme_me_mask && !sev_active();
>
> case PATTR_SEV:
> case PATTR_GUEST_MEM_ENCRYPT:
>
>> {
>> return sme_me_mask && !sev_active();
>> }
>> @@ -428,7 +428,7 @@ bool force_dma_unencrypted(struct device *dev)
>> * device does not support DMA to addresses that include the
>> * encryption mask.
>> */
>> - if (sme_active()) {
>> + if (amd_prot_guest_has(PATTR_SME)) {
>
> So I'm not sure: you add PATTR_SME which you call with
> amd_prot_guest_has() and PATTR_HOST_MEM_ENCRYPT which you call with
> prot_guest_has() and they both end up being the same thing on AMD.
>
> So why even bother with PATTR_SME?
>
> This is only going to cause confusion later and I'd say let's simply use
> prot_guest_has(PATTR_HOST_MEM_ENCRYPT) everywhere...
Ok, I can do that. I was trying to ensure that anything that is truly SME
or SEV specific would be called out now.
I'm ok with letting the TDX folks make changes to these calls to be SME or
SEV specific, if necessary, later.
Thanks,
Tom
>
^ permalink raw reply
* [PATCH] powerpc/32s: Fix random crashes by adding isync() after locking/unlocking KUEP
From: Christophe Leroy @ 2021-08-17 14:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, userm57,
fthain
Cc: linuxppc-dev, linux-kernel
Commit b5efec00b671 ("powerpc/32s: Move KUEP locking/unlocking in C")
removed the 'isync' instruction after adding/removing NX bit in user
segments. The reasoning behind this change was that when setting the
NX bit we don't mind it taking effect with delay as the kernel never
executes text from userspace, and when clearing the NX bit this is
to return to userspace and then the 'rfi' should synchronise the
context.
However, it looks like on book3s/32 having a hash page table, at least
on the G3 processor, we get an unexpected fault from userspace, then
this is followed by something wrong in the verification of MSR_PR
at end of another interrupt.
This is fixed by adding back the removed isync() following update
of NX bit in user segment registers. Only do it for cores with an
hash table, as 603 cores don't exhibit that problem and the two isync
increase ./null_syscall selftest by 6 cycles on an MPC 832x.
First problem: unexpected PROTFAULT
[ 62.896426] WARNING: CPU: 0 PID: 1660 at arch/powerpc/mm/fault.c:354 do_page_fault+0x6c/0x5b0
[ 62.918111] Modules linked in:
[ 62.923350] CPU: 0 PID: 1660 Comm: Xorg Not tainted 5.13.0-pmac-00028-gb3c15b60339a #40
[ 62.943476] NIP: c001b5c8 LR: c001b6f8 CTR: 00000000
[ 62.954714] REGS: e2d09e40 TRAP: 0700 Not tainted (5.13.0-pmac-00028-gb3c15b60339a)
[ 62.974581] MSR: 00021032 <ME,IR,DR,RI> CR: 42d04f30 XER: 20000000
[ 62.990009]
GPR00: c000424c e2d09f00 c301b680 e2d09f40 0000001e 42000000 00cba028 00000000
GPR08: 08000000 48000010 c301b680 e2d09f30 22d09f30 00c1fff0 00cba000 a7b7ba4c
GPR16: 00000031 00000000 00000000 00000000 00000000 00000000 a7b7b0d0 00c5c010
GPR24: a7b7b64c a7b7d2f0 00000004 00000000 c1efa6c0 00cba02c 00000300 e2d09f40
[ 63.075238] NIP [c001b5c8] do_page_fault+0x6c/0x5b0
[ 63.085952] LR [c001b6f8] do_page_fault+0x19c/0x5b0
[ 63.096678] Call Trace:
[ 63.100075] [e2d09f00] [e2d09f04] 0xe2d09f04 (unreliable)
[ 63.112359] [e2d09f30] [c000424c] DataAccess_virt+0xd4/0xe4
[ 63.125168] --- interrupt: 300 at 0xa7a261dc
[ 63.134060] NIP: a7a261dc LR: a7a253bc CTR: 00000000
[ 63.145302] REGS: e2d09f40 TRAP: 0300 Not tainted (5.13.0-pmac-00028-gb3c15b60339a)
[ 63.165167] MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 228428e2 XER: 20000000
[ 63.182162] DAR: 00cba02c DSISR: 42000000
GPR00: a7a27448 afa6b0e0 a74c35c0 a7b7b614 0000001e a7b7b614 00cba028 00000000
GPR08: 00020fd9 00000031 00cb9ff8 a7a273b0 220028e2 00c1fff0 00cba000 a7b7ba4c
GPR16: 00000031 00000000 00000000 00000000 00000000 00000000 a7b7b0d0 00c5c010
GPR24: a7b7b64c a7b7d2f0 00000004 00000002 0000001e a7b7b614 a7b7aff4 00000030
[ 63.275233] NIP [a7a261dc] 0xa7a261dc
[ 63.282291] LR [a7a253bc] 0xa7a253bc
[ 63.289087] --- interrupt: 300
[ 63.294322] Instruction dump:
[ 63.299291] 7c4a1378 810300a0 75278410 83820298 83a300a4 553b018c 551e0036 4082038c
[ 63.318630] 2e1b0000 40920228 75280800 41820220 <0fe00000> 3b600000 41920214 81420594
[ 63.338503] ---[ end trace f642a84639cba377 ]---
Second problem: MSR PR is seen unset allthough the interrupt frame shows it set
[ 63.666846] kernel BUG at arch/powerpc/kernel/interrupt.c:458!
[ 63.680633] Oops: Exception in kernel mode, sig: 5 [#1]
[ 63.692201] BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 PowerMac
[ 63.705011] Modules linked in:
[ 63.710247] CPU: 0 PID: 1660 Comm: Xorg Tainted: G W 5.13.0-pmac-00028-gb3c15b60339a #40
[ 63.734553] NIP: c0011434 LR: c001629c CTR: 00000000
[ 63.745796] REGS: e2d09e70 TRAP: 0700 Tainted: G W (5.13.0-pmac-00028-gb3c15b60339a)
[ 63.769844] MSR: 00029032 <EE,ME,IR,DR,RI> CR: 42d09f30 XER: 00000000
[ 63.786052]
GPR00: 00000000 e2d09f30 c301b680 e2d09f40 83440000 c44d0e68 e2d09e8c 00000000
GPR08: 00000002 00dc228a 00004000 e2d09f30 22d09f30 00c1fff0 afa6ceb4 00c26144
GPR16: 00c25fb8 00c26140 afa6ceb8 90000000 00c944d8 0000001c 00000000 00200000
GPR24: 00000000 000001fb afa6d1b4 00000001 00000000 a539a2a0 a530fd80 00000089
[ 63.871284] NIP [c0011434] interrupt_exit_kernel_prepare+0x10/0x70
[ 63.885922] LR [c001629c] interrupt_return+0x9c/0x144
[ 63.897168] Call Trace:
[ 63.900562] [e2d09f30] [c000424c] DataAccess_virt+0xd4/0xe4 (unreliable)
[ 63.916773] --- interrupt: 300 at 0xa09be008
[ 63.925659] NIP: a09be008 LR: a09bdfe8 CTR: a09bdfc0
[ 63.936903] REGS: e2d09f40 TRAP: 0300 Tainted: G W (5.13.0-pmac-00028-gb3c15b60339a)
[ 63.960953] MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 420028e2 XER: 20000000
[ 63.977948] DAR: a539a308 DSISR: 0a000000
GPR00: a7b90d50 afa6b2d0 a74c35c0 a0a8b690 a0a8b698 a5365d70 a4fa82a8 00000004
GPR08: 00000000 a09bdfc0 00000000 a5360000 a09bde7c 00c1fff0 afa6ceb4 00c26144
GPR16: 00c25fb8 00c26140 afa6ceb8 90000000 00c944d8 0000001c 00000000 00200000
GPR24: 00000000 000001fb afa6d1b4 00000001 00000000 a539a2a0 a530fd80 00000089
[ 64.071020] NIP [a09be008] 0xa09be008
[ 64.078079] LR [a09bdfe8] 0xa09bdfe8
[ 64.084874] --- interrupt: 300
[ 64.090108] Instruction dump:
[ 64.095074] 80010024 83e1001c 7c0803a6 4bffff80 3bc00800 4bffffd0 486b42fd 4bffffcc
[ 64.114419] 81430084 71480002 41820038 554a0462 <0f0a0000> 80620060 74630001 40820034
[ 64.134298] ---[ end trace f642a84639cba378 ]---
Reported-by: Stan Johnson <userm57@yahoo.com>
Fixes: b5efec00b671 ("powerpc/32s: Move KUEP locking/unlocking in C")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/book3s/32/kup.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index 64201125a287..2658d30b223c 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -4,6 +4,8 @@
#include <asm/bug.h>
#include <asm/book3s/32/mmu-hash.h>
+#include <asm/mmu.h>
+#include <asm/synch.h>
#ifndef __ASSEMBLY__
@@ -28,6 +30,8 @@ static inline void kuep_lock(void)
return;
update_user_segments(mfsr(0) | SR_NX);
+ if (mmu_has_feature(MMU_FTR_HPTE_TABLE))
+ isync(); /* Context sync required after mtsr() */
}
static inline void kuep_unlock(void)
@@ -36,6 +40,8 @@ static inline void kuep_unlock(void)
return;
update_user_segments(mfsr(0) & ~SR_NX);
+ if (mmu_has_feature(MMU_FTR_HPTE_TABLE))
+ isync(); /* Context sync required after mtsr() */
}
#ifdef CONFIG_PPC_KUAP
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] powerpc/mm: Fix set_memory_*() against concurrent accesses
From: Christophe Leroy @ 2021-08-17 14:28 UTC (permalink / raw)
To: Fabiano Rosas, Michael Ellerman, linuxppc-dev
Cc: lvivier, jniethe5, npiggin, aneesh.kumar
In-Reply-To: <87sfz8tam3.fsf@linux.ibm.com>
Le 17/08/2021 à 16:21, Fabiano Rosas a écrit :
> Michael Ellerman <mpe@ellerman.id.au> writes:
>
> Hi, I already mentioned these things in private, but I'll post here so
> everyone can see:
>
>> Because pte_update() takes the set of PTE bits to set and clear we can't
>> use our existing helpers, eg. pte_wrprotect() etc. and instead have to
>> open code the set of flags. We will clean that up somehow in a future
>> commit.
>
> I tested the following on P9 and it seems to work fine. Not sure if it
> works for CONFIG_PPC_8xx, though.
>
>
> static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
> {
> long action = (long)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;
Maybe using ptep_get() is better.
>
> /* modify the PTE bits as desired, then apply */
> switch (action) {
> @@ -59,11 +42,9 @@ static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
> break;
> }
>
> - set_pte_at(&init_mm, addr, ptep, pte);
> + pte_update(&init_mm, addr, ptep, ~0UL, pte_val(pte), 0);
Good simple idea, indeed yes it should work with much more effort.
> + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>
> - /* See ptesync comment in radix__set_pte_at() */
> - if (radix_enabled())
> - asm volatile("ptesync": : :"memory");
> spin_unlock(&init_mm.page_table_lock);
>
> return 0;
> ---
>
> For reference, the full patch is here:
> https://github.com/farosas/linux/commit/923c95c84d7081d7be9503bf5b276dd93bd17036.patch
>
>>
>> [1]: https://lore.kernel.org/linuxppc-dev/87y318wp9r.fsf@linux.ibm.com/
>>
>> Fixes: 1f9ad21c3b38 ("powerpc/mm: Implement set_memory() routines")
>> Reported-by: Laurent Vivier <lvivier@redhat.com>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>
> ...
>
>> - set_pte_at(&init_mm, addr, ptep, pte);
>> + pte_update(&init_mm, addr, ptep, clear, set, 0);
>>
>> /* See ptesync comment in radix__set_pte_at() */
>> if (radix_enabled())
>> asm volatile("ptesync": : :"memory");
>> +
>> + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>
> I think there's an optimization possible here, when relaxing access, to
> skip the TLB flush. Would still need the ptesync though. Similar to what
> Nick did in e5f7cb58c2b7 ("powerpc/64s/radix: do not flush TLB when
> relaxing access"). It is out of scope for this patch but maybe worth
> thinking about.
>
>> +
>> spin_unlock(&init_mm.page_table_lock);
>>
>> return 0;
>>
>> base-commit: cbc06f051c524dcfe52ef0d1f30647828e226d30
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Fix set_memory_*() against concurrent accesses
From: Fabiano Rosas @ 2021-08-17 14:21 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: lvivier, jniethe5, npiggin, aneesh.kumar
In-Reply-To: <20210817132552.3375738-1-mpe@ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
Hi, I already mentioned these things in private, but I'll post here so
everyone can see:
> Because pte_update() takes the set of PTE bits to set and clear we can't
> use our existing helpers, eg. pte_wrprotect() etc. and instead have to
> open code the set of flags. We will clean that up somehow in a future
> commit.
I tested the following on P9 and it seems to work fine. Not sure if it
works for CONFIG_PPC_8xx, though.
static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
{
long action = (long)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) {
@@ -59,11 +42,9 @@ static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
break;
}
- set_pte_at(&init_mm, addr, ptep, pte);
+ pte_update(&init_mm, addr, ptep, ~0UL, pte_val(pte), 0);
+ flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
- /* See ptesync comment in radix__set_pte_at() */
- if (radix_enabled())
- asm volatile("ptesync": : :"memory");
spin_unlock(&init_mm.page_table_lock);
return 0;
---
For reference, the full patch is here:
https://github.com/farosas/linux/commit/923c95c84d7081d7be9503bf5b276dd93bd17036.patch
>
> [1]: https://lore.kernel.org/linuxppc-dev/87y318wp9r.fsf@linux.ibm.com/
>
> Fixes: 1f9ad21c3b38 ("powerpc/mm: Implement set_memory() routines")
> Reported-by: Laurent Vivier <lvivier@redhat.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
...
> - set_pte_at(&init_mm, addr, ptep, pte);
> + pte_update(&init_mm, addr, ptep, clear, set, 0);
>
> /* See ptesync comment in radix__set_pte_at() */
> if (radix_enabled())
> asm volatile("ptesync": : :"memory");
> +
> + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
I think there's an optimization possible here, when relaxing access, to
skip the TLB flush. Would still need the ptesync though. Similar to what
Nick did in e5f7cb58c2b7 ("powerpc/64s/radix: do not flush TLB when
relaxing access"). It is out of scope for this patch but maybe worth
thinking about.
> +
> spin_unlock(&init_mm.page_table_lock);
>
> return 0;
>
> base-commit: cbc06f051c524dcfe52ef0d1f30647828e226d30
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Fix set_memory_*() against concurrent accesses
From: Christophe Leroy @ 2021-08-17 14:20 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
Cc: lvivier, jniethe5, aneesh.kumar, npiggin, farosas
In-Reply-To: <20210817132552.3375738-1-mpe@ellerman.id.au>
Le 17/08/2021 à 15:25, Michael Ellerman a écrit :
> Laurent reported that STRICT_MODULE_RWX was causing intermittent crashes
> on one of his systems:
>
> kernel tried to execute exec-protected page (c008000004073278) - exploit attempt? (uid: 0)
> BUG: Unable to handle kernel instruction fetch
> Faulting instruction address: 0xc008000004073278
> Oops: Kernel access of bad area, sig: 11 [#1]
> LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
> Modules linked in: drm virtio_console fuse drm_panel_orientation_quirks ...
> CPU: 3 PID: 44 Comm: kworker/3:1 Not tainted 5.14.0-rc4+ #12
> Workqueue: events control_work_handler [virtio_console]
> NIP: c008000004073278 LR: c008000004073278 CTR: c0000000001e9de0
> REGS: c00000002e4ef7e0 TRAP: 0400 Not tainted (5.14.0-rc4+)
> MSR: 800000004280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24002822 XER: 200400cf
> ...
> NIP fill_queue+0xf0/0x210 [virtio_console]
> LR fill_queue+0xf0/0x210 [virtio_console]
> Call Trace:
> fill_queue+0xb4/0x210 [virtio_console] (unreliable)
> add_port+0x1a8/0x470 [virtio_console]
> control_work_handler+0xbc/0x1e8 [virtio_console]
> process_one_work+0x290/0x590
> worker_thread+0x88/0x620
> kthread+0x194/0x1a0
> ret_from_kernel_thread+0x5c/0x64
>
> Jordan, Fabiano & Murilo were able to reproduce and identify that the
> problem is caused by the call to module_enable_ro() in do_init_module(),
> which happens after the module's init function has already been called.
>
> Our current implementation of change_page_attr() is not safe against
> concurrent accesses, because it invalidates the PTE before flushing the
> TLB and then installing the new PTE. That leaves a window in time where
> there is no valid PTE for the page, if another CPU tries to access the
> page at that time we see something like the fault above.
>
> We can't simply switch to set_pte_at()/flush TLB, because our hash MMU
> code doesn't handle a set_pte_at() of a valid PTE. See [1].
>
> But we do have pte_update(), which replaces the old PTE with the new,
> meaning there's no window where the PTE is invalid. And the hash MMU
> version hash__pte_update() deals with synchronising the hash page table
> correctly.
>
> Because pte_update() takes the set of PTE bits to set and clear we can't
> use our existing helpers, eg. pte_wrprotect() etc. and instead have to
> open code the set of flags. We will clean that up somehow in a future
> commit.
>
> [1]: https://lore.kernel.org/linuxppc-dev/87y318wp9r.fsf@linux.ibm.com/
>
> Fixes: 1f9ad21c3b38 ("powerpc/mm: Implement set_memory() routines")
> Reported-by: Laurent Vivier <lvivier@redhat.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/mm/pageattr.c | 45 +++++++++++++++++++++++---------------
> 1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> index 0876216ceee6..72425b61eb7e 100644
> --- a/arch/powerpc/mm/pageattr.c
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -18,52 +18,61 @@
> /*
> * Updates the attributes of a page in three steps:
> *
> - * 1. invalidate the page table entry
> - * 2. flush the TLB
> - * 3. install the new entry with the updated attributes
> - *
> - * Invalidating the pte means there are situations where this will not work
> - * when in theory it should.
> - * For example:
> - * - removing write from page whilst it is being executed
> - * - setting a page read-only whilst it is being read by another CPU
> + * 1. take the page_table_lock
> + * 2. install the new entry with the updated attributes
> + * 3. flush the TLB
> *
> + * This sequence is safe against concurrent updates, and also allows updating the
> + * attributes of a page currently being executed or accessed.
> */
> static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
> {
> long action = (long)data;
> - pte_t pte;
> + unsigned long set, clear;
>
> 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);
> + set = clear = 0;
>
> /* modify the PTE bits as desired, then apply */
> switch (action) {
> case SET_MEMORY_RO:
> - pte = pte_wrprotect(pte);
> +#ifdef CONFIG_PPC_BOOK3S_64
> + clear = _PAGE_WRITE;
> +#elif defined(CONFIG_PPC_8xx)
> + set = _PAGE_RO;
> +#else
> + clear = _PAGE_RW;
> +#endif
I think it can be handle as follows (untested):
new = pte_wrprotect(pte);
set = pte_val(new) & ~pte_val(pte);
clear = ~pte_val(new) & pte_val(pte);
So just put those two lines before the pte_update() and only change the switch cases to create a
'new' pte instead of changing it.
Or you can do the way we do in ptep_set_wrprotect() in <asm/nohash/32/pgtable.h>
Or can __ptep_set_access_flags() be used ?
> break;
> case SET_MEMORY_RW:
> - pte = pte_mkwrite(pte_mkdirty(pte));
> +#ifdef CONFIG_PPC_8xx
> + clear = _PAGE_RO;
> +#elif defined(CONFIG_PPC_BOOK3S_64)
> + set = _PAGE_RW | _PAGE_DIRTY | _PAGE_SOFT_DIRTY;
> +#else
> + set = _PAGE_RW | _PAGE_DIRTY;
> +#endif
> break;
> case SET_MEMORY_NX:
> - pte = pte_exprotect(pte);
> + clear = _PAGE_EXEC;
> break;
> case SET_MEMORY_X:
> - pte = pte_mkexec(pte);
> + set = _PAGE_EXEC;
> break;
> default:
> WARN_ON_ONCE(1);
> break;
> }
>
> - set_pte_at(&init_mm, addr, ptep, pte);
> + pte_update(&init_mm, addr, ptep, clear, set, 0);
>
> /* See ptesync comment in radix__set_pte_at() */
> if (radix_enabled())
> asm volatile("ptesync": : :"memory");
> +
> + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
Can we use a page version like flush_tlb_page() in order to avoid a 'tlbia' ? (maybe another page as
it was already there).
> +
> spin_unlock(&init_mm.page_table_lock);
>
> return 0;
>
> base-commit: cbc06f051c524dcfe52ef0d1f30647828e226d30
>
^ permalink raw reply
* Re: [PATCH v2 04/12] powerpc/pseries/svm: Add a powerpc version of prot_guest_has()
From: Tom Lendacky @ 2021-08-17 14:11 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, linux-fsdevel, Tianyu Lan, linuxppc-dev,
Paul Mackerras
In-Reply-To: <YRt01F6Mw6sB+hF8@zn.tnic>
On 8/17/21 3:35 AM, Borislav Petkov wrote:
> On Fri, Aug 13, 2021 at 11:59:23AM -0500, Tom Lendacky wrote:
>> Introduce a powerpc version of the prot_guest_has() function. This will
>> be used to replace the powerpc mem_encrypt_active() implementation, so
>> the implementation will initially only support the PATTR_MEM_ENCRYPT
>> attribute.
>>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> arch/powerpc/include/asm/protected_guest.h | 30 ++++++++++++++++++++++
>> arch/powerpc/platforms/pseries/Kconfig | 1 +
>> 2 files changed, 31 insertions(+)
>> create mode 100644 arch/powerpc/include/asm/protected_guest.h
>>
>> diff --git a/arch/powerpc/include/asm/protected_guest.h b/arch/powerpc/include/asm/protected_guest.h
>> new file mode 100644
>> index 000000000000..ce55c2c7e534
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/protected_guest.h
>> @@ -0,0 +1,30 @@
>> +/* 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 _POWERPC_PROTECTED_GUEST_H
>> +#define _POWERPC_PROTECTED_GUEST_H
>> +
>> +#include <asm/svm.h>
>> +
>> +#ifndef __ASSEMBLY__
>
> Same thing here. Pls audit the whole set whether those __ASSEMBLY__
> guards are really needed and remove them if not.
Will do.
Thanks,
Tom
>
> Thx.
>
^ permalink raw reply
* [PATCH] scsi: ibmvfc: Stop using scsi_cmnd.tag
From: John Garry @ 2021-08-17 13:43 UTC (permalink / raw)
To: tyreld, mpe, benh, paulus, jejb, martin.petersen
Cc: sfr, bvanassche, linux-scsi, John Garry, linux-kernel, linux-next,
hare, linuxppc-dev, hch
Use scsi_cmd_to_rq(scsi_cmnd)->tag in preference to scsi_cmnd.tag.
Signed-off-by: John Garry <john.garry@huawei.com>
---
This patch was missed in a series to remove scsi_cmnd.tag, which caused
a build error on Martin's SCSI staging tree:
https://lore.kernel.org/linux-scsi/yq14kbppa42.fsf@ca-mkp.ca.oracle.com/T/#mb47909f38f35837686734369600051b278d124af
I note that scsi_cmnd.tag is/was an unsigned char, and I could not find
anywhere in the driver which limits scsi_host.can_queue to 255, so using
scsi_cmnd.tag looks odd to me.
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index 7fa5e64e38c3..ba7150cb226a 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -1956,7 +1956,7 @@ static int ibmvfc_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmnd)
memcpy(iu->cdb, cmnd->cmnd, cmnd->cmd_len);
if (cmnd->flags & SCMD_TAGGED) {
- vfc_cmd->task_tag = cpu_to_be64(cmnd->tag);
+ vfc_cmd->task_tag = cpu_to_be64(scsi_cmd_to_rq(cmnd)->tag);
iu->pri_task_attr = IBMVFC_SIMPLE_TASK;
}
--
2.17.1
^ permalink raw reply related
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