* Re: [PATCH NOTFORMERGE 0/5] Extend remote madvise API to KSM hints
From: Minchan Kim @ 2019-12-11 1:35 UTC (permalink / raw)
To: Oleksandr Natalenko; +Cc: linux-kernel, linux-mm, linux-api
In-Reply-To: <20191210104939.jauw5hnv3smhtvtr@butterfly.localdomain>
Hi Oleksandr,
On Tue, Dec 10, 2019 at 11:49:39AM +0100, Oleksandr Natalenko wrote:
> Hello, Minchan.
>
> On Sun, Jun 16, 2019 at 10:58:30AM +0200, Oleksandr Natalenko wrote:
> > This is a set of commits based on our discussion on your submission [1].
> >
> > First 2 implement minor suggestions just for you to not forget to take
> > them into account.
> >
> > uio.h inclusion was needed for me to be able to compile your series
> > successfully. Also please note I had to enable "Transparent Hugepage
> > Support" as well as "Enable idle page tracking" options, otherwise the
> > build failed. I guess this can be addressed by you better since the
> > errors are introduced with MADV_COLD introduction.
> >
> > Last 2 commits are the actual KSM hints enablement. The first one
> > implements additional check for the case where the mmap_sem is taken for
> > write, and the second one just allows KSM hints to be used by the remote
> > interface.
> >
> > I'm not Cc'ing else anyone except two mailing lists to not distract
> > people unnecessarily. If you are fine with this addition, please use it
> > for your next iteration of process_madvise(), and then you'll Cc all the
> > people needed.
> >
> > Thanks.
> >
> > [1] https://lore.kernel.org/lkml/20190531064313.193437-1-minchan@kernel.org/
> >
> > Oleksandr Natalenko (5):
> > mm: rename madvise_core to madvise_common
> > mm: revert madvise_inject_error line split
> > mm: include uio.h to madvise.c
> > mm/madvise: employ mmget_still_valid for write lock
> > mm/madvise: allow KSM hints for remote API
> >
> > mm/madvise.c | 23 ++++++++++++++---------
> > 1 file changed, 14 insertions(+), 9 deletions(-)
> >
> > --
> > 2.22.0
> >
>
> This is a gentle ping. Are you still planning to submit process_madvise() solution?
I'm really sorry for being slow progress.
I am stuck with internal stuff of company.
I will do best effort to send it within one or two weeks.
Thanks.
^ permalink raw reply
* [PATCH AUTOSEL 5.4 026/134] selftests/powerpc: Fixup clobbers for TM tests
From: Sasha Levin @ 2019-12-11 15:10 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Michael Ellerman, Sasha Levin, linux-api
In-Reply-To: <20191211151150.19073-1-sashal@kernel.org>
From: Michael Ellerman <mpe@ellerman.id.au>
[ Upstream commit a02cbc7ffe529ed58b6bbe54652104fc2c88bd77 ]
Some of our TM (Transactional Memory) tests, list "r1" (the stack
pointer) as a clobbered register.
GCC >= 9 doesn't accept this, and the build breaks:
ptrace-tm-spd-tar.c: In function 'tm_spd_tar':
ptrace-tm-spd-tar.c:31:2: error: listing the stack pointer register 'r1' in a clobber list is deprecated [-Werror=deprecated]
31 | asm __volatile__(
| ^~~
ptrace-tm-spd-tar.c:31:2: note: the value of the stack pointer after an 'asm' statement must be the same as it was before the statement
We do have some fairly large inline asm blocks in these tests, and
some of them do change the value of r1. However they should all return
to C with the value in r1 restored, so I think it's legitimate to say
r1 is not clobbered.
As Segher points out, the r1 clobbers may have been added because of
the use of `or 1,1,1`, however that doesn't actually clobber r1.
Segher also points out that some of these tests do clobber LR, because
they call functions, and that is not listed in the clobbers, so add
that where appropriate.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Link: https://lore.kernel.org/r/20191029095324.14669-1-mpe@ellerman.id.au
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c | 2 +-
tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c | 4 ++--
tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c | 2 +-
tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c | 4 ++--
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c
index 25e23e73c72e6..2ecfa1158e2bc 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c
@@ -73,7 +73,7 @@ trans:
[sprn_texasr]"i"(SPRN_TEXASR), [tar_1]"i"(TAR_1),
[dscr_1]"i"(DSCR_1), [tar_2]"i"(TAR_2), [dscr_2]"i"(DSCR_2),
[tar_3]"i"(TAR_3), [dscr_3]"i"(DSCR_3)
- : "memory", "r0", "r1", "r3", "r4", "r5", "r6"
+ : "memory", "r0", "r3", "r4", "r5", "r6", "lr"
);
/* TM failed, analyse */
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c
index f603fe5a445b3..6f7fb51f08099 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c
@@ -74,8 +74,8 @@ trans:
"3: ;"
: [res] "=r" (result), [texasr] "=r" (texasr)
: [sprn_texasr] "i" (SPRN_TEXASR)
- : "memory", "r0", "r1", "r3", "r4",
- "r7", "r8", "r9", "r10", "r11"
+ : "memory", "r0", "r3", "r4",
+ "r7", "r8", "r9", "r10", "r11", "lr"
);
if (result) {
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c
index e0d37f07bdeba..46ef378a15ecc 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c
@@ -62,7 +62,7 @@ trans:
[sprn_ppr]"i"(SPRN_PPR), [sprn_texasr]"i"(SPRN_TEXASR),
[tar_1]"i"(TAR_1), [dscr_1]"i"(DSCR_1), [tar_2]"i"(TAR_2),
[dscr_2]"i"(DSCR_2), [cptr1] "b" (&cptr[1])
- : "memory", "r0", "r1", "r3", "r4", "r5", "r6"
+ : "memory", "r0", "r3", "r4", "r5", "r6"
);
/* TM failed, analyse */
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c
index 8027457b97b7c..70ca01234f79c 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c
@@ -62,8 +62,8 @@ trans:
"3: ;"
: [res] "=r" (result), [texasr] "=r" (texasr)
: [sprn_texasr] "i" (SPRN_TEXASR), [cptr1] "b" (&cptr[1])
- : "memory", "r0", "r1", "r3", "r4",
- "r7", "r8", "r9", "r10", "r11"
+ : "memory", "r0", "r3", "r4",
+ "r7", "r8", "r9", "r10", "r11", "lr"
);
if (result) {
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.4 038/134] selftests/powerpc: Skip tm-signal-sigreturn-nt if TM not available
From: Sasha Levin @ 2019-12-11 15:10 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Michael Ellerman, Sasha Levin, linux-api
In-Reply-To: <20191211151150.19073-1-sashal@kernel.org>
From: Michael Ellerman <mpe@ellerman.id.au>
[ Upstream commit 505127068d9b705a6cf335143239db91bfe7bbe2 ]
On systems where TM (Transactional Memory) is disabled the
tm-signal-sigreturn-nt test causes a SIGILL:
test: tm_signal_sigreturn_nt
tags: git_version:7c202575ef63
!! child died by signal 4
failure: tm_signal_sigreturn_nt
We should skip the test if TM is not available.
Fixes: 34642d70ac7e ("selftests/powerpc: Add checks for transactional sigreturn")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Link: https://lore.kernel.org/r/20191104233524.24348-1-mpe@ellerman.id.au
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
tools/testing/selftests/powerpc/tm/tm-signal-sigreturn-nt.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tools/testing/selftests/powerpc/tm/tm-signal-sigreturn-nt.c b/tools/testing/selftests/powerpc/tm/tm-signal-sigreturn-nt.c
index 56fbf9f6bbf30..07c388147b75e 100644
--- a/tools/testing/selftests/powerpc/tm/tm-signal-sigreturn-nt.c
+++ b/tools/testing/selftests/powerpc/tm/tm-signal-sigreturn-nt.c
@@ -10,10 +10,12 @@
*/
#define _GNU_SOURCE
+#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
#include "utils.h"
+#include "tm.h"
void trap_signal_handler(int signo, siginfo_t *si, void *uc)
{
@@ -29,6 +31,8 @@ int tm_signal_sigreturn_nt(void)
{
struct sigaction trap_sa;
+ SKIP_IF(!have_htm());
+
trap_sa.sa_flags = SA_SIGINFO;
trap_sa.sa_sigaction = trap_signal_handler;
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.4 131/134] selftests: vm: add fragment CONFIG_TEST_VMALLOC
From: Sasha Levin @ 2019-12-11 15:11 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Anders Roxell, Shuah Khan, Uladzislau Rezki (Sony), Andrew Morton,
Linus Torvalds, Sasha Levin, linux-api
In-Reply-To: <20191211151150.19073-1-sashal@kernel.org>
From: Anders Roxell <anders.roxell@linaro.org>
[ Upstream commit 746dd4012d215b53152f0001a48856e41ea31730 ]
When running test_vmalloc.sh smoke the following print out states that
the fragment is missing.
# ./test_vmalloc.sh: You must have the following enabled in your kernel:
# CONFIG_TEST_VMALLOC=m
Rework to add the fragment 'CONFIG_TEST_VMALLOC=m' to the config file.
Link: http://lkml.kernel.org/r/20190916095217.19665-1-anders.roxell@linaro.org
Fixes: a05ef00c9790 ("selftests/vm: add script helper for CONFIG_TEST_VMALLOC_MODULE")
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
tools/testing/selftests/vm/config | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/vm/config b/tools/testing/selftests/vm/config
index 1c0d76cb5adfd..93b90a9b1eebf 100644
--- a/tools/testing/selftests/vm/config
+++ b/tools/testing/selftests/vm/config
@@ -1,2 +1,3 @@
CONFIG_SYSVIPC=y
CONFIG_USERFAULTFD=y
+CONFIG_TEST_VMALLOC=m
--
2.20.1
^ permalink raw reply related
* Re: [PATCH for 5.4 0/3] Restartable Sequences Fixes
From: Mathieu Desnoyers @ 2019-12-11 15:28 UTC (permalink / raw)
To: Thomas Gleixner, Shuah Khan
Cc: linux-kernel, Peter Zijlstra, paulmck, Boqun Feng, H. Peter Anvin,
Paul Turner, linux-api, stable
In-Reply-To: <20190917182959.16333-1-mathieu.desnoyers@efficios.com>
Hi Thomas,
I thought those rseq fixes posted in September were in the -tip tree, but it
seems that they never made it to mainline.
Now Shuah Khan noticed the issue with gettid() compatibility with glibc
2.30+. This series contained that fix.
Should I re-post it, or is this series on track to get into mainline
at some point ?
Thanks,
Mathieu
----- On Sep 17, 2019, at 2:29 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
> Hi,
>
> Here is a small set of rseq fixes aiming Linux 5.4. Those should be
> backported to stable kernels >= 4.18.
>
> Thanks,
>
> Mathieu
>
> Mathieu Desnoyers (3):
> rseq: Fix: Reject unknown flags on rseq unregister
> rseq: Fix: Unregister rseq for clone CLONE_VM
> rseq/selftests: Fix: Namespace gettid() for compatibility with glibc
> 2.30
>
> include/linux/sched.h | 4 ++--
> kernel/rseq.c | 2 ++
> tools/testing/selftests/rseq/param_test.c | 18 ++++++++++--------
> 3 files changed, 14 insertions(+), 10 deletions(-)
>
> --
> 2.17.1
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH for 5.4 0/3] Restartable Sequences Fixes
From: Shuah Khan @ 2019-12-11 15:47 UTC (permalink / raw)
To: Mathieu Desnoyers, Thomas Gleixner
Cc: linux-kernel, Peter Zijlstra, paulmck, Boqun Feng, H. Peter Anvin,
Paul Turner, linux-api, stable, Shuah Khan
In-Reply-To: <211848436.2172.1576078102568.JavaMail.zimbra@efficios.com>
On 12/11/19 8:28 AM, Mathieu Desnoyers wrote:
> Hi Thomas,
>
> I thought those rseq fixes posted in September were in the -tip tree, but it
> seems that they never made it to mainline.
>
> Now Shuah Khan noticed the issue with gettid() compatibility with glibc
> 2.30+. This series contained that fix.
>
> Should I re-post it, or is this series on track to get into mainline
> at some point ?
>
It will be great this can make it into 5.5-rc2 or so.
thanks,
-- Shuah
^ permalink raw reply
* [PATCH for 5.5 0/3] Restartable Sequences Fixes
From: Mathieu Desnoyers @ 2019-12-11 16:17 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, Peter Zijlstra, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, stable,
Mathieu Desnoyers
Hi,
Here is a repost of a small set of rseq fixes which was initially posted
in September 2019. It now targets kernel 5.5. Those should be backported
to stable kernels >= 4.18.
Thanks,
Mathieu
Mathieu Desnoyers (3):
rseq: Fix: Reject unknown flags on rseq unregister
rseq: Fix: Unregister rseq for clone CLONE_VM
rseq/selftests: Fix: Namespace gettid() for compatibility with glibc
2.30
include/linux/sched.h | 4 ++--
kernel/rseq.c | 2 ++
tools/testing/selftests/rseq/param_test.c | 18 ++++++++++--------
3 files changed, 14 insertions(+), 10 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH for 5.5 1/3] rseq: Fix: Reject unknown flags on rseq unregister
From: Mathieu Desnoyers @ 2019-12-11 16:17 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, Peter Zijlstra, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, stable,
Mathieu Desnoyers
In-Reply-To: <20191211161713.4490-1-mathieu.desnoyers@efficios.com>
It is preferrable to reject unknown flags within rseq unregistration
rather than to ignore them. It is an oversight caused by the fact that
the check for unknown flags is after the rseq unregister flag check.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Paul Turner <pjt@google.com>
Cc: linux-api@vger.kernel.org
Cc: <stable@vger.kernel.org> # v4.18+
---
kernel/rseq.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 27c48eb7de40..a4f86a9d6937 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -310,6 +310,8 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32, rseq_len,
int ret;
if (flags & RSEQ_FLAG_UNREGISTER) {
+ if (flags & ~RSEQ_FLAG_UNREGISTER)
+ return -EINVAL;
/* Unregister rseq for current thread. */
if (current->rseq != rseq || !current->rseq)
return -EINVAL;
--
2.17.1
^ permalink raw reply related
* [PATCH for 5.5 2/3] rseq: Fix: Unregister rseq for clone CLONE_VM
From: Mathieu Desnoyers @ 2019-12-11 16:17 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, Peter Zijlstra, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, stable,
Mathieu Desnoyers, Dmitry Vyukov, Neel Natu
In-Reply-To: <20191211161713.4490-1-mathieu.desnoyers@efficios.com>
It has been reported by Google that rseq is not behaving properly
with respect to clone when CLONE_VM is used without CLONE_THREAD.
It keeps the prior thread's rseq TLS registered when the TLS of the
thread has moved, so the kernel can corrupt the TLS of the parent.
The approach of clearing the per task-struct rseq registration
on clone with CLONE_THREAD flag is incomplete. It does not cover
the use-case of clone with CLONE_VM set, but without CLONE_THREAD.
Here is the rationale for unregistering rseq on clone with CLONE_VM
flag set:
1) CLONE_THREAD requires CLONE_SIGHAND, which requires CLONE_VM to be
set. Therefore, just checking for CLONE_VM covers all CLONE_THREAD
uses. There is no point in checking for both CLONE_THREAD and
CLONE_VM,
2) There is the possibility of an unlikely scenario where CLONE_SETTLS
is used without CLONE_VM. In order to be an issue, it would require
that the rseq TLS is in a shared memory area.
I do not plan on adding CLONE_SETTLS to the set of clone flags which
unregister RSEQ, because it would require that we also unregister RSEQ
on set_thread_area(2) and arch_prctl(2) ARCH_SET_FS for completeness.
So rather than doing a partial solution, it appears better to let
user-space explicitly perform rseq unregistration across clone if
needed in scenarios where CLONE_VM is not set.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Paul Turner <pjt@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Neel Natu <neelnatu@google.com>
Cc: linux-api@vger.kernel.org
Cc: <stable@vger.kernel.org> # v4.18+
---
include/linux/sched.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 467d26046416..716ad1d8d95e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1929,11 +1929,11 @@ static inline void rseq_migrate(struct task_struct *t)
/*
* If parent process has a registered restartable sequences area, the
- * child inherits. Only applies when forking a process, not a thread.
+ * child inherits. Unregister rseq for a clone with CLONE_VM set.
*/
static inline void rseq_fork(struct task_struct *t, unsigned long clone_flags)
{
- if (clone_flags & CLONE_THREAD) {
+ if (clone_flags & CLONE_VM) {
t->rseq = NULL;
t->rseq_sig = 0;
t->rseq_event_mask = 0;
--
2.17.1
^ permalink raw reply related
* [PATCH for 5.5 3/3] rseq/selftests: Fix: Namespace gettid() for compatibility with glibc 2.30
From: Mathieu Desnoyers @ 2019-12-11 16:17 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, Peter Zijlstra, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, stable,
Mathieu Desnoyers, Shuah Khan, Tommi T . Rantala, Dmitry Vyukov
In-Reply-To: <20191211161713.4490-1-mathieu.desnoyers@efficios.com>
glibc 2.30 introduces gettid() in public headers, which clashes with
the internal static definition within rseq selftests.
Rename gettid() to rseq_gettid() to eliminate this symbol name clash.
Reported-by: Tommi T. Rantala <tommi.t.rantala@nokia.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Tommi T. Rantala <tommi.t.rantala@nokia.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Paul Turner <pjt@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: <stable@vger.kernel.org> # v4.18+
---
tools/testing/selftests/rseq/param_test.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/rseq/param_test.c b/tools/testing/selftests/rseq/param_test.c
index eec2663261f2..e8a657a5f48a 100644
--- a/tools/testing/selftests/rseq/param_test.c
+++ b/tools/testing/selftests/rseq/param_test.c
@@ -15,7 +15,7 @@
#include <errno.h>
#include <stddef.h>
-static inline pid_t gettid(void)
+static inline pid_t rseq_gettid(void)
{
return syscall(__NR_gettid);
}
@@ -373,11 +373,12 @@ void *test_percpu_spinlock_thread(void *arg)
rseq_percpu_unlock(&data->lock, cpu);
#ifndef BENCHMARK
if (i != 0 && !(i % (reps / 10)))
- printf_verbose("tid %d: count %lld\n", (int) gettid(), i);
+ printf_verbose("tid %d: count %lld\n",
+ (int) rseq_gettid(), i);
#endif
}
printf_verbose("tid %d: number of rseq abort: %d, signals delivered: %u\n",
- (int) gettid(), nr_abort, signals_delivered);
+ (int) rseq_gettid(), nr_abort, signals_delivered);
if (!opt_disable_rseq && thread_data->reg &&
rseq_unregister_current_thread())
abort();
@@ -454,11 +455,12 @@ void *test_percpu_inc_thread(void *arg)
} while (rseq_unlikely(ret));
#ifndef BENCHMARK
if (i != 0 && !(i % (reps / 10)))
- printf_verbose("tid %d: count %lld\n", (int) gettid(), i);
+ printf_verbose("tid %d: count %lld\n",
+ (int) rseq_gettid(), i);
#endif
}
printf_verbose("tid %d: number of rseq abort: %d, signals delivered: %u\n",
- (int) gettid(), nr_abort, signals_delivered);
+ (int) rseq_gettid(), nr_abort, signals_delivered);
if (!opt_disable_rseq && thread_data->reg &&
rseq_unregister_current_thread())
abort();
@@ -605,7 +607,7 @@ void *test_percpu_list_thread(void *arg)
}
printf_verbose("tid %d: number of rseq abort: %d, signals delivered: %u\n",
- (int) gettid(), nr_abort, signals_delivered);
+ (int) rseq_gettid(), nr_abort, signals_delivered);
if (!opt_disable_rseq && rseq_unregister_current_thread())
abort();
@@ -796,7 +798,7 @@ void *test_percpu_buffer_thread(void *arg)
}
printf_verbose("tid %d: number of rseq abort: %d, signals delivered: %u\n",
- (int) gettid(), nr_abort, signals_delivered);
+ (int) rseq_gettid(), nr_abort, signals_delivered);
if (!opt_disable_rseq && rseq_unregister_current_thread())
abort();
@@ -1011,7 +1013,7 @@ void *test_percpu_memcpy_buffer_thread(void *arg)
}
printf_verbose("tid %d: number of rseq abort: %d, signals delivered: %u\n",
- (int) gettid(), nr_abort, signals_delivered);
+ (int) rseq_gettid(), nr_abort, signals_delivered);
if (!opt_disable_rseq && rseq_unregister_current_thread())
abort();
--
2.17.1
^ permalink raw reply related
* [PATCH for 5.5 1/1] rseq/selftests: Turn off timeout setting
From: Mathieu Desnoyers @ 2019-12-11 16:28 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, Peter Zijlstra, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, stable,
Mathieu Desnoyers, Shuah Khan, Dmitry Vyukov
As the rseq selftests can run for a long period of time, disable the
timeout that the general selftests have.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Paul Turner <pjt@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
---
tools/testing/selftests/rseq/settings | 1 +
1 file changed, 1 insertion(+)
create mode 100644 tools/testing/selftests/rseq/settings
diff --git a/tools/testing/selftests/rseq/settings b/tools/testing/selftests/rseq/settings
new file mode 100644
index 000000000000..e7b9417537fb
--- /dev/null
+++ b/tools/testing/selftests/rseq/settings
@@ -0,0 +1 @@
+timeout=0
--
2.17.1
^ permalink raw reply related
* Re: [PATCHv8 00/34] kernel: Introduce Time Namespace
From: Dmitry Safonov @ 2019-12-11 20:38 UTC (permalink / raw)
To: Thomas Gleixner, Andy Lutomirski, Ingo Molnar
Cc: Dmitry Safonov, Andrei Vagin, open list, Adrian Reber,
Andrei Vagin, Arnd Bergmann, Christian Brauner, Cyrill Gorcunov,
Eric W. Biederman, H. Peter Anvin, Jann Horn, Jeff Dike,
Oleg Nesterov, Pavel Emelyanov, Shuah Khan, Vincenzo Frascino,
Linux Containers, crml, Linux API, X86 ML
In-Reply-To: <20191121180555.GA440967@gmail.com>
Gentle ping, in case you have time to look at this.
On Thu, 21 Nov 2019 at 18:05, Andrei Vagin <avagin@gmail.com> wrote:
>
> Hi Thomas,
>
> What is your plan on this series? We know you are probably busy with
> the next merge window. We just want to check that this is still in your
> TODO list.
>
> On Tue, Nov 12, 2019 at 01:26:49AM +0000, Dmitry Safonov wrote:
> >
> > v7..v8 Changes:
> > * Fix compile-time errors:
> > - on architectures without the support of time namespaces.
> > - when CONFIG_POSIX_TIMERS isn't set.
> > * Added checks in selftests for CONFIG_POSIX_TIMERS.
> > * Inline do_hres and do_coarse.
> > (And added Tested-by Vincenzo - thanks!)
> > * Make TIME_NS depends on GENERIC_VDSO_TIME_NS and set it per-arch.
> >
> > [v1..v7 Changelogs is at the very bottom here]
> >
> > Our performance measurements show that the price of VDSO's clock_gettime()
> > in a child time namespace is about 8% with a hot CPU cache and about 90%
>
> Here is a typo. The price of VDSO's clock_gettime() in a child time
> namespace is about 12% with a cold CPU cache. The table with
> measurements for a cold CPU cache contains correct data.
>
> > with a cold CPU cache. There is no performance regression for host
> > processes outside time namespace on those tests.
> >
>
> ....
>
> >
> > Cold CPU cache (lesser tsc per cycle - the better):
> >
> > | before | CONFIG_TIME_NS=n | host | inside timens
> > --------------------------------------------------------------
> > tsc | 476 | 480 | 487 | 531
> > stdev(tsc) | 0.6 | 1.3 | 4.3 | 5.7
> > diff (%) | 100 | 100.9 | 102 | 112
> >
Thanks,
Dmitry
^ permalink raw reply
* Re: BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out
From: Greg KH @ 2019-12-12 10:57 UTC (permalink / raw)
To: syzbot
Cc: andriy.shevchenko, asierra, corbet, ext-kimmo.rautkoski, jslaby,
kai.heng.feng, linux-api, linux-doc, linux-kernel, linux-serial,
mika.westerberg, paulburton, peter, sr, syzkaller-bugs,
yamada.masahiro, yegorslists
In-Reply-To: <00000000000003cc8505994f9036@google.com>
On Mon, Dec 09, 2019 at 05:38:01PM -0800, syzbot wrote:
> syzbot has bisected this bug to:
>
> commit bd94c4077a0b2ecc35562c294f80f3659ecd8499
> Author: Masahiro Yamada <yamada.masahiro@socionext.com>
> Date: Wed Oct 28 03:46:05 2015 +0000
>
> serial: support 16-bit register interface for console
That would be because that is when this function was added to the kernel
:)
Again, you are asking the kernel to write to a bad place in memory, and
then crash when that happens. That sounds like the correct
functionality to me...
thanks,
greg k-h
^ permalink raw reply
* Re: BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out
From: Dmitry Vyukov @ 2019-12-13 9:05 UTC (permalink / raw)
To: Greg KH
Cc: syzbot, Andy Shevchenko, asierra, Jonathan Corbet,
ext-kimmo.rautkoski, Jiri Slaby, kai heng feng, Linux API,
open list:DOCUMENTATION, LKML, linux-serial, mika.westerberg,
paulburton, Peter Hurley, sr, syzkaller-bugs, Masahiro Yamada,
yegorslists
In-Reply-To: <20191212105754.GC1476206@kroah.com>
On Thu, Dec 12, 2019 at 11:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Dec 09, 2019 at 05:38:01PM -0800, syzbot wrote:
> > syzbot has bisected this bug to:
> >
> > commit bd94c4077a0b2ecc35562c294f80f3659ecd8499
> > Author: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Date: Wed Oct 28 03:46:05 2015 +0000
> >
> > serial: support 16-bit register interface for console
>
> That would be because that is when this function was added to the kernel
> :)
>
> Again, you are asking the kernel to write to a bad place in memory, and
> then crash when that happens. That sounds like the correct
> functionality to me...
This looks like:
#syz dup:
BUG: unable to handle kernel NULL pointer dereference in mem_serial_out
Let's continue in that thread.
^ permalink raw reply
* Re: [PATCH v7 1/1] ns: add binfmt_misc to the user namespace
From: Henning Schild @ 2019-12-13 17:51 UTC (permalink / raw)
To: Laurent Vivier
Cc: linux-kernel, Dmitry Safonov, linux-fsdevel, James Bottomley,
Eric Biederman, linux-api, Andrei Vagin, Cédric Le Goater,
Greg Kurz, Jann Horn, containers, Alexander Viro, Jan Kiszka
In-Reply-To: <7cb245ed-f738-7991-a09b-b27152274b9f@vivier.eu>
Hi all,
that is a very useful contribution, which will hopefully be considered.
Tested-by: Henning Schild <henning.schild@siemens.com>
Having a private binfmt_misc allows having different interpreters
(paths to them) for the same format in each namespace. Imagine chroots
of different architectures where the static qemu is in a different
place i.e. /usr/bin/qemu-arm-static, vs
/sbin/qemu-arm-static, or /home/dev/qemu/qemu-arm-static
And it also eventually allows setting up binfmt_misc for a chroot
without requiring root.
Henning
Am Tue, 3 Dec 2019 10:44:44 +0100
schrieb Laurent Vivier <laurent@vivier.eu>:
> Ping
>
> Thanks,
> Laurent
>
> Le 07/11/2019 à 15:03, Laurent Vivier a écrit :
> > This patch allows to have a different binfmt_misc configuration
> > for each new user namespace. By default, the binfmt_misc
> > configuration is the one of the previous level, but if the
> > binfmt_misc filesystem is mounted in the new namespace a new empty
> > binfmt instance is created and used in this namespace.
> >
> > For instance, using "unshare" we can start a chroot of another
> > architecture and configure the binfmt_misc interpreter without
> > being root to run the binaries in this chroot.
> >
> > Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> > Acked-by: Andrei Vagin <avagin@gmail.com>
> > ---
> > fs/binfmt_misc.c | 115
> > +++++++++++++++++++++++++-------- include/linux/user_namespace.h |
> > 15 +++++ kernel/user.c | 14 ++++
> > kernel/user_namespace.c | 3 +
> > 4 files changed, 119 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> > index cdb45829354d..ba5f0d2ade96 100644
> > --- a/fs/binfmt_misc.c
> > +++ b/fs/binfmt_misc.c
> > @@ -40,9 +40,6 @@ enum {
> > VERBOSE_STATUS = 1 /* make it zero to save 400 bytes
> > kernel memory */ };
> >
> > -static LIST_HEAD(entries);
> > -static int enabled = 1;
> > -
> > enum {Enabled, Magic};
> > #define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
> > #define MISC_FMT_OPEN_BINARY (1 << 30)
> > @@ -62,10 +59,7 @@ typedef struct {
> > struct file *interp_file;
> > } Node;
> >
> > -static DEFINE_RWLOCK(entries_lock);
> > static struct file_system_type bm_fs_type;
> > -static struct vfsmount *bm_mnt;
> > -static int entry_count;
> >
> > /*
> > * Max length of the register string. Determined by:
> > @@ -82,18 +76,37 @@ static int entry_count;
> > */
> > #define MAX_REGISTER_LENGTH 1920
> >
> > +static struct binfmt_namespace *binfmt_ns(struct user_namespace
> > *ns) +{
> > + struct binfmt_namespace *b_ns;
> > +
> > + while (ns) {
> > + b_ns = READ_ONCE(ns->binfmt_ns);
> > + if (b_ns)
> > + return b_ns;
> > + ns = ns->parent;
> > + }
> > + /* as the first user namespace is initialized with
> > + * &init_binfmt_ns we should never come here
> > + * but we try to stay safe by logging a warning
> > + * and returning a sane value
> > + */
> > + WARN_ON_ONCE(1);
> > + return &init_binfmt_ns;
> > +}
> > +
> > /*
> > * Check if we support the binfmt
> > * if we do, return the node, else NULL
> > * locking is done in load_misc_binary
> > */
> > -static Node *check_file(struct linux_binprm *bprm)
> > +static Node *check_file(struct binfmt_namespace *ns, struct
> > linux_binprm *bprm) {
> > char *p = strrchr(bprm->interp, '.');
> > struct list_head *l;
> >
> > /* Walk all the registered handlers. */
> > - list_for_each(l, &entries) {
> > + list_for_each(l, &ns->entries) {
> > Node *e = list_entry(l, Node, list);
> > char *s;
> > int j;
> > @@ -135,17 +148,18 @@ static int load_misc_binary(struct
> > linux_binprm *bprm) struct file *interp_file = NULL;
> > int retval;
> > int fd_binary = -1;
> > + struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
> >
> > retval = -ENOEXEC;
> > - if (!enabled)
> > + if (!ns->enabled)
> > return retval;
> >
> > /* to keep locking time low, we copy the interpreter
> > string */
> > - read_lock(&entries_lock);
> > - fmt = check_file(bprm);
> > + read_lock(&ns->entries_lock);
> > + fmt = check_file(ns, bprm);
> > if (fmt)
> > dget(fmt->dentry);
> > - read_unlock(&entries_lock);
> > + read_unlock(&ns->entries_lock);
> > if (!fmt)
> > return retval;
> >
> > @@ -611,19 +625,19 @@ static void bm_evict_inode(struct inode
> > *inode) kfree(e);
> > }
> >
> > -static void kill_node(Node *e)
> > +static void kill_node(struct binfmt_namespace *ns, Node *e)
> > {
> > struct dentry *dentry;
> >
> > - write_lock(&entries_lock);
> > + write_lock(&ns->entries_lock);
> > list_del_init(&e->list);
> > - write_unlock(&entries_lock);
> > + write_unlock(&ns->entries_lock);
> >
> > dentry = e->dentry;
> > drop_nlink(d_inode(dentry));
> > d_drop(dentry);
> > dput(dentry);
> > - simple_release_fs(&bm_mnt, &entry_count);
> > + simple_release_fs(&ns->bm_mnt, &ns->entry_count);
> > }
> >
> > /* /<entry> */
> > @@ -653,6 +667,9 @@ static ssize_t bm_entry_write(struct file
> > *file, const char __user *buffer, struct dentry *root;
> > Node *e = file_inode(file)->i_private;
> > int res = parse_command(buffer, count);
> > + struct binfmt_namespace *ns;
> > +
> > + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> >
> > switch (res) {
> > case 1:
> > @@ -669,7 +686,7 @@ static ssize_t bm_entry_write(struct file
> > *file, const char __user *buffer, inode_lock(d_inode(root));
> >
> > if (!list_empty(&e->list))
> > - kill_node(e);
> > + kill_node(ns, e);
> >
> > inode_unlock(d_inode(root));
> > break;
> > @@ -695,6 +712,7 @@ static ssize_t bm_register_write(struct file
> > *file, const char __user *buffer, struct inode *inode;
> > struct super_block *sb = file_inode(file)->i_sb;
> > struct dentry *root = sb->s_root, *dentry;
> > + struct binfmt_namespace *ns;
> > int err = 0;
> >
> > e = create_entry(buffer, count);
> > @@ -718,7 +736,9 @@ static ssize_t bm_register_write(struct file
> > *file, const char __user *buffer, if (!inode)
> > goto out2;
> >
> > - err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
> > + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> > + err = simple_pin_fs(&bm_fs_type, &ns->bm_mnt,
> > + &ns->entry_count);
> > if (err) {
> > iput(inode);
> > inode = NULL;
> > @@ -727,12 +747,16 @@ static ssize_t bm_register_write(struct file
> > *file, const char __user *buffer,
> > if (e->flags & MISC_FMT_OPEN_FILE) {
> > struct file *f;
> > + const struct cred *old_cred;
> >
> > + old_cred = override_creds(file->f_cred);
> > f = open_exec(e->interpreter);
> > + revert_creds(old_cred);
> > if (IS_ERR(f)) {
> > err = PTR_ERR(f);
> > pr_notice("register: failed to install
> > interpreter file %s\n", e->interpreter);
> > - simple_release_fs(&bm_mnt, &entry_count);
> > + simple_release_fs(&ns->bm_mnt,
> > + &ns->entry_count);
> > iput(inode);
> > inode = NULL;
> > goto out2;
> > @@ -745,9 +769,9 @@ static ssize_t bm_register_write(struct file
> > *file, const char __user *buffer, inode->i_fop =
> > &bm_entry_operations;
> > d_instantiate(dentry, inode);
> > - write_lock(&entries_lock);
> > - list_add(&e->list, &entries);
> > - write_unlock(&entries_lock);
> > + write_lock(&ns->entries_lock);
> > + list_add(&e->list, &ns->entries);
> > + write_unlock(&ns->entries_lock);
> >
> > err = 0;
> > out2:
> > @@ -772,7 +796,9 @@ static const struct file_operations
> > bm_register_operations = { static ssize_t
> > bm_status_read(struct file *file, char __user *buf, size_t nbytes,
> > loff_t *ppos) {
> > - char *s = enabled ? "enabled\n" : "disabled\n";
> > + struct binfmt_namespace *ns =
> > +
> > binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> > + char *s = ns->enabled ? "enabled\n" : "disabled\n";
> >
> > return simple_read_from_buffer(buf, nbytes, ppos, s,
> > strlen(s)); }
> > @@ -780,25 +806,28 @@ bm_status_read(struct file *file, char __user
> > *buf, size_t nbytes, loff_t *ppos) static ssize_t
> > bm_status_write(struct file *file, const char __user *buffer,
> > size_t count, loff_t *ppos) {
> > + struct binfmt_namespace *ns;
> > int res = parse_command(buffer, count);
> > struct dentry *root;
> >
> > + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> > switch (res) {
> > case 1:
> > /* Disable all handlers. */
> > - enabled = 0;
> > + ns->enabled = 0;
> > break;
> > case 2:
> > /* Enable all handlers. */
> > - enabled = 1;
> > + ns->enabled = 1;
> > break;
> > case 3:
> > /* Delete all handlers. */
> > root = file_inode(file)->i_sb->s_root;
> > inode_lock(d_inode(root));
> >
> > - while (!list_empty(&entries))
> > - kill_node(list_first_entry(&entries, Node,
> > list));
> > + while (!list_empty(&ns->entries))
> > + kill_node(ns,
> > list_first_entry(&ns->entries,
> > + Node,
> > list));
> > inode_unlock(d_inode(root));
> > break;
> > @@ -825,24 +854,53 @@ static const struct super_operations s_ops = {
> > static int bm_fill_super(struct super_block *sb, struct fs_context
> > *fc) {
> > int err;
> > + struct user_namespace *ns = sb->s_user_ns;
> > static const struct tree_descr bm_files[] = {
> > [2] = {"status", &bm_status_operations,
> > S_IWUSR|S_IRUGO}, [3] = {"register", &bm_register_operations,
> > S_IWUSR}, /* last one */ {""}
> > };
> >
> > + /* create a new binfmt namespace
> > + * if we are not in the first user namespace
> > + * but the binfmt namespace is the first one
> > + */
> > + if (READ_ONCE(ns->binfmt_ns) == NULL) {
> > + struct binfmt_namespace *new_ns;
> > +
> > + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> > + GFP_KERNEL);
> > + if (new_ns == NULL)
> > + return -ENOMEM;
> > + INIT_LIST_HEAD(&new_ns->entries);
> > + new_ns->enabled = 1;
> > + rwlock_init(&new_ns->entries_lock);
> > + new_ns->bm_mnt = NULL;
> > + new_ns->entry_count = 0;
> > + /* ensure new_ns is completely initialized before
> > sharing it */
> > + smp_wmb();
> > + WRITE_ONCE(ns->binfmt_ns, new_ns);
> > + }
> > +
> > err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
> > if (!err)
> > sb->s_op = &s_ops;
> > return err;
> > }
> >
> > +static void bm_free(struct fs_context *fc)
> > +{
> > + if (fc->s_fs_info)
> > + put_user_ns(fc->s_fs_info);
> > +}
> > +
> > static int bm_get_tree(struct fs_context *fc)
> > {
> > - return get_tree_single(fc, bm_fill_super);
> > + return get_tree_keyed(fc, bm_fill_super,
> > get_user_ns(fc->user_ns)); }
> >
> > static const struct fs_context_operations bm_context_ops = {
> > + .free = bm_free,
> > .get_tree = bm_get_tree,
> > };
> >
> > @@ -861,6 +919,7 @@ static struct file_system_type bm_fs_type = {
> > .owner = THIS_MODULE,
> > .name = "binfmt_misc",
> > .init_fs_context = bm_init_fs_context,
> > + .fs_flags = FS_USERNS_MOUNT,
> > .kill_sb = kill_litter_super,
> > };
> > MODULE_ALIAS_FS("binfmt_misc");
> > diff --git a/include/linux/user_namespace.h
> > b/include/linux/user_namespace.h index fb9f4f799554..16e6f3a97a01
> > 100644 --- a/include/linux/user_namespace.h
> > +++ b/include/linux/user_namespace.h
> > @@ -52,6 +52,18 @@ enum ucount_type {
> > UCOUNT_COUNTS,
> > };
> >
> > +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> > +struct binfmt_namespace {
> > + struct list_head entries;
> > + rwlock_t entries_lock;
> > + int enabled;
> > + struct vfsmount *bm_mnt;
> > + int entry_count;
> > +} __randomize_layout;
> > +
> > +extern struct binfmt_namespace init_binfmt_ns;
> > +#endif
> > +
> > struct user_namespace {
> > struct uid_gid_map uid_map;
> > struct uid_gid_map gid_map;
> > @@ -86,6 +98,9 @@ struct user_namespace {
> > #endif
> > struct ucounts *ucounts;
> > int ucount_max[UCOUNT_COUNTS];
> > +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> > + struct binfmt_namespace *binfmt_ns;
> > +#endif
> > } __randomize_layout;
> >
> > struct ucounts {
> > diff --git a/kernel/user.c b/kernel/user.c
> > index 5235d7f49982..092b2b4d47a6 100644
> > --- a/kernel/user.c
> > +++ b/kernel/user.c
> > @@ -20,6 +20,17 @@
> > #include <linux/user_namespace.h>
> > #include <linux/proc_ns.h>
> >
> > +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> > +struct binfmt_namespace init_binfmt_ns = {
> > + .entries = LIST_HEAD_INIT(init_binfmt_ns.entries),
> > + .enabled = 1,
> > + .entries_lock =
> > __RW_LOCK_UNLOCKED(init_binfmt_ns.entries_lock),
> > + .bm_mnt = NULL,
> > + .entry_count = 0,
> > +};
> > +EXPORT_SYMBOL_GPL(init_binfmt_ns);
> > +#endif
> > +
> > /*
> > * userns count is 1 for root user, 1 for init_uts_ns,
> > * and 1 for... ?
> > @@ -67,6 +78,9 @@ struct user_namespace init_user_ns = {
> > .keyring_name_list =
> > LIST_HEAD_INIT(init_user_ns.keyring_name_list), .keyring_sem =
> > __RWSEM_INITIALIZER(init_user_ns.keyring_sem), #endif
> > +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> > + .binfmt_ns = &init_binfmt_ns,
> > +#endif
> > };
> > EXPORT_SYMBOL_GPL(init_user_ns);
> >
> > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> > index 8eadadc478f9..f42c32269e20 100644
> > --- a/kernel/user_namespace.c
> > +++ b/kernel/user_namespace.c
> > @@ -191,6 +191,9 @@ static void free_user_ns(struct work_struct
> > *work) kfree(ns->projid_map.forward);
> > kfree(ns->projid_map.reverse);
> > }
> > +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> > + kfree(ns->binfmt_ns);
> > +#endif
> > retire_userns_sysctls(ns);
> > key_free_user_ns(ns);
> > ns_free_inum(&ns->ns);
> >
>
^ permalink raw reply
* Re: [PATCH v7 1/1] ns: add binfmt_misc to the user namespace
From: James Bottomley @ 2019-12-13 19:59 UTC (permalink / raw)
To: Henning Schild, Laurent Vivier
Cc: linux-kernel, Dmitry Safonov, linux-fsdevel, Eric Biederman,
linux-api, Andrei Vagin, Cédric Le Goater, Greg Kurz,
Jann Horn, containers, Alexander Viro, Jan Kiszka
In-Reply-To: <20191213185110.06b52cf4@md1za8fc.ad001.siemens.net>
On Fri, 2019-12-13 at 18:51 +0100, Henning Schild wrote:
> Hi all,
>
> that is a very useful contribution, which will hopefully be
> considered.
I'm technically the maintainer on the you touched it last you own it
basis, so if Christian's concerns get addressed I'll shepherd it
upstream.
James
^ permalink raw reply
* [PATCH v2 23/24] y2038: rename itimerval to __kernel_old_itimerval
From: Arnd Bergmann @ 2019-12-13 20:53 UTC (permalink / raw)
To: y2038, linux-kernel, Thomas Gleixner
Cc: Arnd Bergmann, David Howells, Al Viro, Christian Brauner,
Heiko Carstens, Jens Axboe, Deepa Dinamani, David S. Miller,
Frederic Weisbecker, Peter Zijlstra (Intel), Anna-Maria Gleixner,
linux-api
In-Reply-To: <20191213204936.3643476-1-arnd@arndb.de>
Take the renaming of timeval and timespec one level further,
also renaming itimerval to __kernel_old_itimerval, to avoid
namespace conflicts with the user-space structure that may
use 64-bit time_t members.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
include/linux/syscalls.h | 9 ++++-----
include/uapi/linux/time_types.h | 5 +++++
kernel/time/itimer.c | 18 +++++++++---------
3 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d0391cc2dae9..27245fec2a8a 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -16,8 +16,7 @@ struct inode;
struct iocb;
struct io_event;
struct iovec;
-struct itimerspec;
-struct itimerval;
+struct __kernel_old_itimerval;
struct kexec_segment;
struct linux_dirent;
struct linux_dirent64;
@@ -591,10 +590,10 @@ asmlinkage long sys_nanosleep_time32(struct old_timespec32 __user *rqtp,
struct old_timespec32 __user *rmtp);
/* kernel/itimer.c */
-asmlinkage long sys_getitimer(int which, struct itimerval __user *value);
+asmlinkage long sys_getitimer(int which, struct __kernel_old_itimerval __user *value);
asmlinkage long sys_setitimer(int which,
- struct itimerval __user *value,
- struct itimerval __user *ovalue);
+ struct __kernel_old_itimerval __user *value,
+ struct __kernel_old_itimerval __user *ovalue);
/* kernel/kexec.c */
asmlinkage long sys_kexec_load(unsigned long entry, unsigned long nr_segments,
diff --git a/include/uapi/linux/time_types.h b/include/uapi/linux/time_types.h
index 074e391d73a1..bcc0002115d3 100644
--- a/include/uapi/linux/time_types.h
+++ b/include/uapi/linux/time_types.h
@@ -33,6 +33,11 @@ struct __kernel_old_timespec {
long tv_nsec; /* nanoseconds */
};
+struct __kernel_old_itimerval {
+ struct __kernel_old_timeval it_interval;/* timer interval */
+ struct __kernel_old_timeval it_value; /* current value */
+};
+
struct __kernel_sock_timeval {
__s64 tv_sec;
__s64 tv_usec;
diff --git a/kernel/time/itimer.c b/kernel/time/itimer.c
index 9e59c9ea92aa..ca4e6d57d68b 100644
--- a/kernel/time/itimer.c
+++ b/kernel/time/itimer.c
@@ -97,20 +97,20 @@ static int do_getitimer(int which, struct itimerspec64 *value)
return 0;
}
-static int put_itimerval(struct itimerval __user *o,
+static int put_itimerval(struct __kernel_old_itimerval __user *o,
const struct itimerspec64 *i)
{
- struct itimerval v;
+ struct __kernel_old_itimerval v;
v.it_interval.tv_sec = i->it_interval.tv_sec;
v.it_interval.tv_usec = i->it_interval.tv_nsec / NSEC_PER_USEC;
v.it_value.tv_sec = i->it_value.tv_sec;
v.it_value.tv_usec = i->it_value.tv_nsec / NSEC_PER_USEC;
- return copy_to_user(o, &v, sizeof(struct itimerval)) ? -EFAULT : 0;
+ return copy_to_user(o, &v, sizeof(struct __kernel_old_itimerval)) ? -EFAULT : 0;
}
-SYSCALL_DEFINE2(getitimer, int, which, struct itimerval __user *, value)
+SYSCALL_DEFINE2(getitimer, int, which, struct __kernel_old_itimerval __user *, value)
{
struct itimerspec64 get_buffer;
int error = do_getitimer(which, &get_buffer);
@@ -314,11 +314,11 @@ SYSCALL_DEFINE1(alarm, unsigned int, seconds)
#endif
-static int get_itimerval(struct itimerspec64 *o, const struct itimerval __user *i)
+static int get_itimerval(struct itimerspec64 *o, const struct __kernel_old_itimerval __user *i)
{
- struct itimerval v;
+ struct __kernel_old_itimerval v;
- if (copy_from_user(&v, i, sizeof(struct itimerval)))
+ if (copy_from_user(&v, i, sizeof(struct __kernel_old_itimerval)))
return -EFAULT;
/* Validate the timevals in value. */
@@ -333,8 +333,8 @@ static int get_itimerval(struct itimerspec64 *o, const struct itimerval __user *
return 0;
}
-SYSCALL_DEFINE3(setitimer, int, which, struct itimerval __user *, value,
- struct itimerval __user *, ovalue)
+SYSCALL_DEFINE3(setitimer, int, which, struct __kernel_old_itimerval __user *, value,
+ struct __kernel_old_itimerval __user *, ovalue)
{
struct itimerspec64 set_buffer, get_buffer;
int error;
--
2.20.0
^ permalink raw reply related
* [PATCH] openat2: switch to __attribute__((packed)) for open_how
From: Aleksa Sarai @ 2019-12-13 22:23 UTC (permalink / raw)
To: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan
Cc: Aleksa Sarai, dev, containers, linux-api, linux-fsdevel,
linux-kernel, linux-kselftest
The design of the original open_how struct layout was such that it
ensured that there would be no un-labelled (and thus potentially
non-zero) padding to avoid issues with struct expansion, as well as
providing a uniform representation on all architectures (to avoid
complications with OPEN_HOW_SIZE versioning).
However, there were a few other desirable features which were not
fulfilled by the previous struct layout:
* Adding new features (other than new flags) should always result in
the struct getting larger. However, by including a padding field, it
was possible for new fields to be added without expanding the
structure. This would somewhat complicate version-number based
checking of feature support.
* A non-zero bit in __padding yielded -EINVAL when it should arguably
have been -E2BIG (because the padding bits are effectively
yet-to-be-used fields). However, the semantics are not entirely clear
because userspace may expect -E2BIG to only signify that the
structure is too big. It's much simpler to just provide the guarantee
that new fields will always result in a struct size increase, and
-E2BIG indicates you're using a field that's too recent for an older
kernel.
* While the alignment for u64s was manually backed by extra padding
fields, some languages (such as Rust) do not currently support
enforcing alignment of struct field members.
* The padding wasted space needlessly, and would very likely not be
used up entirely by future extensions for a long time (because it
couldn't fit a u64).
While none of these outstanding issues are deal-breakers, we can iron
out these warts before openat2(2) lands in Linus's tree. Instead of
using alignment and padding, we simply pack the structure with
__attribute__((packed)). Rust supports #[repr(packed)] and it removes
all of the issues with having explicit padding.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/open.c | 2 --
include/uapi/linux/fcntl.h | 11 +++++------
tools/testing/selftests/openat2/helpers.h | 11 +++++------
tools/testing/selftests/openat2/openat2_test.c | 18 +-----------------
4 files changed, 11 insertions(+), 31 deletions(-)
diff --git a/fs/open.c b/fs/open.c
index 50a46501bcc9..8cdb2b675867 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -993,8 +993,6 @@ static inline int build_open_flags(const struct open_how *how,
return -EINVAL;
if (how->resolve & ~VALID_RESOLVE_FLAGS)
return -EINVAL;
- if (memchr_inv(how->__padding, 0, sizeof(how->__padding)))
- return -EINVAL;
/* Deal with the mode. */
if (WILL_CREATE(flags)) {
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index d886bdb585e4..0e070c7f568a 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -109,17 +109,16 @@
* O_TMPFILE} are set.
*
* @flags: O_* flags.
- * @mode: O_CREAT/O_TMPFILE file mode.
* @resolve: RESOLVE_* flags.
+ * @mode: O_CREAT/O_TMPFILE file mode.
*/
struct open_how {
- __aligned_u64 flags;
+ __u64 flags;
+ __u64 resolve;
__u16 mode;
- __u16 __padding[3]; /* must be zeroed */
- __aligned_u64 resolve;
-};
+} __attribute__((packed));
-#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */
+#define OPEN_HOW_SIZE_VER0 18 /* sizeof first published struct */
#define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0
/* how->resolve flags for openat2(2). */
diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
index 43ca5ceab6e3..eb1535c8fa2e 100644
--- a/tools/testing/selftests/openat2/helpers.h
+++ b/tools/testing/selftests/openat2/helpers.h
@@ -32,17 +32,16 @@
* O_TMPFILE} are set.
*
* @flags: O_* flags.
- * @mode: O_CREAT/O_TMPFILE file mode.
* @resolve: RESOLVE_* flags.
+ * @mode: O_CREAT/O_TMPFILE file mode.
*/
struct open_how {
- __aligned_u64 flags;
+ __u64 flags;
+ __u64 resolve;
__u16 mode;
- __u16 __padding[3]; /* must be zeroed */
- __aligned_u64 resolve;
-};
+} __attribute__((packed));
-#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */
+#define OPEN_HOW_SIZE_VER0 18 /* sizeof first published struct */
#define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0
bool needs_openat2(const struct open_how *how);
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 0b64fedc008b..cbf95d160b1b 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -40,7 +40,7 @@ struct struct_test {
int err;
};
-#define NUM_OPENAT2_STRUCT_TESTS 10
+#define NUM_OPENAT2_STRUCT_TESTS 7
#define NUM_OPENAT2_STRUCT_VARIATIONS 13
void test_openat2_struct(void)
@@ -57,22 +57,6 @@ void test_openat2_struct(void)
.arg.inner.flags = O_RDONLY,
.size = sizeof(struct open_how_ext) },
- /* Normal struct with broken padding. */
- { .name = "normal struct (non-zero padding[0])",
- .arg.inner.flags = O_RDONLY,
- .arg.inner.__padding = {0xa0, 0x00, 0x00},
- .size = sizeof(struct open_how_ext), .err = -EINVAL },
- { .name = "normal struct (non-zero padding[1])",
- .arg.inner.flags = O_RDONLY,
- .arg.inner.__padding = {0x00, 0x1a, 0x00},
- .size = sizeof(struct open_how_ext), .err = -EINVAL },
- { .name = "normal struct (non-zero padding[2])",
- .arg.inner.flags = O_RDONLY,
- .arg.inner.__padding = {0x00, 0x00, 0xef},
- .size = sizeof(struct open_how_ext), .err = -EINVAL },
-
- /* TODO: Once expanded, check zero-padding. */
-
/* Smaller than version-0 struct. */
{ .name = "zero-sized 'struct'",
.arg.inner.flags = O_RDONLY, .size = 0, .err = -EINVAL },
base-commit: 912dfe068c43fa13c587b8d30e73d335c5ba7d44
--
2.24.0
^ permalink raw reply related
* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
From: Michael Kerrisk (man-pages) @ 2019-12-14 1:55 UTC (permalink / raw)
To: Yang Shi, John Hubbard, cl, mhocko, cai, akpm
Cc: mtk.manpages, linux-man, linux-api, linux-mm, linux-kernel
In-Reply-To: <95170ea5-5b62-9168-fcd9-93b43330a1b4@linux.alibaba.com>
On 12/6/19 6:26 PM, Yang Shi wrote:
>
>
> On 12/6/19 12:25 AM, John Hubbard wrote:
>> On 12/5/19 5:34 PM, Yang Shi wrote:
>>> Since commit e78bbfa82624 ("mm: stop returning -ENOENT
>>> from sys_move_pages() if nothing got migrated"), move_pages doesn't
>>> return -ENOENT anymore if the pages are already on the target nodes, but
>>> this change is never reflected in manpage.
>>>
>>> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
>>> Cc: Christoph Lameter <cl@linux.com>
>>> Cc: John Hubbard <jhubbard@nvidia.com>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: Qian Cai <cai@lca.pw>
>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>>> ---
>>> man2/move_pages.2 | 5 ++---
>>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/man2/move_pages.2 b/man2/move_pages.2
>>> index 2d96468..2a2f3cd 100644
>>> --- a/man2/move_pages.2
>>> +++ b/man2/move_pages.2
>>> @@ -192,9 +192,8 @@ was specified or an attempt was made to migrate
>>> pages of a kernel thread.
>>> One of the target nodes is not online.
>>> .TP
>>> .B ENOENT
>>> -No pages were found that require moving.
>>> -All pages are either already
>>> -on the target node, not present, had an invalid address or could not be
>>> +No pages were found.
>>> +All pages are either not present, had an invalid address or could
>>> not be
>>> moved because they were mapped by multiple processes.
>>> .TP
>>> .B EPERM
>>>
>>
>> whoa, hold on. If I'm reading through the various error paths
>> correctly, then this
>> code is *never* going to return ENOENT for the whole function. It can
>> fill in that
>> value per-page, in the status array, but that's all. Did I get that
>> right?
>
> Nice catch. Yes, you are right.
>
>>
>> If so, we need to redo this part of the man page.
>
> Yes.
So where are things at with this? Is an improved man-pages
patch on the way, or is some other action (on the API) planned?
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH v7 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2019-12-14 11:34 UTC (permalink / raw)
To: James Bottomley, Henning Schild
Cc: linux-kernel, Dmitry Safonov, linux-fsdevel, Eric Biederman,
linux-api, Andrei Vagin, Cédric Le Goater, Greg Kurz,
Jann Horn, containers, Alexander Viro, Jan Kiszka
In-Reply-To: <1576267177.4060.4.camel@HansenPartnership.com>
Le 13/12/2019 à 20:59, James Bottomley a écrit :
> On Fri, 2019-12-13 at 18:51 +0100, Henning Schild wrote:
>> Hi all,
>>
>> that is a very useful contribution, which will hopefully be
>> considered.
>
> I'm technically the maintainer on the you touched it last you own it
> basis, so if Christian's concerns get addressed I'll shepherd it
> upstream.
Thank you.
I update this in the next days and re-send the patch.
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH v7 1/1] ns: add binfmt_misc to the user namespace
From: Michael Kerrisk (man-pages) @ 2019-12-14 12:32 UTC (permalink / raw)
To: Laurent Vivier
Cc: James Bottomley, Henning Schild, lkml, Dmitry Safonov,
linux-fsdevel@vger.kernel.org, Eric Biederman, Linux API,
Andrei Vagin, Cédric Le Goater, Greg Kurz, Jann Horn,
Containers, Alexander Viro, Jan Kiszka
In-Reply-To: <3205e74b-71f1-14f4-b784-d878b3ef697f@vivier.eu>
Hello Laurent,
On Sat, 14 Dec 2019 at 12:35, Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 13/12/2019 à 20:59, James Bottomley a écrit :
> > On Fri, 2019-12-13 at 18:51 +0100, Henning Schild wrote:
> >> Hi all,
> >>
> >> that is a very useful contribution, which will hopefully be
> >> considered.
> >
> > I'm technically the maintainer on the you touched it last you own it
> > basis, so if Christian's concerns get addressed I'll shepherd it
> > upstream.
>
> Thank you.
>
> I update this in the next days and re-send the patch.
Would you also be so kind as to craft a patch for the
user_namespaces(7) manual page describing the changes (sent to me,
linux-man@vger.kernel.org, and the other parties already in CC)?
If you do not have the time to familiarize yourself with groff/man
markup, a patch that uses plain text is fine; I can handle the
formatting.
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH] openat2: switch to __attribute__((packed)) for open_how
From: Rasmus Villemoes @ 2019-12-14 22:17 UTC (permalink / raw)
To: Aleksa Sarai, Alexander Viro, Jeff Layton, J. Bruce Fields,
Shuah Khan
Cc: dev, containers, linux-api, linux-fsdevel, linux-kernel,
linux-kselftest
In-Reply-To: <20191213222351.14071-1-cyphar@cyphar.com>
On 13/12/2019 23.23, Aleksa Sarai wrote:
> The design of the original open_how struct layout was such that it
> ensured that there would be no un-labelled (and thus potentially
> non-zero) padding to avoid issues with struct expansion, as well as
> providing a uniform representation on all architectures (to avoid
> complications with OPEN_HOW_SIZE versioning).
>
> However, there were a few other desirable features which were not
> fulfilled by the previous struct layout:
>
> * Adding new features (other than new flags) should always result in
> the struct getting larger. However, by including a padding field, it
> was possible for new fields to be added without expanding the
> structure. This would somewhat complicate version-number based
> checking of feature support.
>
> * A non-zero bit in __padding yielded -EINVAL when it should arguably
> have been -E2BIG (because the padding bits are effectively
> yet-to-be-used fields). However, the semantics are not entirely clear
> because userspace may expect -E2BIG to only signify that the
> structure is too big. It's much simpler to just provide the guarantee
> that new fields will always result in a struct size increase, and
> -E2BIG indicates you're using a field that's too recent for an older
> kernel.
And when the first extension adds another u64 field, that padding has to
be added back in and checked for being 0, at which point the padding is
again yet-to-be-used fields. So what exactly is the problem with
returning EINVAL now?
> * The padding wasted space needlessly, and would very likely not be
> used up entirely by future extensions for a long time (because it
> couldn't fit a u64).
Who knows, it does fit a u32. And if the struct is to be 8-byte aligned
(see below), it doesn't actually waste space.
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index d886bdb585e4..0e070c7f568a 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -109,17 +109,16 @@
> * O_TMPFILE} are set.
> *
> * @flags: O_* flags.
> - * @mode: O_CREAT/O_TMPFILE file mode.
> * @resolve: RESOLVE_* flags.
> + * @mode: O_CREAT/O_TMPFILE file mode.
> */
> struct open_how {
> - __aligned_u64 flags;
> + __u64 flags;
> + __u64 resolve;
> __u16 mode;
> - __u16 __padding[3]; /* must be zeroed */
> - __aligned_u64 resolve;
> -};
> +} __attribute__((packed));
IIRC, gcc assumes such a struct has alignment 1, which means that it
will generate horrible code to access it. So if you do this (and I don't
think it's a good idea), I think you'd also want to include a
__attribute__((__aligned__(8))) - or perhaps that can be accomplished by
just keeping flags as an explicitly aligned member. But that will of
course bump its sizeof() back to 24, at which point it seems better to
just make the padding explicit.
Rasmus
^ permalink raw reply
* Re: [PATCH] openat2: switch to __attribute__((packed)) for open_how
From: Aleksa Sarai @ 2019-12-15 12:34 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan, dev,
containers, linux-api, linux-fsdevel, linux-kernel,
linux-kselftest
In-Reply-To: <a328b91d-fd8f-4f27-b3c2-91a9c45f18c0@rasmusvillemoes.dk>
[-- Attachment #1: Type: text/plain, Size: 7559 bytes --]
On 2019-12-14, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> On 13/12/2019 23.23, Aleksa Sarai wrote:
> > The design of the original open_how struct layout was such that it
> > ensured that there would be no un-labelled (and thus potentially
> > non-zero) padding to avoid issues with struct expansion, as well as
> > providing a uniform representation on all architectures (to avoid
> > complications with OPEN_HOW_SIZE versioning).
> >
> > However, there were a few other desirable features which were not
> > fulfilled by the previous struct layout:
> >
> > * Adding new features (other than new flags) should always result in
> > the struct getting larger. However, by including a padding field, it
> > was possible for new fields to be added without expanding the
> > structure. This would somewhat complicate version-number based
> > checking of feature support.
> >
> > * A non-zero bit in __padding yielded -EINVAL when it should arguably
> > have been -E2BIG (because the padding bits are effectively
> > yet-to-be-used fields). However, the semantics are not entirely clear
> > because userspace may expect -E2BIG to only signify that the
> > structure is too big. It's much simpler to just provide the guarantee
> > that new fields will always result in a struct size increase, and
> > -E2BIG indicates you're using a field that's too recent for an older
> > kernel.
>
> And when the first extension adds another u64 field, that padding has to
> be added back in and checked for being 0, at which point the padding is
> again yet-to-be-used fields.
Maybe I'm missing something, but what is the issue with
struct open_how {
u64 flags;
u64 resolve;
u16 mode;
u64 next_extension;
} __attribute__((packed));
It was my understanding that __aligned_u64 was used to ensure consistent
layouts, not that it was needed for safety against unaligned accesses.
> So what exactly is the problem with returning EINVAL now?
I would argue that -EINVAL was the wrong choice of return code from the
outset (and if we do keep the padding, I will send a patch to switch it
to -E2BIG -- see below). The purpose of -E2BIG for the newer
"extensible" syscalls is to differentiate between using an unsupported
extension field and an unsupported (or invalid) flag.
This will be useful for a few other extension ideas for these types of
syscalls (related to allowing userspace to more efficiently figure out
what flags are supported by the kernel without having to try each one
separately).
> > * The padding wasted space needlessly, and would very likely not be
> > used up entirely by future extensions for a long time (because it
> > couldn't fit a u64).
>
> Who knows, it does fit a u32. And if the struct is to be 8-byte aligned
> (see below), it doesn't actually waste space.
Yeah, though giving it some more thought I think this might be a better
layout to avoid this problem:
struct open_how {
__aligned_u64 flags;
__aligned_u64 resolve;
__u16 mode;
__u16 __padding[3]; /* must be zero */
};
That way, we won't end up with a u16 which we never use (and we won't
have multiple __padding fields in the future).
> > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > index d886bdb585e4..0e070c7f568a 100644
> > --- a/include/uapi/linux/fcntl.h
> > +++ b/include/uapi/linux/fcntl.h
> > @@ -109,17 +109,16 @@
> > * O_TMPFILE} are set.
> > *
> > * @flags: O_* flags.
> > - * @mode: O_CREAT/O_TMPFILE file mode.
> > * @resolve: RESOLVE_* flags.
> > + * @mode: O_CREAT/O_TMPFILE file mode.
> > */
> > struct open_how {
> > - __aligned_u64 flags;
> > + __u64 flags;
> > + __u64 resolve;
> > __u16 mode;
> > - __u16 __padding[3]; /* must be zeroed */
> > - __aligned_u64 resolve;
> > -};
> > +} __attribute__((packed));
>
> IIRC, gcc assumes such a struct has alignment 1, which means that it
> will generate horrible code to access it. So if you do this (and I don't
> think it's a good idea), I think you'd also want to include a
> __attribute__((__aligned__(8))) - or perhaps that can be accomplished by
> just keeping flags as an explicitly aligned member. But that will of
> course bump its sizeof() back to 24, at which point it seems better to
> just make the padding explicit.
Yeah, you're quite right -- I was aware that GCC generated "less than
great" code for aligned(1) structures, but wasn't sure whether it would
be seen as being a serious enough issue to NACK the change.
There is an additional problem -- unfortunately, having the struct be
__attribute__((aligned(8))) doesn't solve the Rust representation
problem because Rust can't represent a struct as both being
#[repr(packed)] and #[repr(align(n))]. Obviously the kernel doesn't
really care about Rust language restrictions, but given one of the main
users of how->resolve will be libpathrs, I'd prefer to not make my own
life any harder if possible. ;)
So, given all of the above, I suggest that instead I send something like
this instead:
diff --git a/fs/open.c b/fs/open.c
index 50a46501bcc9..6c97f52453fe 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -994,7 +994,7 @@ static inline int build_open_flags(const struct open_how *how,
if (how->resolve & ~VALID_RESOLVE_FLAGS)
return -EINVAL;
if (memchr_inv(how->__padding, 0, sizeof(how->__padding)))
- return -EINVAL;
+ return -E2BIG;
/* Deal with the mode. */
if (WILL_CREATE(flags)) {
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index d886bdb585e4..c307640071c8 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -114,9 +114,9 @@
*/
struct open_how {
__aligned_u64 flags;
+ __aligned_u64 resolve;
__u16 mode;
__u16 __padding[3]; /* must be zeroed */
- __aligned_u64 resolve;
};
#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 0b64fedc008b..88e3614cbb3a 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -61,15 +61,15 @@ void test_openat2_struct(void)
{ .name = "normal struct (non-zero padding[0])",
.arg.inner.flags = O_RDONLY,
.arg.inner.__padding = {0xa0, 0x00, 0x00},
- .size = sizeof(struct open_how_ext), .err = -EINVAL },
+ .size = sizeof(struct open_how_ext), .err = -E2BIG },
{ .name = "normal struct (non-zero padding[1])",
.arg.inner.flags = O_RDONLY,
.arg.inner.__padding = {0x00, 0x1a, 0x00},
- .size = sizeof(struct open_how_ext), .err = -EINVAL },
+ .size = sizeof(struct open_how_ext), .err = -E2BIG },
{ .name = "normal struct (non-zero padding[2])",
.arg.inner.flags = O_RDONLY,
.arg.inner.__padding = {0x00, 0x00, 0xef},
- .size = sizeof(struct open_how_ext), .err = -EINVAL },
+ .size = sizeof(struct open_how_ext), .err = -E2BIG },
/* TODO: Once expanded, check zero-padding. */
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply related
* [PATCH v6 0/2] platform/x86: asus_wmi: Support of ASUS TUF laptops on Ryzen CPUs
From: Leonid Maksymchuk @ 2019-12-15 14:25 UTC (permalink / raw)
To: linux-kernel
Cc: platform-driver-x86, linux-api, acpi4asus-user, chiu,
yurii.pavlovskyi, kristian, andy, dvhart, corentin.chary,
Leonid Maksymchuk
Hi,
this patch series adds support of Throttle themal policy ACPI device to
existing asus_wmi platform driver. Support of this device is required
for ASUS TUF laptops on Ryzen CPUs to properly work on Linux.
v2: fixed indentation.
v3: patches 2/3 and 3/3 are refactored.
v4: patch 2/3 are simplified.
v5: add new device instead of merging with fan boost mode.
v6: document new ABI, simplify code.
Leonid Maksymchuk (2):
platform/x86: asus_wmi: Support throttle thermal policy
platform/x86: asus_wmi: Set throttle thermal policy to default
.../ABI/testing/sysfs-platform-asus-wmi | 10 ++
drivers/platform/x86/asus-wmi.c | 124 ++++++++++++++++++
include/linux/platform_data/x86/asus-wmi.h | 1 +
3 files changed, 135 insertions(+)
--
2.24.0
^ permalink raw reply
* [PATCH v6 1/2] platform/x86: asus_wmi: Support throttle thermal policy
From: Leonid Maksymchuk @ 2019-12-15 14:26 UTC (permalink / raw)
To: linux-kernel
Cc: platform-driver-x86, linux-api, acpi4asus-user, chiu,
yurii.pavlovskyi, kristian, andy, dvhart, corentin.chary,
Leonid Maksymchuk
In-Reply-To: <20191215142527.13780-1-leonmaxx@gmail.com>
Throttle thermal policy ACPI device is used to control CPU cooling and
throttling. This patch adds sysfs entry for setting current mode and
Fn+F5 hotkey that switches to next.
Policy modes:
* 0x00 - default
* 0x01 - overboost
* 0x02 - silent
Signed-off-by: Leonid Maksymchuk <leonmaxx@gmail.com>
---
.../ABI/testing/sysfs-platform-asus-wmi | 10 ++
drivers/platform/x86/asus-wmi.c | 113 ++++++++++++++++++
include/linux/platform_data/x86/asus-wmi.h | 1 +
3 files changed, 124 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 9e99f2909612..1efac0ddb417 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -46,3 +46,13 @@ Description:
* 0 - normal,
* 1 - overboost,
* 2 - silent
+
+What: /sys/devices/platform/<platform>/throttle_thermal_policy
+Date: Dec 2019
+KernelVersion: 5.6
+Contact: "Leonid Maksymchuk" <leonmaxx@gmail.com>
+Description:
+ Throttle thermal policy mode:
+ * 0 - default,
+ * 1 - overboost,
+ * 2 - silent
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 821b08e01635..f10ec9d745e5 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -61,6 +61,7 @@ MODULE_LICENSE("GPL");
#define NOTIFY_KBD_BRTDWN 0xc5
#define NOTIFY_KBD_BRTTOGGLE 0xc7
#define NOTIFY_KBD_FBM 0x99
+#define NOTIFY_KBD_TTP 0xae
#define ASUS_WMI_FNLOCK_BIOS_DISABLED BIT(0)
@@ -81,6 +82,10 @@ MODULE_LICENSE("GPL");
#define ASUS_FAN_BOOST_MODE_SILENT_MASK 0x02
#define ASUS_FAN_BOOST_MODES_MASK 0x03
+#define ASUS_THROTTLE_THERMAL_POLICY_DEFAULT 0
+#define ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST 1
+#define ASUS_THROTTLE_THERMAL_POLICY_SILENT 2
+
#define USB_INTEL_XUSB2PR 0xD0
#define PCI_DEVICE_ID_INTEL_LYNXPOINT_LP_XHCI 0x9c31
@@ -198,6 +203,9 @@ struct asus_wmi {
u8 fan_boost_mode_mask;
u8 fan_boost_mode;
+ bool throttle_thermal_policy_available;
+ u8 throttle_thermal_policy_mode;
+
// The RSOC controls the maximum charging percentage.
bool battery_rsoc_available;
@@ -1724,6 +1732,98 @@ static ssize_t fan_boost_mode_store(struct device *dev,
// Fan boost mode: 0 - normal, 1 - overboost, 2 - silent
static DEVICE_ATTR_RW(fan_boost_mode);
+/* Throttle thermal policy ****************************************************/
+
+static int throttle_thermal_policy_check_present(struct asus_wmi *asus)
+{
+ u32 result;
+ int err;
+
+ asus->throttle_thermal_policy_available = false;
+
+ err = asus_wmi_get_devstate(asus,
+ ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
+ &result);
+ if (err) {
+ if (err == -ENODEV)
+ return 0;
+ return err;
+ }
+
+ if (result & ASUS_WMI_DSTS_PRESENCE_BIT)
+ asus->throttle_thermal_policy_available = true;
+
+ return 0;
+}
+
+static int throttle_thermal_policy_write(struct asus_wmi *asus)
+{
+ int err;
+ u8 value;
+ u32 retval;
+
+ value = asus->throttle_thermal_policy_mode;
+
+ err = asus_wmi_set_devstate(ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
+ value, &retval);
+ if (err) {
+ pr_warn("Failed to set throttle thermal policy: %d\n", err);
+ return err;
+ }
+
+ if (retval != 1) {
+ pr_warn("Failed to set throttle thermal policy (retval): 0x%x\n",
+ retval);
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
+{
+ u8 new_mode = asus->throttle_thermal_policy_mode + 1;
+
+ if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
+ new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
+
+ asus->throttle_thermal_policy_mode = new_mode;
+ return throttle_thermal_policy_write(asus);
+}
+
+static ssize_t throttle_thermal_policy_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct asus_wmi *asus = dev_get_drvdata(dev);
+ u8 mode = asus->throttle_thermal_policy_mode;
+
+ return scnprintf(buf, PAGE_SIZE, "%d\n", mode);
+}
+
+static ssize_t throttle_thermal_policy_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int result;
+ u8 new_mode;
+ struct asus_wmi *asus = dev_get_drvdata(dev);
+
+ result = kstrtou8(buf, 10, &new_mode);
+ if (result < 0)
+ return result;
+
+ if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
+ return -EINVAL;
+
+ asus->throttle_thermal_policy_mode = new_mode;
+ throttle_thermal_policy_write(asus);
+
+ return count;
+}
+
+// Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
+static DEVICE_ATTR_RW(throttle_thermal_policy);
+
/* Backlight ******************************************************************/
static int read_backlight_power(struct asus_wmi *asus)
@@ -2005,6 +2105,11 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
return;
}
+ if (asus->throttle_thermal_policy_available && code == NOTIFY_KBD_TTP) {
+ throttle_thermal_policy_switch_next(asus);
+ return;
+ }
+
if (is_display_toggle(code) && asus->driver->quirks->no_display_toggle)
return;
@@ -2155,6 +2260,7 @@ static struct attribute *platform_attributes[] = {
&dev_attr_lid_resume.attr,
&dev_attr_als_enable.attr,
&dev_attr_fan_boost_mode.attr,
+ &dev_attr_throttle_thermal_policy.attr,
NULL
};
@@ -2178,6 +2284,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
devid = ASUS_WMI_DEVID_ALS_ENABLE;
else if (attr == &dev_attr_fan_boost_mode.attr)
ok = asus->fan_boost_mode_available;
+ else if (attr == &dev_attr_throttle_thermal_policy.attr)
+ ok = asus->throttle_thermal_policy_available;
if (devid != -1)
ok = !(asus_wmi_get_devstate_simple(asus, devid) < 0);
@@ -2437,6 +2545,10 @@ static int asus_wmi_add(struct platform_device *pdev)
if (err)
goto fail_fan_boost_mode;
+ err = throttle_thermal_policy_check_present(asus);
+ if (err)
+ goto fail_throttle_thermal_policy;
+
err = asus_wmi_sysfs_init(asus->platform_device);
if (err)
goto fail_sysfs;
@@ -2521,6 +2633,7 @@ static int asus_wmi_add(struct platform_device *pdev)
fail_input:
asus_wmi_sysfs_exit(asus->platform_device);
fail_sysfs:
+fail_throttle_thermal_policy:
fail_fan_boost_mode:
fail_platform:
kfree(asus);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 60249e22e844..d39fc658c320 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -58,6 +58,7 @@
#define ASUS_WMI_DEVID_LIGHT_SENSOR 0x00050022 /* ?? */
#define ASUS_WMI_DEVID_LIGHTBAR 0x00050025
#define ASUS_WMI_DEVID_FAN_BOOST_MODE 0x00110018
+#define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
/* Misc */
#define ASUS_WMI_DEVID_CAMERA 0x00060013
--
2.24.0
^ 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