* Re: [PATCH 00/15] selftests/seccomp: Refactor change_syscall()
From: Michael Ellerman @ 2020-09-15 12:52 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <202009141321.366935EF52@keescook>
Kees Cook <keescook@chromium.org> writes:
> On Mon, Sep 14, 2020 at 10:15:18PM +1000, Michael Ellerman wrote:
>> Kees Cook <keescook@chromium.org> writes:
>> > Hi,
>> >
>> > This refactors the seccomp selftest macros used in change_syscall(),
>> > in an effort to remove special cases for mips, arm, arm64, and xtensa,
>> > which paves the way for powerpc fixes.
>> >
>> > I'm not entirely done testing, but all-arch build tests and x86_64
>> > selftests pass. I'll be doing arm, arm64, and i386 selftests shortly,
>> > but I currently don't have an easy way to check xtensa, mips, nor
>> > powerpc. Any help there would be appreciated!
>>
>> The series builds fine for me, and all the tests pass (see below).
>>
>> Thanks for picking up those changes to deal with powerpc being oddball.
>>
>> Tested-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
>
> Awesome; thanks!
>
> However...
>
>> ./seccomp_bpf
>> TAP version 13
>> 1..86
>> # Starting 86 tests from 7 test cases.
>> # RUN global.kcmp ...
>> # OK global.kcmp
>> ok 1 global.kcmp
>> [...]
>> # RUN global.KILL_thread ...
>> TAP version 13
>> 1..86
>> # Starting 86 tests from 7 test cases.
>
> Was this a mis-paste, or has something very very bad happened here in
> global.KILL_one_arg_six finishes?
>
...
>> TAP version 13
>> 1..86
>> # Starting 86 tests from 7 test cases.
>> [...]
>> # PASSED: 86 / 86 tests passed.
>> # Totals: pass:86 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> And after every user_notification test? O_O
Haha, I thought that was normal :)
It's because of redirection, I run the tests with:
find . -executable -type f -print -execdir '{}' ';' | tee test.log
If I just run it directly on the terminal everything is normal.
It'll be fork() vs libc buffering.
I can fix it with:
$ stdbuf -oL ./seccomp_bpf | tee test.log
Or the patch below.
I can send a proper patch for that tomorrow, I don't know that harness
code, but I think that's the right fix.
cheers
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 4f78e4805633..b1bd00ff3d94 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -971,6 +971,7 @@ void __run_test(struct __fixture_metadata *f,
ksft_print_msg(" RUN %s%s%s.%s ...\n",
f->name, variant->name[0] ? "." : "", variant->name, t->name);
+ fflush(stdout);
t->pid = fork();
if (t->pid < 0) {
ksft_print_msg("ERROR SPAWNING TEST CHILD\n");
^ permalink raw reply related
* Re: Injecting SLB miltihit crashes kernel 5.9.0-rc5
From: Michael Ellerman @ 2020-09-15 12:54 UTC (permalink / raw)
To: Michal Suchánek, linuxppc-dev, mahesh
In-Reply-To: <20200915084302.GG29778@kitsune.suse.cz>
Michal Suchánek <msuchanek@suse.de> writes:
> Hello,
>
> Using the SLB mutihit injection test module (which I did not write so I
> do not want to post it here) to verify updates on my 5.3 frankernekernel
> I found that the kernel crashes with Oops: kernel bad access.
>
> I tested on latest upstream kernel build that I have at hand and the
> result is te same (minus the message - nothing was logged and the kernel
> simply rebooted).
That's disappointing.
> Since the whole effort to write a real mode MCE handler was supposed to
> prevent this maybe the SLB injection module should be added to the
> kernel selftests?
Yes I'd like to see it upstream. I think it should be integrated into
LKDTM, which contains other dangerous things like that and is designed
for testing how the kernel handles/recovers from bad conditions.
cheers
^ permalink raw reply
* Re: [5.9.0-rc5-20200914] Kernel crash while running LTP(mlock201)
From: Matthew Wilcox @ 2020-09-15 13:09 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Sachin Sant, linux-next, linuxppc-dev
In-Reply-To: <87o8m7p9jd.fsf@mpe.ellerman.id.au>
On Tue, Sep 15, 2020 at 09:24:38PM +1000, Michael Ellerman wrote:
> Sachin Sant <sachinp@linux.vnet.ibm.com> writes:
> > While running LTP tests (specifically mlock201) against next-20200914 tree
> > on a POWER9 LPAR results in following crash.
>
> Looks the same as:
>
> https://lore.kernel.org/linux-mm/20200914085545.GB28738@shao2-debian/
https://lore.kernel.org/linux-mm/20200914112738.GM6583@casper.infradead.org/
^ permalink raw reply
* [Bug 209277] New: Dead code :q
From: bugzilla-daemon @ 2020-09-15 14:09 UTC (permalink / raw)
To: linuxppc-dev
https://bugzilla.kernel.org/show_bug.cgi?id=209277
Bug ID: 209277
Summary: Dead code :q
Product: Platform Specific/Hardware
Version: 2.5
Kernel Version: 5.9-rc4
Hardware: All
OS: Linux
Tree: Mainline
Status: NEW
Severity: enhancement
Priority: P1
Component: PPC-32
Assignee: platform_ppc-32@kernel-bugs.osdl.org
Reporter: fazilyildiran@gmail.com
Regression: No
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [Bug 209277] powerpc: obsolete driver: Marvell MV64X60 MPSC
From: bugzilla-daemon @ 2020-09-15 14:25 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-209277-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=209277
--- Comment #1 from Necip Fazil Yildiran (fazilyildiran@gmail.com) ---
The config MV64X60 in arch/powerpc/platforms/embedded6xx/Kconfig is
non-prompt selected nowhere -- thus, cannot be enabled.
In addition, a few other configs cannot be enabled due to their dependency
on MV64X60, e.g., EDAC_MV64X60.
The last to use this driver was by PrPMC 280/2800, for which the support
was ended with the commit 3c8464a9b12b
("powerpc: Delete old PrPMC 280/2800 support").
This looks like the related configs (e.g., MV64X60, EDAC_MV64X60) and the
code (e.g., drivers/edac/mv64x60_edac.c) for Marvell MV64X60 MPSC are now
obsolete.
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH 01/15] selftests/seccomp: Refactor arch register macros to avoid xtensa special case
From: Christian Brauner @ 2020-09-15 15:51 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-2-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:06AM -0700, Kees Cook wrote:
> To avoid an xtensa special-case, refactor all arch register macros to
> take the register variable instead of depending on the macro expanding
> as a struct member name.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Looks good!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH 02/15] selftests/seccomp: Provide generic syscall setting macro
From: Christian Brauner @ 2020-09-15 15:53 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-3-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:07AM -0700, Kees Cook wrote:
> In order to avoid "#ifdef"s in the main function bodies, create a new
> macro, SYSCALL_NUM_SET(), where arch-specific logic can live.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
SYSCALL_SWITCH(_regs, nr)?
But looks good either way!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH 03/15] selftests/seccomp: mips: Define SYSCALL_NUM_SET macro
From: Christian Brauner @ 2020-09-15 15:55 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-4-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:08AM -0700, Kees Cook wrote:
> Remove the mips special-case in change_syscall().
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> tools/testing/selftests/seccomp/seccomp_bpf.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index 1c83e743bfb1..02a9a6599746 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -1742,6 +1742,13 @@ TEST_F(TRACE_poke, getpid_runs_normally)
> # define ARCH_REGS struct pt_regs
> # define SYSCALL_NUM(_regs) (_regs).regs[2]
> # define SYSCALL_SYSCALL_NUM regs[4]
> +# define SYSCALL_NUM_SET(_regs, _nr) \
> + do { \
> + if ((_regs).regs[2] == __NR_O32_Linux) \
> + (_regs).regs[4] = _nr; \
> + else \
> + (_regs).regs[2] = _nr; \
> + } while (0)
I think that
# define SYSCALL_NUM_SET(_regs, _nr) \
do { \
if (SYSCALL_NUM(_regs) == __NR_O32_Linux) \
(_regs).regs[4] = _nr; \
else \
(_regs).regs[2] = _nr; \
} while (0)
would read better but that's just a matter of taste. :)
Looks good!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH 04/15] selftests/seccomp: arm: Define SYSCALL_NUM_SET macro
From: Christian Brauner @ 2020-09-15 15:56 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-5-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:09AM -0700, Kees Cook wrote:
> Remove the arm special-case in change_syscall().
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Looks good!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH 05/15] selftests/seccomp: arm64: Define SYSCALL_NUM_SET macro
From: Christian Brauner @ 2020-09-15 15:58 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-6-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:10AM -0700, Kees Cook wrote:
> Remove the arm64 special-case in change_syscall().
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
We're using iovecs in ptrace()??
Looks good!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH 06/15] selftests/seccomp: mips: Remove O32-specific macro
From: Christian Brauner @ 2020-09-15 16:00 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-7-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:11AM -0700, Kees Cook wrote:
> Instead of having the mips O32 macro special-cased, pull the logic into
> the SYSCALL_NUM() macro. Additionally include the ABI headers, since
> these appear to have been missing, leaving __NR_O32_Linux undefined.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Looks good!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH 07/15] selftests/seccomp: Remove syscall setting #ifdefs
From: Christian Brauner @ 2020-09-15 16:01 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-8-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:12AM -0700, Kees Cook wrote:
> With all architectures now using the common SYSCALL_NUM_SET() macro, the
> arch-specific #ifdef can be removed from change_syscall() itself.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Looks good!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH 08/15] selftests/seccomp: Convert HAVE_GETREG into ARCH_GETREG/ARCH_SETREG
From: Christian Brauner @ 2020-09-15 16:03 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-9-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:13AM -0700, Kees Cook wrote:
> Instead of special-casing the get/set-registers routines, move the
> HAVE_GETREG logic into the new ARCH_GETREG() and ARCH_SETREG() macros.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Looks good!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH v2] powerpc/papr_scm: Fix warning triggered by perf_stats_show()
From: Vaibhav Jain @ 2020-09-15 16:03 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev, linux-nvdimm
Cc: Aneesh Kumar K . V, Santosh Sivaraj, Oliver O'Halloran,
Dan Williams, Ira Weiny
In-Reply-To: <87imcfp9a7.fsf@mpe.ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>> A warning is reported by the kernel in case perf_stats_show() returns
>> an error code. The warning is of the form below:
>>
>> papr_scm ibm,persistent-memory:ibm,pmemory@44100001:
>> Failed to query performance stats, Err:-10
>> dev_attr_show: perf_stats_show+0x0/0x1c0 [papr_scm] returned bad count
>> fill_read_buffer: dev_attr_show+0x0/0xb0 returned bad count
>>
>> On investigation it looks like that the compiler is silently truncating the
>> return value of drc_pmem_query_stats() from 'long' to 'int', since the
>> variable used to store the return code 'rc' is an 'int'. This
>> truncated value is then returned back as a 'ssize_t' back from
>> perf_stats_show() to 'dev_attr_show()' which thinks of it as a large
>> unsigned number and triggers this warning..
>>
>> To fix this we update the type of variable 'rc' from 'int' to
>> 'ssize_t' that prevents the compiler from truncating the return value
>> of drc_pmem_query_stats() and returning correct signed value back from
>> perf_stats_show().
>>
>> Fixes: 2d02bf835e573 ('powerpc/papr_scm: Fetch nvdimm performance
>> stats from PHYP')
>
> Please don't word wrap the Fixes tag it breaks b4.
>
> I've fixed it up this time.
Thanks Mpe
>
> cheers
--
Cheers
~ Vaibhav
^ permalink raw reply
* Re: [PATCH 09/15] selftests/seccomp: Convert REGSET calls into ARCH_GETREG/ARCH_SETREG
From: Christian Brauner @ 2020-09-15 16:05 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-10-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:14AM -0700, Kees Cook wrote:
> Consolidate the REGSET logic into the new ARCH_GETREG() and
> ARCH_SETREG() macros, avoiding more #ifdef code in function bodies.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Looks good!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH 10/15] selftests/seccomp: Avoid redundant register flushes
From: Christian Brauner @ 2020-09-15 16:08 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-11-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:15AM -0700, Kees Cook wrote:
> When none of the registers have changed, don't flush them back. This can
> happen if the architecture uses a non-register way to change the syscall
> (e.g. arm64) , and a return value hasn't been written.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Looks good!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH 11/15] selftests/seccomp: Remove SYSCALL_NUM_RET_SHARE_REG in favor of SYSCALL_RET_SET
From: Christian Brauner @ 2020-09-15 16:11 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-12-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:16AM -0700, Kees Cook wrote:
> Instead of special-casing the specific case of shared registers, create
> a default SYSCALL_RET_SET() macro (mirroring SYSCALL_NUM_SET()), that
> writes to the SYSCALL_RET register. For architectures that can't set the
> return value (for whatever reason), they can define SYSCALL_RET_SET()
> without an associated SYSCALL_RET() macro. This also paves the way for
> architectures that need to do special things to set the return value
> (e.g. powerpc).
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Looks good!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH 15/15] selftests/seccomp: Use __NR_mknodat instead of __NR_mknod
From: Christian Brauner @ 2020-09-15 16:16 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-16-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:20AM -0700, Kees Cook wrote:
> The __NR_mknod syscall doesn't exist on arm64 (only __NR_mknodat).
> Switch to the modern syscall.
>
> Fixes: ad5682184a81 ("selftests/seccomp: Check for EPOLLHUP for user_notif")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Thanks! Looks good.
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH 14/15] selftests/clone3: Avoid OS-defined clone_args
From: Christian Brauner @ 2020-09-15 16:25 UTC (permalink / raw)
To: Kees Cook
Cc: Thadeu Lima de Souza Cascardo, Will Drewry, linux-xtensa,
linux-kernel, Andy Lutomirski, Max Filippov, linux-arm-kernel,
linux-kselftest, linux-mips, linuxppc-dev, Christian Brauner
In-Reply-To: <20200912110820.597135-15-keescook@chromium.org>
On Sat, Sep 12, 2020 at 04:08:19AM -0700, Kees Cook wrote:
> As the UAPI headers start to appear in distros, we need to avoid
> outdated versions of struct clone_args to be able to test modern
> features. Additionally pull in the syscall numbers correctly.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Hm, with this patch applied I'm getting:
gcc -g -I../../../../usr/include/ clone3_set_tid.c /home/brauner/src/git/linux/linux/tools/testing/selftests/kselftest_harness.h /home/brauner/src/git/linux/linux/tools/testing/selftests/kselftest.h -lcap -o /home/brauner/src/git/linux/linux/tools/testing/selftests/clone3/clone3_set_tid
In file included from clone3_set_tid.c:24:
clone3_selftests.h:37:8: error: redefinition of ‘struct clone_args’
37 | struct clone_args {
| ^~~~~~~~~~
In file included from clone3_set_tid.c:12:
/usr/include/linux/sched.h:92:8: note: originally defined here
92 | struct clone_args {
| ^~~~~~~~~~
make: *** [../lib.mk:140: /home/brauner/src/git/linux/linux/tools/testing/selftests/clone3/clone3_set_tid] Error 1
One trick to avoid this could be:
#ifndef CLONE_ARGS_SIZE_VER0
#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
#endif
#ifndef CLONE_ARGS_SIZE_VER1
#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */
#endif
#ifndef CLONE_ARGS_SIZE_VER2
#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
#endif
struct __clone_args {
__aligned_u64 flags;
__aligned_u64 pidfd;
__aligned_u64 child_tid;
__aligned_u64 parent_tid;
__aligned_u64 exit_signal;
__aligned_u64 stack;
__aligned_u64 stack_size;
__aligned_u64 tls;
__aligned_u64 set_tid;
__aligned_u64 set_tid_size;
__aligned_u64 cgroup;
};
static pid_t sys_clone3(struct __clone_args *args, size_t size)
{
return syscall(__NR_clone3, args, size);
}
Christian
^ permalink raw reply
* Re: [5.9.0-rc5-20200914] Kernel crash while running LTP(mlock201)
From: Sachin Sant @ 2020-09-15 16:31 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-next, linuxppc-dev
In-Reply-To: <20200915130907.GE5449@casper.infradead.org>
> On 15-Sep-2020, at 6:39 PM, Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Sep 15, 2020 at 09:24:38PM +1000, Michael Ellerman wrote:
>> Sachin Sant <sachinp@linux.vnet.ibm.com> writes:
>>> While running LTP tests (specifically mlock201) against next-20200914 tree
>>> on a POWER9 LPAR results in following crash.
>>
>> Looks the same as:
>>
>> https://lore.kernel.org/linux-mm/20200914085545.GB28738@shao2-debian/
>
> https://lore.kernel.org/linux-mm/20200914112738.GM6583@casper.infradead.org/
Thanks. The patch fixes the problem for me.
Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
thanks
-Sachin
^ permalink raw reply
* Re: [PATCH v2 2/7] powerpc/prom: Introduce early_reserve_mem_old()
From: Christophe Leroy @ 2020-09-15 16:46 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: Christophe Leroy, linuxppc-dev
In-Reply-To: <20200914211007.2285999-3-clg@kaod.org>
Cédric Le Goater <clg@kaod.org> a écrit :
> and condition its call with IS_ENABLED(CONFIG_PPC32). This fixes a
> compile error with W=1.
>
> arch/powerpc/kernel/prom.c: In function ‘early_reserve_mem’:
> arch/powerpc/kernel/prom.c:625:10: error: variable ‘reserve_map’ set
> but not used [-Werror=unused-but-set-variable]
> __be64 *reserve_map;
> ^~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
@csgroup.eu instead of @c-s.fr please
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> arch/powerpc/kernel/prom.c | 37 ++++++++++++++++++++-----------------
> 1 file changed, 20 insertions(+), 17 deletions(-)
That's a lot of changes for a tiny warning.
You could make it easy by just replacing the #ifdef by:
if (!IS_ENABLED(CONFIG_PPC32))
return;
>
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index d8a2fb87ba0c..c958b67cf1a5 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -620,27 +620,14 @@ static void __init early_reserve_mem_dt(void)
> }
> }
>
> -static void __init early_reserve_mem(void)
> +static void __init early_reserve_mem_old(void)
Why _old ? Do you mean ppc32 are old ? Modern ADSL boxes like for
instance the famous French freebox have powerpc32 microcontroller.
Eventually you could name it _ppc32, but I don't think that's the good
way, see above.
Christophe
> {
> __be64 *reserve_map;
>
> reserve_map = (__be64 *)(((unsigned long)initial_boot_params) +
> fdt_off_mem_rsvmap(initial_boot_params));
>
> - /* Look for the new "reserved-regions" property in the DT */
> - early_reserve_mem_dt();
> -
> -#ifdef CONFIG_BLK_DEV_INITRD
> - /* Then reserve the initrd, if any */
> - if (initrd_start && (initrd_end > initrd_start)) {
> - memblock_reserve(ALIGN_DOWN(__pa(initrd_start), PAGE_SIZE),
> - ALIGN(initrd_end, PAGE_SIZE) -
> - ALIGN_DOWN(initrd_start, PAGE_SIZE));
> - }
> -#endif /* CONFIG_BLK_DEV_INITRD */
> -
> -#ifdef CONFIG_PPC32
> - /*
> + /*
> * Handle the case where we might be booting from an old kexec
> * image that setup the mem_rsvmap as pairs of 32-bit values
> */
> @@ -658,9 +645,25 @@ static void __init early_reserve_mem(void)
> DBG("reserving: %x -> %x\n", base_32, size_32);
> memblock_reserve(base_32, size_32);
> }
> - return;
> }
> -#endif
> +}
> +
> +static void __init early_reserve_mem(void)
> +{
> + /* Look for the new "reserved-regions" property in the DT */
> + early_reserve_mem_dt();
> +
> +#ifdef CONFIG_BLK_DEV_INITRD
> + /* Then reserve the initrd, if any */
> + if (initrd_start && (initrd_end > initrd_start)) {
> + memblock_reserve(ALIGN_DOWN(__pa(initrd_start), PAGE_SIZE),
> + ALIGN(initrd_end, PAGE_SIZE) -
> + ALIGN_DOWN(initrd_start, PAGE_SIZE));
> + }
> +#endif /* CONFIG_BLK_DEV_INITRD */
> +
> + if (IS_ENABLED(CONFIG_PPC32))
> + early_reserve_mem_old();
> }
>
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> --
> 2.25.4
^ permalink raw reply
* Re: [PATCH v2] mm/gup: fix gup_fast with dynamic page table folding
From: Vasily Gorbik @ 2020-09-15 17:09 UTC (permalink / raw)
To: Andrew Morton, Jason Gunthorpe, John Hubbard
Cc: Peter Zijlstra, Dave Hansen, linux-mm, Paul Mackerras,
linux-sparc, Alexander Gordeev, Claudio Imbrenda, Will Deacon,
linux-arch, linux-s390, Richard Weinberger, linux-x86,
Russell King, Christian Borntraeger, Ingo Molnar, Catalin Marinas,
Andrey Ryabinin, Gerald Schaefer, Heiko Carstens, Arnd Bergmann,
Jeff Dike, linux-um, Borislav Petkov, Andy Lutomirski,
Thomas Gleixner, linux-arm, Dave Hansen, linux-power, LKML,
Linus Torvalds, Mike Rapoport
In-Reply-To: <patch.git-943f1e5dcff2.your-ad-here.call-01599856292-ext-8676@work.hours>
On Fri, Sep 11, 2020 at 10:36:43PM +0200, Vasily Gorbik wrote:
> Currently to make sure that every page table entry is read just once
> gup_fast walks perform READ_ONCE and pass pXd value down to the next
> gup_pXd_range function by value e.g.:
...snip...
> ---
> v2: added brackets &pgd -> &(pgd)
>
> arch/s390/include/asm/pgtable.h | 42 +++++++++++++++++++++++----------
> include/linux/pgtable.h | 10 ++++++++
> mm/gup.c | 18 +++++++-------
> 3 files changed, 49 insertions(+), 21 deletions(-)
Andrew, any chance you would pick this up?
There is an Ack from Linus. And I haven't seen any objections from Jason or John.
This seems to be as safe for other architectures as possible.
@Jason and John
Any acks/nacks?
Thank you,
Vasily
^ permalink raw reply
* Re: [PATCH v2] mm/gup: fix gup_fast with dynamic page table folding
From: Jason Gunthorpe @ 2020-09-15 17:14 UTC (permalink / raw)
To: Vasily Gorbik
Cc: Peter Zijlstra, Dave Hansen, linux-mm, Paul Mackerras,
linux-sparc, Alexander Gordeev, Claudio Imbrenda, Will Deacon,
linux-arch, linux-s390, Richard Weinberger, linux-x86,
Russell King, Christian Borntraeger, Ingo Molnar, Catalin Marinas,
Andrey Ryabinin, Gerald Schaefer, Heiko Carstens, Arnd Bergmann,
John Hubbard, Jeff Dike, linux-um, Borislav Petkov,
Andy Lutomirski, Thomas Gleixner, linux-arm, Dave Hansen,
linux-power, LKML, Andrew Morton, Linus Torvalds, Mike Rapoport
In-Reply-To: <patch.git-943f1e5dcff2.your-ad-here.call-01599856292-ext-8676@work.hours>
On Fri, Sep 11, 2020 at 10:36:43PM +0200, Vasily Gorbik wrote:
> Currently to make sure that every page table entry is read just once
> gup_fast walks perform READ_ONCE and pass pXd value down to the next
> gup_pXd_range function by value e.g.:
>
> static int gup_pud_range(p4d_t p4d, unsigned long addr, unsigned long end,
> unsigned int flags, struct page **pages, int *nr)
> ...
> pudp = pud_offset(&p4d, addr);
>
> This function passes a reference on that local value copy to pXd_offset,
> and might get the very same pointer in return. This happens when the
> level is folded (on most arches), and that pointer should not be iterated.
>
> On s390 due to the fact that each task might have different 5,4 or
> 3-level address translation and hence different levels folded the logic
> is more complex and non-iteratable pointer to a local copy leads to
> severe problems.
>
> Here is an example of what happens with gup_fast on s390, for a task
> with 3-levels paging, crossing a 2 GB pud boundary:
>
> // addr = 0x1007ffff000, end = 0x10080001000
> static int gup_pud_range(p4d_t p4d, unsigned long addr, unsigned long end,
> unsigned int flags, struct page **pages, int *nr)
> {
> unsigned long next;
> pud_t *pudp;
>
> // pud_offset returns &p4d itself (a pointer to a value on stack)
> pudp = pud_offset(&p4d, addr);
> do {
> // on second iteratation reading "random" stack value
> pud_t pud = READ_ONCE(*pudp);
>
> // next = 0x10080000000, due to PUD_SIZE/MASK != PGDIR_SIZE/MASK on s390
> next = pud_addr_end(addr, end);
> ...
> } while (pudp++, addr = next, addr != end); // pudp++ iterating over stack
>
> return 1;
> }
>
> This happens since s390 moved to common gup code with
> commit d1874a0c2805 ("s390/mm: make the pxd_offset functions more robust")
> and commit 1a42010cdc26 ("s390/mm: convert to the generic
> get_user_pages_fast code"). s390 tried to mimic static level folding by
> changing pXd_offset primitives to always calculate top level page table
> offset in pgd_offset and just return the value passed when pXd_offset
> has to act as folded.
>
> What is crucial for gup_fast and what has been overlooked is
> that PxD_SIZE/MASK and thus pXd_addr_end should also change
> correspondingly. And the latter is not possible with dynamic folding.
>
> To fix the issue in addition to pXd values pass original
> pXdp pointers down to gup_pXd_range functions. And introduce
> pXd_offset_lockless helpers, which take an additional pXd
> entry value parameter. This has already been discussed in
> https://lkml.kernel.org/r/20190418100218.0a4afd51@mschwideX1
>
> Cc: <stable@vger.kernel.org> # 5.2+
> Fixes: 1a42010cdc26 ("s390/mm: convert to the generic get_user_pages_fast code")
> Reviewed-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> Reviewed-by: Alexander Gordeev <agordeev@linux.ibm.com>
> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
> ---
> v2: added brackets &pgd -> &(pgd)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Regards,
Jason
^ permalink raw reply
* Re: [PATCH v2] mm/gup: fix gup_fast with dynamic page table folding
From: Mike Rapoport @ 2020-09-15 17:18 UTC (permalink / raw)
To: Vasily Gorbik
Cc: Peter Zijlstra, Dave Hansen, Dave Hansen, Paul Mackerras,
linux-sparc, Alexander Gordeev, Claudio Imbrenda, Will Deacon,
linux-arch, linux-s390, Christian Borntraeger, Richard Weinberger,
linux-x86, Russell King, Jason Gunthorpe, Ingo Molnar,
Catalin Marinas, Andrey Ryabinin, Gerald Schaefer, Heiko Carstens,
Arnd Bergmann, John Hubbard, Jeff Dike, linux-um, Borislav Petkov,
Andy Lutomirski, Thomas Gleixner, linux-arm, linux-mm,
linux-power, LKML, Andrew Morton, Linus Torvalds
In-Reply-To: <patch.git-943f1e5dcff2.your-ad-here.call-01599856292-ext-8676@work.hours>
On Fri, Sep 11, 2020 at 10:36:43PM +0200, Vasily Gorbik wrote:
> Currently to make sure that every page table entry is read just once
> gup_fast walks perform READ_ONCE and pass pXd value down to the next
> gup_pXd_range function by value e.g.:
>
> static int gup_pud_range(p4d_t p4d, unsigned long addr, unsigned long end,
> unsigned int flags, struct page **pages, int *nr)
> ...
> pudp = pud_offset(&p4d, addr);
>
> This function passes a reference on that local value copy to pXd_offset,
> and might get the very same pointer in return. This happens when the
> level is folded (on most arches), and that pointer should not be iterated.
>
> On s390 due to the fact that each task might have different 5,4 or
> 3-level address translation and hence different levels folded the logic
> is more complex and non-iteratable pointer to a local copy leads to
> severe problems.
>
> Here is an example of what happens with gup_fast on s390, for a task
> with 3-levels paging, crossing a 2 GB pud boundary:
>
> // addr = 0x1007ffff000, end = 0x10080001000
> static int gup_pud_range(p4d_t p4d, unsigned long addr, unsigned long end,
> unsigned int flags, struct page **pages, int *nr)
> {
> unsigned long next;
> pud_t *pudp;
>
> // pud_offset returns &p4d itself (a pointer to a value on stack)
> pudp = pud_offset(&p4d, addr);
> do {
> // on second iteratation reading "random" stack value
> pud_t pud = READ_ONCE(*pudp);
>
> // next = 0x10080000000, due to PUD_SIZE/MASK != PGDIR_SIZE/MASK on s390
> next = pud_addr_end(addr, end);
> ...
> } while (pudp++, addr = next, addr != end); // pudp++ iterating over stack
>
> return 1;
> }
>
> This happens since s390 moved to common gup code with
> commit d1874a0c2805 ("s390/mm: make the pxd_offset functions more robust")
> and commit 1a42010cdc26 ("s390/mm: convert to the generic
> get_user_pages_fast code"). s390 tried to mimic static level folding by
> changing pXd_offset primitives to always calculate top level page table
> offset in pgd_offset and just return the value passed when pXd_offset
> has to act as folded.
>
> What is crucial for gup_fast and what has been overlooked is
> that PxD_SIZE/MASK and thus pXd_addr_end should also change
> correspondingly. And the latter is not possible with dynamic folding.
>
> To fix the issue in addition to pXd values pass original
> pXdp pointers down to gup_pXd_range functions. And introduce
> pXd_offset_lockless helpers, which take an additional pXd
> entry value parameter. This has already been discussed in
> https://lkml.kernel.org/r/20190418100218.0a4afd51@mschwideX1
>
> Cc: <stable@vger.kernel.org> # 5.2+
> Fixes: 1a42010cdc26 ("s390/mm: convert to the generic get_user_pages_fast code")
> Reviewed-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> Reviewed-by: Alexander Gordeev <agordeev@linux.ibm.com>
> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>
> ---
> v2: added brackets &pgd -> &(pgd)
>
> arch/s390/include/asm/pgtable.h | 42 +++++++++++++++++++++++----------
> include/linux/pgtable.h | 10 ++++++++
> mm/gup.c | 18 +++++++-------
> 3 files changed, 49 insertions(+), 21 deletions(-)
>
> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
> index 7eb01a5459cd..b55561cc8786 100644
> --- a/arch/s390/include/asm/pgtable.h
> +++ b/arch/s390/include/asm/pgtable.h
> @@ -1260,26 +1260,44 @@ static inline pgd_t *pgd_offset_raw(pgd_t *pgd, unsigned long address)
>
> #define pgd_offset(mm, address) pgd_offset_raw(READ_ONCE((mm)->pgd), address)
>
> -static inline p4d_t *p4d_offset(pgd_t *pgd, unsigned long address)
> +static inline p4d_t *p4d_offset_lockless(pgd_t *pgdp, pgd_t pgd, unsigned long address)
> {
> - if ((pgd_val(*pgd) & _REGION_ENTRY_TYPE_MASK) >= _REGION_ENTRY_TYPE_R1)
> - return (p4d_t *) pgd_deref(*pgd) + p4d_index(address);
> - return (p4d_t *) pgd;
> + if ((pgd_val(pgd) & _REGION_ENTRY_TYPE_MASK) >= _REGION_ENTRY_TYPE_R1)
> + return (p4d_t *) pgd_deref(pgd) + p4d_index(address);
> + return (p4d_t *) pgdp;
> }
> +#define p4d_offset_lockless p4d_offset_lockless
>
> -static inline pud_t *pud_offset(p4d_t *p4d, unsigned long address)
> +static inline p4d_t *p4d_offset(pgd_t *pgdp, unsigned long address)
> {
> - if ((p4d_val(*p4d) & _REGION_ENTRY_TYPE_MASK) >= _REGION_ENTRY_TYPE_R2)
> - return (pud_t *) p4d_deref(*p4d) + pud_index(address);
> - return (pud_t *) p4d;
> + return p4d_offset_lockless(pgdp, *pgdp, address);
> +}
> +
> +static inline pud_t *pud_offset_lockless(p4d_t *p4dp, p4d_t p4d, unsigned long address)
> +{
> + if ((p4d_val(p4d) & _REGION_ENTRY_TYPE_MASK) >= _REGION_ENTRY_TYPE_R2)
> + return (pud_t *) p4d_deref(p4d) + pud_index(address);
> + return (pud_t *) p4dp;
> +}
> +#define pud_offset_lockless pud_offset_lockless
> +
> +static inline pud_t *pud_offset(p4d_t *p4dp, unsigned long address)
> +{
> + return pud_offset_lockless(p4dp, *p4dp, address);
> }
> #define pud_offset pud_offset
>
> -static inline pmd_t *pmd_offset(pud_t *pud, unsigned long address)
> +static inline pmd_t *pmd_offset_lockless(pud_t *pudp, pud_t pud, unsigned long address)
> +{
> + if ((pud_val(pud) & _REGION_ENTRY_TYPE_MASK) >= _REGION_ENTRY_TYPE_R3)
> + return (pmd_t *) pud_deref(pud) + pmd_index(address);
> + return (pmd_t *) pudp;
> +}
> +#define pmd_offset_lockless pmd_offset_lockless
> +
> +static inline pmd_t *pmd_offset(pud_t *pudp, unsigned long address)
> {
> - if ((pud_val(*pud) & _REGION_ENTRY_TYPE_MASK) >= _REGION_ENTRY_TYPE_R3)
> - return (pmd_t *) pud_deref(*pud) + pmd_index(address);
> - return (pmd_t *) pud;
> + return pmd_offset_lockless(pudp, *pudp, address);
> }
> #define pmd_offset pmd_offset
>
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index e8cbc2e795d5..90654cb63e9e 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -1427,6 +1427,16 @@ typedef unsigned int pgtbl_mod_mask;
> #define mm_pmd_folded(mm) __is_defined(__PAGETABLE_PMD_FOLDED)
> #endif
>
> +#ifndef p4d_offset_lockless
> +#define p4d_offset_lockless(pgdp, pgd, address) p4d_offset(&(pgd), address)
> +#endif
> +#ifndef pud_offset_lockless
> +#define pud_offset_lockless(p4dp, p4d, address) pud_offset(&(p4d), address)
> +#endif
> +#ifndef pmd_offset_lockless
> +#define pmd_offset_lockless(pudp, pud, address) pmd_offset(&(pud), address)
> +#endif
> +
> /*
> * p?d_leaf() - true if this entry is a final mapping to a physical address.
> * This differs from p?d_huge() by the fact that they are always available (if
> diff --git a/mm/gup.c b/mm/gup.c
> index e5739a1974d5..578bf5bd8bf8 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2485,13 +2485,13 @@ static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr,
> return 1;
> }
>
> -static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
> +static int gup_pmd_range(pud_t *pudp, pud_t pud, unsigned long addr, unsigned long end,
> unsigned int flags, struct page **pages, int *nr)
> {
> unsigned long next;
> pmd_t *pmdp;
>
> - pmdp = pmd_offset(&pud, addr);
> + pmdp = pmd_offset_lockless(pudp, pud, addr);
> do {
> pmd_t pmd = READ_ONCE(*pmdp);
>
> @@ -2528,13 +2528,13 @@ static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
> return 1;
> }
>
> -static int gup_pud_range(p4d_t p4d, unsigned long addr, unsigned long end,
> +static int gup_pud_range(p4d_t *p4dp, p4d_t p4d, unsigned long addr, unsigned long end,
> unsigned int flags, struct page **pages, int *nr)
> {
> unsigned long next;
> pud_t *pudp;
>
> - pudp = pud_offset(&p4d, addr);
> + pudp = pud_offset_lockless(p4dp, p4d, addr);
> do {
> pud_t pud = READ_ONCE(*pudp);
>
> @@ -2549,20 +2549,20 @@ static int gup_pud_range(p4d_t p4d, unsigned long addr, unsigned long end,
> if (!gup_huge_pd(__hugepd(pud_val(pud)), addr,
> PUD_SHIFT, next, flags, pages, nr))
> return 0;
> - } else if (!gup_pmd_range(pud, addr, next, flags, pages, nr))
> + } else if (!gup_pmd_range(pudp, pud, addr, next, flags, pages, nr))
> return 0;
> } while (pudp++, addr = next, addr != end);
>
> return 1;
> }
>
> -static int gup_p4d_range(pgd_t pgd, unsigned long addr, unsigned long end,
> +static int gup_p4d_range(pgd_t *pgdp, pgd_t pgd, unsigned long addr, unsigned long end,
> unsigned int flags, struct page **pages, int *nr)
> {
> unsigned long next;
> p4d_t *p4dp;
>
> - p4dp = p4d_offset(&pgd, addr);
> + p4dp = p4d_offset_lockless(pgdp, pgd, addr);
> do {
> p4d_t p4d = READ_ONCE(*p4dp);
>
> @@ -2574,7 +2574,7 @@ static int gup_p4d_range(pgd_t pgd, unsigned long addr, unsigned long end,
> if (!gup_huge_pd(__hugepd(p4d_val(p4d)), addr,
> P4D_SHIFT, next, flags, pages, nr))
> return 0;
> - } else if (!gup_pud_range(p4d, addr, next, flags, pages, nr))
> + } else if (!gup_pud_range(p4dp, p4d, addr, next, flags, pages, nr))
> return 0;
> } while (p4dp++, addr = next, addr != end);
>
> @@ -2602,7 +2602,7 @@ static void gup_pgd_range(unsigned long addr, unsigned long end,
> if (!gup_huge_pd(__hugepd(pgd_val(pgd)), addr,
> PGDIR_SHIFT, next, flags, pages, nr))
> return;
> - } else if (!gup_p4d_range(pgd, addr, next, flags, pages, nr))
> + } else if (!gup_p4d_range(pgdp, pgd, addr, next, flags, pages, nr))
> return;
> } while (pgdp++, addr = next, addr != end);
> }
> --
> ⣿⣿⣿⣿⢋⡀⣀⠹⣿⣿⣿⣿
> ⣿⣿⣿⣿⠠⣶⡦⠀⣿⣿⣿⣿
> ⣿⣿⣿⠏⣴⣮⣴⣧⠈⢿⣿⣿
> ⣿⣿⡏⢰⣿⠖⣠⣿⡆⠈⣿⣿
> ⣿⢛⣵⣄⠙⣶⣶⡟⣅⣠⠹⣿
> ⣿⣜⣛⠻⢎⣉⣉⣀⠿⣫⣵⣿
--
Sincerely yours,
Mike.
^ permalink raw reply
* Re: [PATCH v2] mm/gup: fix gup_fast with dynamic page table folding
From: John Hubbard @ 2020-09-15 17:31 UTC (permalink / raw)
To: Vasily Gorbik, Jason Gunthorpe
Cc: Peter Zijlstra, Dave Hansen, linux-mm, Paul Mackerras,
linux-sparc, Alexander Gordeev, Claudio Imbrenda, Will Deacon,
linux-arch, linux-s390, Richard Weinberger, linux-x86,
Russell King, Christian Borntraeger, Ingo Molnar, Catalin Marinas,
Andrey Ryabinin, Gerald Schaefer, Heiko Carstens, Arnd Bergmann,
Jeff Dike, linux-um, Borislav Petkov, Andy Lutomirski,
Thomas Gleixner, linux-arm, Dave Hansen, linux-power, LKML,
Andrew Morton, Linus Torvalds, Mike Rapoport
In-Reply-To: <patch.git-943f1e5dcff2.your-ad-here.call-01599856292-ext-8676@work.hours>
On 9/11/20 1:36 PM, Vasily Gorbik wrote:
> Currently to make sure that every page table entry is read just once
> gup_fast walks perform READ_ONCE and pass pXd value down to the next
> gup_pXd_range function by value e.g.:
>
> static int gup_pud_range(p4d_t p4d, unsigned long addr, unsigned long end,
> unsigned int flags, struct page **pages, int *nr)
> ...
> pudp = pud_offset(&p4d, addr);
>
> This function passes a reference on that local value copy to pXd_offset,
> and might get the very same pointer in return. This happens when the
> level is folded (on most arches), and that pointer should not be iterated.
>
> On s390 due to the fact that each task might have different 5,4 or
> 3-level address translation and hence different levels folded the logic
> is more complex and non-iteratable pointer to a local copy leads to
> severe problems.
>
> Here is an example of what happens with gup_fast on s390, for a task
> with 3-levels paging, crossing a 2 GB pud boundary:
>
> // addr = 0x1007ffff000, end = 0x10080001000
> static int gup_pud_range(p4d_t p4d, unsigned long addr, unsigned long end,
> unsigned int flags, struct page **pages, int *nr)
> {
> unsigned long next;
> pud_t *pudp;
>
> // pud_offset returns &p4d itself (a pointer to a value on stack)
> pudp = pud_offset(&p4d, addr);
> do {
> // on second iteratation reading "random" stack value
> pud_t pud = READ_ONCE(*pudp);
>
> // next = 0x10080000000, due to PUD_SIZE/MASK != PGDIR_SIZE/MASK on s390
> next = pud_addr_end(addr, end);
> ...
> } while (pudp++, addr = next, addr != end); // pudp++ iterating over stack
>
> return 1;
> }
>
> This happens since s390 moved to common gup code with
> commit d1874a0c2805 ("s390/mm: make the pxd_offset functions more robust")
> and commit 1a42010cdc26 ("s390/mm: convert to the generic
> get_user_pages_fast code"). s390 tried to mimic static level folding by
> changing pXd_offset primitives to always calculate top level page table
> offset in pgd_offset and just return the value passed when pXd_offset
> has to act as folded.
>
> What is crucial for gup_fast and what has been overlooked is
> that PxD_SIZE/MASK and thus pXd_addr_end should also change
> correspondingly. And the latter is not possible with dynamic folding.
>
> To fix the issue in addition to pXd values pass original
> pXdp pointers down to gup_pXd_range functions. And introduce
> pXd_offset_lockless helpers, which take an additional pXd
> entry value parameter. This has already been discussed in
> https://lkml.kernel.org/r/20190418100218.0a4afd51@mschwideX1
>
> Cc: <stable@vger.kernel.org> # 5.2+
> Fixes: 1a42010cdc26 ("s390/mm: convert to the generic get_user_pages_fast code")
> Reviewed-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> Reviewed-by: Alexander Gordeev <agordeev@linux.ibm.com>
> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
> ---
Looks cleaner than I'd dared hope for. :)
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
> v2: added brackets &pgd -> &(pgd)
>
> arch/s390/include/asm/pgtable.h | 42 +++++++++++++++++++++++----------
> include/linux/pgtable.h | 10 ++++++++
> mm/gup.c | 18 +++++++-------
> 3 files changed, 49 insertions(+), 21 deletions(-)
>
> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
> index 7eb01a5459cd..b55561cc8786 100644
> --- a/arch/s390/include/asm/pgtable.h
> +++ b/arch/s390/include/asm/pgtable.h
> @@ -1260,26 +1260,44 @@ static inline pgd_t *pgd_offset_raw(pgd_t *pgd, unsigned long address)
>
> #define pgd_offset(mm, address) pgd_offset_raw(READ_ONCE((mm)->pgd), address)
>
> -static inline p4d_t *p4d_offset(pgd_t *pgd, unsigned long address)
> +static inline p4d_t *p4d_offset_lockless(pgd_t *pgdp, pgd_t pgd, unsigned long address)
> {
> - if ((pgd_val(*pgd) & _REGION_ENTRY_TYPE_MASK) >= _REGION_ENTRY_TYPE_R1)
> - return (p4d_t *) pgd_deref(*pgd) + p4d_index(address);
> - return (p4d_t *) pgd;
> + if ((pgd_val(pgd) & _REGION_ENTRY_TYPE_MASK) >= _REGION_ENTRY_TYPE_R1)
> + return (p4d_t *) pgd_deref(pgd) + p4d_index(address);
> + return (p4d_t *) pgdp;
> }
> +#define p4d_offset_lockless p4d_offset_lockless
>
> -static inline pud_t *pud_offset(p4d_t *p4d, unsigned long address)
> +static inline p4d_t *p4d_offset(pgd_t *pgdp, unsigned long address)
> {
> - if ((p4d_val(*p4d) & _REGION_ENTRY_TYPE_MASK) >= _REGION_ENTRY_TYPE_R2)
> - return (pud_t *) p4d_deref(*p4d) + pud_index(address);
> - return (pud_t *) p4d;
> + return p4d_offset_lockless(pgdp, *pgdp, address);
> +}
> +
> +static inline pud_t *pud_offset_lockless(p4d_t *p4dp, p4d_t p4d, unsigned long address)
> +{
> + if ((p4d_val(p4d) & _REGION_ENTRY_TYPE_MASK) >= _REGION_ENTRY_TYPE_R2)
> + return (pud_t *) p4d_deref(p4d) + pud_index(address);
> + return (pud_t *) p4dp;
> +}
> +#define pud_offset_lockless pud_offset_lockless
> +
> +static inline pud_t *pud_offset(p4d_t *p4dp, unsigned long address)
> +{
> + return pud_offset_lockless(p4dp, *p4dp, address);
> }
> #define pud_offset pud_offset
>
> -static inline pmd_t *pmd_offset(pud_t *pud, unsigned long address)
> +static inline pmd_t *pmd_offset_lockless(pud_t *pudp, pud_t pud, unsigned long address)
> +{
> + if ((pud_val(pud) & _REGION_ENTRY_TYPE_MASK) >= _REGION_ENTRY_TYPE_R3)
> + return (pmd_t *) pud_deref(pud) + pmd_index(address);
> + return (pmd_t *) pudp;
> +}
> +#define pmd_offset_lockless pmd_offset_lockless
> +
> +static inline pmd_t *pmd_offset(pud_t *pudp, unsigned long address)
> {
> - if ((pud_val(*pud) & _REGION_ENTRY_TYPE_MASK) >= _REGION_ENTRY_TYPE_R3)
> - return (pmd_t *) pud_deref(*pud) + pmd_index(address);
> - return (pmd_t *) pud;
> + return pmd_offset_lockless(pudp, *pudp, address);
> }
> #define pmd_offset pmd_offset
>
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index e8cbc2e795d5..90654cb63e9e 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -1427,6 +1427,16 @@ typedef unsigned int pgtbl_mod_mask;
> #define mm_pmd_folded(mm) __is_defined(__PAGETABLE_PMD_FOLDED)
> #endif
>
> +#ifndef p4d_offset_lockless
> +#define p4d_offset_lockless(pgdp, pgd, address) p4d_offset(&(pgd), address)
> +#endif
> +#ifndef pud_offset_lockless
> +#define pud_offset_lockless(p4dp, p4d, address) pud_offset(&(p4d), address)
> +#endif
> +#ifndef pmd_offset_lockless
> +#define pmd_offset_lockless(pudp, pud, address) pmd_offset(&(pud), address)
> +#endif
> +
> /*
> * p?d_leaf() - true if this entry is a final mapping to a physical address.
> * This differs from p?d_huge() by the fact that they are always available (if
> diff --git a/mm/gup.c b/mm/gup.c
> index e5739a1974d5..578bf5bd8bf8 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2485,13 +2485,13 @@ static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr,
> return 1;
> }
>
> -static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
> +static int gup_pmd_range(pud_t *pudp, pud_t pud, unsigned long addr, unsigned long end,
> unsigned int flags, struct page **pages, int *nr)
> {
> unsigned long next;
> pmd_t *pmdp;
>
> - pmdp = pmd_offset(&pud, addr);
> + pmdp = pmd_offset_lockless(pudp, pud, addr);
> do {
> pmd_t pmd = READ_ONCE(*pmdp);
>
> @@ -2528,13 +2528,13 @@ static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
> return 1;
> }
>
> -static int gup_pud_range(p4d_t p4d, unsigned long addr, unsigned long end,
> +static int gup_pud_range(p4d_t *p4dp, p4d_t p4d, unsigned long addr, unsigned long end,
> unsigned int flags, struct page **pages, int *nr)
> {
> unsigned long next;
> pud_t *pudp;
>
> - pudp = pud_offset(&p4d, addr);
> + pudp = pud_offset_lockless(p4dp, p4d, addr);
> do {
> pud_t pud = READ_ONCE(*pudp);
>
> @@ -2549,20 +2549,20 @@ static int gup_pud_range(p4d_t p4d, unsigned long addr, unsigned long end,
> if (!gup_huge_pd(__hugepd(pud_val(pud)), addr,
> PUD_SHIFT, next, flags, pages, nr))
> return 0;
> - } else if (!gup_pmd_range(pud, addr, next, flags, pages, nr))
> + } else if (!gup_pmd_range(pudp, pud, addr, next, flags, pages, nr))
> return 0;
> } while (pudp++, addr = next, addr != end);
>
> return 1;
> }
>
> -static int gup_p4d_range(pgd_t pgd, unsigned long addr, unsigned long end,
> +static int gup_p4d_range(pgd_t *pgdp, pgd_t pgd, unsigned long addr, unsigned long end,
> unsigned int flags, struct page **pages, int *nr)
> {
> unsigned long next;
> p4d_t *p4dp;
>
> - p4dp = p4d_offset(&pgd, addr);
> + p4dp = p4d_offset_lockless(pgdp, pgd, addr);
> do {
> p4d_t p4d = READ_ONCE(*p4dp);
>
> @@ -2574,7 +2574,7 @@ static int gup_p4d_range(pgd_t pgd, unsigned long addr, unsigned long end,
> if (!gup_huge_pd(__hugepd(p4d_val(p4d)), addr,
> P4D_SHIFT, next, flags, pages, nr))
> return 0;
> - } else if (!gup_pud_range(p4d, addr, next, flags, pages, nr))
> + } else if (!gup_pud_range(p4dp, p4d, addr, next, flags, pages, nr))
> return 0;
> } while (p4dp++, addr = next, addr != end);
>
> @@ -2602,7 +2602,7 @@ static void gup_pgd_range(unsigned long addr, unsigned long end,
> if (!gup_huge_pd(__hugepd(pgd_val(pgd)), addr,
> PGDIR_SHIFT, next, flags, pages, nr))
> return;
> - } else if (!gup_p4d_range(pgd, addr, next, flags, pages, nr))
> + } else if (!gup_p4d_range(pgdp, pgd, addr, next, flags, pages, nr))
> return;
> } while (pgdp++, addr = next, addr != end);
> }
>
^ 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