* Re: [RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields, validate abort_ip < TASK_SIZE
From: Mathieu Desnoyers @ 2018-07-02 22:03 UTC (permalink / raw)
To: Linus Torvalds
Cc: Thomas Gleixner, linux-kernel, linux-api, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon,
Michael Kerrisk <mtk.ma>
In-Reply-To: <CA+55aFwuBef8ajmeE-mw=6YHwYOQdnCMir7h2ebWxOzNrSQQFw@mail.gmail.com>
----- On Jul 2, 2018, at 5:20 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Mon, Jul 2, 2018 at 2:03 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> /* Ensure that abort_ip is not in the critical section. */
>> if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
>> return -EINVAL;
>> ...
>> What underflow issues are you concerned with ?
>
> That.
>
> Looking closer, it looks like what you want to do is
>
> if (rseq_cs->abort_ip >= rseq_cs->start_ip && rseq_cs->abort_ip <
> rseq_cs->start_ip + rseq_cs->post_commit_offset)
>
> but you're not actually verifying that the range you're testing is
> even vlid, because "rseq_cs->start_ip + rseq_cs->post_commit_offset"
> could be something invalid that overflowed (or, put another way, the
> subtraction you did on both sides to get the simplified version
> underflowed).
>
> So to actually get the range check you want, you should check the
> overflow/underflow condition. Maybe it ends up being
>
> if (rseq_cs->start_ip + rseq_cs->post_commit_offset < rseq_cs->start_ip)
> return -EINVAL;
>
> after which your simplified conditional looks fine.
>
> But I think you should also do
>
> if (rseq_cs->start_ip + rseq_cs->post_commit_offset > TASK_SIZE)
> return -EINVAL;
>
> to make sure the range is valid in the first place.
Taking into account your comments, and adding also an extra check for
rseq_cs->start_ip >= TASK_SIZE, and restricting the end of range
rseq_cs->start_ip + rseq_cs->post_commit_offset to exclude TASK_SIZE
(>= rather than >), the resulting function now looks like this:
static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
{
struct rseq_cs __user *urseq_cs;
unsigned long ptr;
u32 __user *usig;
u32 sig;
if (__get_user(ptr, &t->rseq->rseq_cs))
return -EINVAL;
if (check_rseq_cs_padding(t))
return -EINVAL;
if (!ptr) {
memset(rseq_cs, 0, sizeof(*rseq_cs));
return 0;
}
urseq_cs = (struct rseq_cs __user *)ptr;
if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)) ||
rseq_cs->start_ip >= TASK_SIZE ||
rseq_cs->start_ip + rseq_cs->post_commit_offset >= TASK_SIZE ||
rseq_cs->abort_ip >= TASK_SIZE ||
rseq_cs->version > 0)
return -EINVAL;
/* Check for overflow. */
if (rseq_cs->start_ip + rseq_cs->post_commit_offset < rseq_cs->start_ip)
return -EINVAL;
/* Ensure that abort_ip is not in the critical section. */
if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
return -EINVAL;
usig = (u32 __user *)(unsigned long)(rseq_cs->abort_ip - sizeof(u32));
if (get_user(sig, usig))
return -EINVAL;
if (current->rseq_sig != sig) {
printk_ratelimited(KERN_WARNING
"Possible attack attempt. Unexpected rseq signature 0x%x, expecting 0x%x (pid=%d, addr=%p).\n",
sig, current->rseq_sig, current->pid, usig);
return -EINVAL;
}
return 0;
}
The end of range exclusion with (rseq_cs->start_ip + rseq_cs->post_commit_offset >= TASK_SIZE)
stems from the reasoning that we need a valid user-space instruction _after_ the range, so
having the range end exactly at the very last byte of TASK_SIZE would require to have a
user-space instruction at TASK_SIZE, which is not valid.
Does it capture your intent ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields, validate abort_ip < TASK_SIZE
From: Linus Torvalds @ 2018-07-02 22:08 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Thomas Gleixner, Linux Kernel Mailing List, Linux API,
Peter Zijlstra, Paul McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, Paul Turner, Andrew Morton, Russell King - ARM Linux,
Ingo Molnar, Peter Anvin, Andi Kleen, Christoph Lameter,
Ben Maurer, Steven Rostedt, Josh Triplett, Catalin Marinas,
Will Deacon
In-Reply-To: <522686232.10814.1530569002544.JavaMail.zimbra@efficios.com>
On Mon, Jul 2, 2018 at 3:03 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> if (__get_user(ptr, &t->rseq->rseq_cs))
> return -EINVAL;
> if (check_rseq_cs_padding(t))
> return -EINVAL;
Small nit.
I think the _actual_ user access faults should return -EFAULT, and
then the *validation* checks should return -EINVAL.
So when the "copy_from_user()" fails, that's -EFAULT, but when you
have (rseq_cs->start_ip >= TASK_SIZE), that's -EINVAL.
That said, nothing actually cares or exposes the error number, I
think. Afaik, all the callers just check "did it work" or not.
So this is more a "let's be consistent" than anything that matters.
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields, validate abort_ip < TASK_SIZE
From: Mathieu Desnoyers @ 2018-07-02 22:16 UTC (permalink / raw)
To: Linus Torvalds
Cc: Thomas Gleixner, linux-kernel, linux-api, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon,
Michael Kerrisk <mtk.ma>
In-Reply-To: <CA+55aFz5cq5uP7VFwAmo3BimHyMdu_FQnbxZG3i66_qZXCkjNg@mail.gmail.com>
----- On Jul 2, 2018, at 6:08 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Mon, Jul 2, 2018 at 3:03 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> if (__get_user(ptr, &t->rseq->rseq_cs))
>> return -EINVAL;
>> if (check_rseq_cs_padding(t))
>> return -EINVAL;
>
> Small nit.
>
> I think the _actual_ user access faults should return -EFAULT, and
> then the *validation* checks should return -EINVAL.
>
> So when the "copy_from_user()" fails, that's -EFAULT, but when you
> have (rseq_cs->start_ip >= TASK_SIZE), that's -EINVAL.
Fair enough.
>
> That said, nothing actually cares or exposes the error number, I
> think. Afaik, all the callers just check "did it work" or not.
Indeed, it's a static function and callers just check for zero/nonzero.
>
> So this is more a "let's be consistent" than anything that matters.
Allright, here is the function updated accordingly:
static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
{
struct rseq_cs __user *urseq_cs;
unsigned long ptr;
u32 __user *usig;
u32 sig;
int ret;
ret = __get_user(ptr, &t->rseq->rseq_cs);
if (ret)
return ret;
if (check_rseq_cs_padding(t))
return -EINVAL;
if (!ptr) {
memset(rseq_cs, 0, sizeof(*rseq_cs));
return 0;
}
urseq_cs = (struct rseq_cs __user *)ptr;
if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
return -EFAULT;
if (rseq_cs->start_ip >= TASK_SIZE ||
rseq_cs->start_ip + rseq_cs->post_commit_offset >= TASK_SIZE ||
rseq_cs->abort_ip >= TASK_SIZE ||
rseq_cs->version > 0)
return -EINVAL;
/* Check for overflow. */
if (rseq_cs->start_ip + rseq_cs->post_commit_offset < rseq_cs->start_ip)
return -EINVAL;
/* Ensure that abort_ip is not in the critical section. */
if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
return -EINVAL;
usig = (u32 __user *)(unsigned long)(rseq_cs->abort_ip - sizeof(u32));
ret = get_user(sig, usig);
if (ret)
return ret;
if (current->rseq_sig != sig) {
printk_ratelimited(KERN_WARNING
"Possible attack attempt. Unexpected rseq signature 0x%x, expecting 0x%x (pid=%d, addr=%p).\n",
sig, current->rseq_sig, current->pid, usig);
return -EINVAL;
}
return 0;
}
Thanks for the feedback!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-02 22:31 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E . McKenney,
Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
Andrew Morton, Russell King, Ingo Molnar, H . Peter Anvin,
Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
Michael Kerrisk
Change the rseq ABI so rseq_cs start_ip, post_commit_offset and abort_ip
fields are seen as 64-bit fields by both 32-bit and 64-bit kernels rather
that ignoring the 32 upper bits on 32-bit kernels. This ensures we have a
consistent behavior for a 32-bit binary executed on 32-bit kernels and in
compat mode on 64-bit kernels.
Validating the value of abort_ip field to be below TASK_SIZE ensures the
kernel don't return to an invalid address when returning to userspace
after an abort. I don't fully trust each architecture code to consistently
deal with invalid return addresses.
Validating the value of the start_ip and post_commit_offset fields
prevents overflow on arithmetic performed on those values, used to
check whether abort_ip is within the rseq critical section.
On 32-bit kernels, the rseq->rseq_cs_padding field is never read by the
kernel. However, 64-bit kernels dealing with 32-bit compat tasks read the
full 64-bit in its entirety, and terminates the offending process with
a segmentation fault if the upper 32 bits are set due to failure of
copy_from_user().
Ensure that both 32-bit and 64-bit kernels dealing with 32-bit tasks end
up terminating offending tasks with a segmentation fault if the upper
32-bit padding bits (rseq->rseq_cs_padding) are set by explicitly ensuring
that padding is zero on 32-bit kernels.
If validation fails, the process is killed with a segmentation fault.
When the signature encountered before abort_ip does not match the expected
signature, return -EINVAL rather than -EPERM to be consistent with other
input validation return codes from rseq_get_rseq_cs().
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Paul Turner <pjt@google.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Andi Kleen <andi@firstfloor.org>
CC: Dave Watson <davejwatson@fb.com>
CC: Chris Lameter <cl@linux.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ben Maurer <bmaurer@fb.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Russell King <linux@arm.linux.org.uk>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: linux-api@vger.kernel.org
---
include/uapi/linux/rseq.h | 6 +++---
kernel/rseq.c | 39 +++++++++++++++++++++++++++++++++++----
2 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index d620fa43756c..519ad6e176d1 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -52,10 +52,10 @@ struct rseq_cs {
__u32 version;
/* enum rseq_cs_flags */
__u32 flags;
- LINUX_FIELD_u32_u64(start_ip);
+ __u64 start_ip;
/* Offset from start_ip. */
- LINUX_FIELD_u32_u64(post_commit_offset);
- LINUX_FIELD_u32_u64(abort_ip);
+ __u64 post_commit_offset;
+ __u64 abort_ip;
} __attribute__((aligned(4 * sizeof(__u64))));
/*
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 22b6acf1ad63..1d1dd6aa43f8 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -112,6 +112,29 @@ static int rseq_reset_rseq_cpu_id(struct task_struct *t)
return 0;
}
+#ifndef __LP64__
+/*
+ * Check that padding is zero.
+ */
+static int check_rseq_cs_padding(struct task_struct *t)
+{
+ u32 pad;
+ int ret;
+
+ ret = __get_user(pad, &t->rseq->rseq_cs_padding);
+ if (ret)
+ return ret;
+ if (pad)
+ return -EINVAL;
+ return 0;
+}
+#else
+static int check_rseq_cs_padding(struct task_struct *t)
+{
+ return 0;
+}
+#endif
+
static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
{
struct rseq_cs __user *urseq_cs;
@@ -123,6 +146,8 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
ret = __get_user(ptr, &t->rseq->rseq_cs);
if (ret)
return ret;
+ if (check_rseq_cs_padding(t))
+ return -EINVAL;
if (!ptr) {
memset(rseq_cs, 0, sizeof(*rseq_cs));
return 0;
@@ -130,14 +155,20 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
urseq_cs = (struct rseq_cs __user *)ptr;
if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
return -EFAULT;
- if (rseq_cs->version > 0)
- return -EINVAL;
+ if (rseq_cs->start_ip >= TASK_SIZE ||
+ rseq_cs->start_ip + rseq_cs->post_commit_offset >= TASK_SIZE ||
+ rseq_cs->abort_ip >= TASK_SIZE ||
+ rseq_cs->version > 0)
+ return -EINVAL;
+ /* Check for overflow. */
+ if (rseq_cs->start_ip + rseq_cs->post_commit_offset < rseq_cs->start_ip)
+ return -EINVAL;
/* Ensure that abort_ip is not in the critical section. */
if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
return -EINVAL;
- usig = (u32 __user *)(rseq_cs->abort_ip - sizeof(u32));
+ usig = (u32 __user *)(unsigned long)(rseq_cs->abort_ip - sizeof(u32));
ret = get_user(sig, usig);
if (ret)
return ret;
@@ -146,7 +177,7 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
printk_ratelimited(KERN_WARNING
"Possible attack attempt. Unexpected rseq signature 0x%x, expecting 0x%x (pid=%d, addr=%p).\n",
sig, current->rseq_sig, current->pid, usig);
- return -EPERM;
+ return -EINVAL;
}
return 0;
}
--
2.11.0
^ permalink raw reply related
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Linus Torvalds @ 2018-07-02 22:45 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Thomas Gleixner, Linux Kernel Mailing List, Linux API,
Peter Zijlstra, Paul McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, Paul Turner, Andrew Morton, Russell King - ARM Linux,
Ingo Molnar, Peter Anvin, Andi Kleen, Christoph Lameter,
Ben Maurer, Steven Rostedt, Josh Triplett, Catalin Marinas,
Will Deacon
In-Reply-To: <20180702223143.4663-1-mathieu.desnoyers@efficios.com>
On Mon, Jul 2, 2018 at 3:31 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> Change the rseq ABI so rseq_cs start_ip, post_commit_offset and abort_ip
> fields are seen as 64-bit fields by both 32-bit and 64-bit kernels rather
> that ignoring the 32 upper bits on 32-bit kernels. This ensures we have a
> consistent behavior for a 32-bit binary executed on 32-bit kernels and in
> compat mode on 64-bit kernels.
Actually, now that I see this again, I react to:
> +static int check_rseq_cs_padding(struct task_struct *t)
> +{
> + u32 pad;
> + int ret;
> +
> + ret = __get_user(pad, &t->rseq->rseq_cs_padding);
> + if (ret)
> + return ret;
> + if (pad)
> + return -EINVAL;
> + return 0;
> +}
This is all wrong.
Just make "rseq_cs" be an __u64" too. That will clean up everything,
and user space will have a much easier time filling it in too, since
it's just one field. Instead of having to remember about the "let's
fill in padding for 32-bit cases".
Then the rseq_get_rseq_cs() will be
__u64 rseq_cs;
ret = get_user(rseq_cs, &t->rseq->rseq_cs);
if (ret)
return ret;
ptr = (void *)rseq_cs;
if (rseq_cs != (unsigned long)ptr)
return -EINVAL;
and it's all good, no #ifdef's etc needed.
Hmm?
Sorry for the bike-shedding, but this is now the last remaining user
of that LINUX_FIELD_u32_u64, so let's just get rid of it entirely, ok?
Then we can also get rid of that silly uapi/linux/types_32_64.h header
file entirely.
That would be *lovely*. Simpler code, simpler and less error-prone
interfaces, and one less specialized header file.
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-02 23:00 UTC (permalink / raw)
To: Linus Torvalds
Cc: Thomas Gleixner, linux-kernel, linux-api, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon,
Michael Kerrisk <mtk.ma>
In-Reply-To: <CA+55aFxrX+wRoe+5HDaTrRXg1K-Qpks4jJ9Buc_OevoG8hoGFw@mail.gmail.com>
----- On Jul 2, 2018, at 6:45 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Mon, Jul 2, 2018 at 3:31 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> Change the rseq ABI so rseq_cs start_ip, post_commit_offset and abort_ip
>> fields are seen as 64-bit fields by both 32-bit and 64-bit kernels rather
>> that ignoring the 32 upper bits on 32-bit kernels. This ensures we have a
>> consistent behavior for a 32-bit binary executed on 32-bit kernels and in
>> compat mode on 64-bit kernels.
>
> Actually, now that I see this again, I react to:
>
>
>> +static int check_rseq_cs_padding(struct task_struct *t)
>> +{
>> + u32 pad;
>> + int ret;
>> +
>> + ret = __get_user(pad, &t->rseq->rseq_cs_padding);
>> + if (ret)
>> + return ret;
>> + if (pad)
>> + return -EINVAL;
>> + return 0;
>> +}
>
> This is all wrong.
>
> Just make "rseq_cs" be an __u64" too. That will clean up everything,
> and user space will have a much easier time filling it in too, since
> it's just one field. Instead of having to remember about the "let's
> fill in padding for 32-bit cases".
>
> Then the rseq_get_rseq_cs() will be
>
> __u64 rseq_cs;
>
> ret = get_user(rseq_cs, &t->rseq->rseq_cs);
> if (ret)
> return ret;
> ptr = (void *)rseq_cs;
> if (rseq_cs != (unsigned long)ptr)
> return -EINVAL;
>
> and it's all good, no #ifdef's etc needed.
>
> Hmm?
Unfortunately, that rseq->rseq_cs field needs to be updated by user-space
with single-copy atomicity. Therefore, we want 32-bit user-space to initialize
the padding with 0, and only update the low bits with single-copy atomicity.
>
> Sorry for the bike-shedding, but this is now the last remaining user
> of that LINUX_FIELD_u32_u64, so let's just get rid of it entirely, ok?
>
> Then we can also get rid of that silly uapi/linux/types_32_64.h header
> file entirely.
>
> That would be *lovely*. Simpler code, simpler and less error-prone
> interfaces, and one less specialized header file.
We can easily switch from LINUX_FIELD_u32_u64 to __u64 for fields within
struct rseq_cs because we have no requirement on update single-copy
atomicity. However, this is not true for the rseq->rseq_cs pointer.
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Linus Torvalds @ 2018-07-02 23:06 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Thomas Gleixner, Linux Kernel Mailing List, Linux API,
Peter Zijlstra, Paul McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, Paul Turner, Andrew Morton, Russell King - ARM Linux,
Ingo Molnar, Peter Anvin, Andi Kleen, Christoph Lameter,
Ben Maurer, Steven Rostedt, Josh Triplett, Catalin Marinas,
Will Deacon
In-Reply-To: <415287289.10831.1530572418907.JavaMail.zimbra@efficios.com>
On Mon, Jul 2, 2018 at 4:00 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> Unfortunately, that rseq->rseq_cs field needs to be updated by user-space
> with single-copy atomicity. Therefore, we want 32-bit user-space to initialize
> the padding with 0, and only update the low bits with single-copy atomicity.
Well... It's actually still single-copy atomicity as a 64-bit value.
Why? Because it doesn't matter how you write the upper bits. You'll be
writing the same value to them (zero) anyway.
So who cares if the write ends up being two instructions, because the
write to the upper bits doesn't actually *do* anything.
Hmm?
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-02 23:16 UTC (permalink / raw)
To: Linus Torvalds
Cc: Thomas Gleixner, linux-kernel, linux-api, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon,
Michael Kerrisk <mtk.ma>
In-Reply-To: <CA+55aFzZYOatOtjrm7fD7878_SzCAmEoEu74n0+pgd7faH-sTQ@mail.gmail.com>
----- On Jul 2, 2018, at 7:06 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Mon, Jul 2, 2018 at 4:00 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> Unfortunately, that rseq->rseq_cs field needs to be updated by user-space
>> with single-copy atomicity. Therefore, we want 32-bit user-space to initialize
>> the padding with 0, and only update the low bits with single-copy atomicity.
>
> Well... It's actually still single-copy atomicity as a 64-bit value.
>
> Why? Because it doesn't matter how you write the upper bits. You'll be
> writing the same value to them (zero) anyway.
>
> So who cares if the write ends up being two instructions, because the
> write to the upper bits doesn't actually *do* anything.
>
> Hmm?
Are there any kind of guarantees that a __u64 update on a 32-bit architecture
won't be torn into something daft like byte-per-byte stores when performed
from C code ?
I don't worry whether the upper bits get updated or how, but I really care
about not having store tearing of the low bits update.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Linus Torvalds @ 2018-07-02 23:22 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Thomas Gleixner, Linux Kernel Mailing List, Linux API,
Peter Zijlstra, Paul McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, Paul Turner, Andrew Morton, Russell King - ARM Linux,
Ingo Molnar, Peter Anvin, Andi Kleen, Christoph Lameter,
Ben Maurer, Steven Rostedt, Josh Triplett, Catalin Marinas,
Will Deacon
In-Reply-To: <825871008.10839.1530573419561.JavaMail.zimbra@efficios.com>
On Mon, Jul 2, 2018 at 4:17 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> Are there any kind of guarantees that a __u64 update on a 32-bit architecture
> won't be torn into something daft like byte-per-byte stores when performed
> from C code ?
Guarantees? No. Not that there are any guarantees that the same won't
happen for a plain 32-bit value either.
Will compilers generate that kind of code? I guess some crazy compiler
could simply be really bad at handling 64-bit values, and just happen
to handle 32-bit values better. So in that sense a 64-bit entity is
certainly a bit riskier. But that would be a really bad compiler, I
have to say.
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-02 23:22 UTC (permalink / raw)
To: Linus Torvalds
Cc: Thomas Gleixner, linux-kernel, linux-api, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon,
Michael Kerrisk <mtk.ma>
In-Reply-To: <825871008.10839.1530573419561.JavaMail.zimbra@efficios.com>
----- On Jul 2, 2018, at 7:16 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
> ----- On Jul 2, 2018, at 7:06 PM, Linus Torvalds torvalds@linux-foundation.org
> wrote:
>
>> On Mon, Jul 2, 2018 at 4:00 PM Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>>>
>>> Unfortunately, that rseq->rseq_cs field needs to be updated by user-space
>>> with single-copy atomicity. Therefore, we want 32-bit user-space to initialize
>>> the padding with 0, and only update the low bits with single-copy atomicity.
>>
>> Well... It's actually still single-copy atomicity as a 64-bit value.
>>
>> Why? Because it doesn't matter how you write the upper bits. You'll be
>> writing the same value to them (zero) anyway.
>>
>> So who cares if the write ends up being two instructions, because the
>> write to the upper bits doesn't actually *do* anything.
>>
>> Hmm?
>
> Are there any kind of guarantees that a __u64 update on a 32-bit architecture
> won't be torn into something daft like byte-per-byte stores when performed
> from C code ?
>
> I don't worry whether the upper bits get updated or how, but I really care
> about not having store tearing of the low bits update.
For the records, most updates of those low bits are done in assembly
from critical sections, for which we control exactly how the update is
performed.
However, there is one helper function in user-space that updates that value
from C through a volatile store, e.g.:
static inline void rseq_prepare_unload(void)
{
__rseq_abi.rseq_cs = 0;
}
Thanks,
Mathieu
>
> Thanks,
>
> Mathieu
>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-02 23:25 UTC (permalink / raw)
To: Linus Torvalds
Cc: Thomas Gleixner, linux-kernel, linux-api, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon,
Michael Kerrisk <mtk.ma>
In-Reply-To: <CA+55aFyQwfXxMnx2WQadfZKAnEfkoT+zomrRRToZz8-uWzUccQ@mail.gmail.com>
----- On Jul 2, 2018, at 7:22 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Mon, Jul 2, 2018 at 4:17 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> Are there any kind of guarantees that a __u64 update on a 32-bit architecture
>> won't be torn into something daft like byte-per-byte stores when performed
>> from C code ?
>
> Guarantees? No. Not that there are any guarantees that the same won't
> happen for a plain 32-bit value either.
>
> Will compilers generate that kind of code? I guess some crazy compiler
> could simply be really bad at handling 64-bit values, and just happen
> to handle 32-bit values better. So in that sense a 64-bit entity is
> certainly a bit riskier. But that would be a really bad compiler, I
> have to say.
Given that the only C code updating that field is rseq_prepare_unload()
(the rest is only ever updated from assembly), we could perhaps mandate
that user-space always update it from assembly, and therefore implement
rseq_prepare_unload as an inline asm which clears rseq->rseq_cs.
Does it sound better than the LINUX_FIELD_u32_u64 macro ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Andy Lutomirski @ 2018-07-02 23:37 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Linus Torvalds, Thomas Gleixner, linux-kernel, linux-api,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon, Michael
In-Reply-To: <1959930320.10843.1530573742647.JavaMail.zimbra@efficios.com>
> On Jul 2, 2018, at 4:22 PM, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
> ----- On Jul 2, 2018, at 7:16 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
>
>> ----- On Jul 2, 2018, at 7:06 PM, Linus Torvalds torvalds@linux-foundation.org
>> wrote:
>>
>>> On Mon, Jul 2, 2018 at 4:00 PM Mathieu Desnoyers
>>> <mathieu.desnoyers@efficios.com> wrote:
>>>>
>>>> Unfortunately, that rseq->rseq_cs field needs to be updated by user-space
>>>> with single-copy atomicity. Therefore, we want 32-bit user-space to initialize
>>>> the padding with 0, and only update the low bits with single-copy atomicity.
>>>
>>> Well... It's actually still single-copy atomicity as a 64-bit value.
>>>
>>> Why? Because it doesn't matter how you write the upper bits. You'll be
>>> writing the same value to them (zero) anyway.
>>>
>>> So who cares if the write ends up being two instructions, because the
>>> write to the upper bits doesn't actually *do* anything.
>>>
>>> Hmm?
>>
>> Are there any kind of guarantees that a __u64 update on a 32-bit architecture
>> won't be torn into something daft like byte-per-byte stores when performed
>> from C code ?
>>
>> I don't worry whether the upper bits get updated or how, but I really care
>> about not having store tearing of the low bits update.
>
> For the records, most updates of those low bits are done in assembly
> from critical sections, for which we control exactly how the update is
> performed.
>
> However, there is one helper function in user-space that updates that value
> from C through a volatile store, e.g.:
>
> static inline void rseq_prepare_unload(void)
> {
> __rseq_abi.rseq_cs = 0;
> }
How about making the field be:
union {
__u64 rseq_cs;
struct {
__u32 rseq_cs_low;
__u32 rseq_cs_high;
};
};
32-bit user code that cares about performance can just write to rseq_cs_low because it already knows that rseq_cs_high == 0.
The header could even supply a static inline helper write_rseq_cs() that atomically writes a pointer and just does the right thing for 64-bit, for 32-bit BE, and for 32-bit LE.
I think the union really is needed because we can’t rely on user code being built with -fno-strict-aliasing. Or the helper could use inline asm.
Anyway, the point is that we get optimal code generation (a single instruction write of the correct number of bits) without any compat magic in the kernel.
>
> Thanks,
>
> Mathieu
>
>>
>> Thanks,
>>
>> Mathieu
>>
>>
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> http://www.efficios.com
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Christopher Lameter @ 2018-07-03 0:19 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Linus Torvalds, Thomas Gleixner, linux-kernel, linux-api,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon, Micha
In-Reply-To: <825871008.10839.1530573419561.JavaMail.zimbra@efficios.com>
On Mon, 2 Jul 2018, Mathieu Desnoyers wrote:
> Are there any kind of guarantees that a __u64 update on a 32-bit architecture
> won't be torn into something daft like byte-per-byte stores when performed
> from C code ?
>
> I don't worry whether the upper bits get updated or how, but I really care
> about not having store tearing of the low bits update.
Platforms with 32 bit word size only guarantee atomicity of a 32 bit
write or RMV instruction.
Special instructions may exist on a platform to perform 64 bit atomic
updates. We use cmpxchg64 f.e. on Intel 32 bit platforms to guarantee
atomicity8.
So use the macros that we have to guarantee 64 bit ops and you should be
fine. See linux/arch/x86/include/asm/atomic64_32.h
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-03 0:23 UTC (permalink / raw)
To: Chris Lameter
Cc: Linus Torvalds, Thomas Gleixner, linux-kernel, linux-api,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon, Micha
In-Reply-To: <010001645d81f652-8a506dd2-cd49-47f9-950a-24ef52bda9f7-000000@email.amazonses.com>
----- On Jul 2, 2018, at 8:19 PM, Chris Lameter cl@linux.com wrote:
> On Mon, 2 Jul 2018, Mathieu Desnoyers wrote:
>
>> Are there any kind of guarantees that a __u64 update on a 32-bit architecture
>> won't be torn into something daft like byte-per-byte stores when performed
>> from C code ?
>>
>> I don't worry whether the upper bits get updated or how, but I really care
>> about not having store tearing of the low bits update.
>
> Platforms with 32 bit word size only guarantee atomicity of a 32 bit
> write or RMV instruction.
>
> Special instructions may exist on a platform to perform 64 bit atomic
> updates. We use cmpxchg64 f.e. on Intel 32 bit platforms to guarantee
> atomicity8.
>
> So use the macros that we have to guarantee 64 bit ops and you should be
> fine. See linux/arch/x86/include/asm/atomic64_32.h
We are talking about user-space here. What we need is a single instruction
atomic store, similar to what WRITE_ONCE() does in the kernel. The discussion
is about whether doing the user-space equivalent of a WRITE_ONCE() to a u64
on a 32-bit architecture should be considered to provide single-copy atomicity
on the low 32 bits.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Christopher Lameter @ 2018-07-03 0:35 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Linus Torvalds, Thomas Gleixner, linux-kernel, linux-api,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon, Micha
In-Reply-To: <1020760632.10855.1530577391822.JavaMail.zimbra@efficios.com>
On Mon, 2 Jul 2018, Mathieu Desnoyers wrote:
> >
> > Platforms with 32 bit word size only guarantee atomicity of a 32 bit
> > write or RMV instruction.
> >
> > Special instructions may exist on a platform to perform 64 bit atomic
> > updates. We use cmpxchg64 f.e. on Intel 32 bit platforms to guarantee
> > atomicity8.
> >
> > So use the macros that we have to guarantee 64 bit ops and you should be
> > fine. See linux/arch/x86/include/asm/atomic64_32.h
>
> We are talking about user-space here. What we need is a single instruction
> atomic store, similar to what WRITE_ONCE() does in the kernel. The discussion
> is about whether doing the user-space equivalent of a WRITE_ONCE() to a u64
> on a 32-bit architecture should be considered to provide single-copy atomicity
> on the low 32 bits.
Right. You would need to make this work for userspace. atomic64_32.h is a
good reference as to which instructions provide 64 bit atomicity on 32
bit platforms.
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-03 1:17 UTC (permalink / raw)
To: Chris Lameter
Cc: Linus Torvalds, Thomas Gleixner, linux-kernel, linux-api,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon, Micha
In-Reply-To: <010001645d90c295-71d0a2e6-d5ce-4b8a-a60d-5d66fe17c8b3-000000@email.amazonses.com>
----- On Jul 2, 2018, at 8:35 PM, Chris Lameter cl@linux.com wrote:
> On Mon, 2 Jul 2018, Mathieu Desnoyers wrote:
>
>> >
>> > Platforms with 32 bit word size only guarantee atomicity of a 32 bit
>> > write or RMV instruction.
>> >
>> > Special instructions may exist on a platform to perform 64 bit atomic
>> > updates. We use cmpxchg64 f.e. on Intel 32 bit platforms to guarantee
>> > atomicity8.
>> >
>> > So use the macros that we have to guarantee 64 bit ops and you should be
>> > fine. See linux/arch/x86/include/asm/atomic64_32.h
>>
>> We are talking about user-space here. What we need is a single instruction
>> atomic store, similar to what WRITE_ONCE() does in the kernel. The discussion
>> is about whether doing the user-space equivalent of a WRITE_ONCE() to a u64
>> on a 32-bit architecture should be considered to provide single-copy atomicity
>> on the low 32 bits.
>
> Right. You would need to make this work for userspace. atomic64_32.h is a
> good reference as to which instructions provide 64 bit atomicity on 32
> bit platforms.
We only need to update a pointer, so we don't need 64-bit atomicity on
32-bit processes.
What we need is to ensure single-copy atomicity of the 32-bit pointer update
on the 32-bit process in a field read from the kernel as a __u64.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-03 1:19 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Thomas Gleixner, linux-kernel, linux-api,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon, Michael
In-Reply-To: <8B2E4CEB-3080-4602-8B62-774E400892EB@amacapital.net>
----- On Jul 2, 2018, at 7:37 PM, Andy Lutomirski luto@amacapital.net wrote:
>> On Jul 2, 2018, at 4:22 PM, Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> wrote:
>>
>> ----- On Jul 2, 2018, at 7:16 PM, Mathieu Desnoyers
>> mathieu.desnoyers@efficios.com wrote:
>>
>>> ----- On Jul 2, 2018, at 7:06 PM, Linus Torvalds torvalds@linux-foundation.org
>>> wrote:
>>>
>>>> On Mon, Jul 2, 2018 at 4:00 PM Mathieu Desnoyers
>>>> <mathieu.desnoyers@efficios.com> wrote:
>>>>>
>>>>> Unfortunately, that rseq->rseq_cs field needs to be updated by user-space
>>>>> with single-copy atomicity. Therefore, we want 32-bit user-space to initialize
>>>>> the padding with 0, and only update the low bits with single-copy atomicity.
>>>>
>>>> Well... It's actually still single-copy atomicity as a 64-bit value.
>>>>
>>>> Why? Because it doesn't matter how you write the upper bits. You'll be
>>>> writing the same value to them (zero) anyway.
>>>>
>>>> So who cares if the write ends up being two instructions, because the
>>>> write to the upper bits doesn't actually *do* anything.
>>>>
>>>> Hmm?
>>>
>>> Are there any kind of guarantees that a __u64 update on a 32-bit architecture
>>> won't be torn into something daft like byte-per-byte stores when performed
>>> from C code ?
>>>
>>> I don't worry whether the upper bits get updated or how, but I really care
>>> about not having store tearing of the low bits update.
>>
>> For the records, most updates of those low bits are done in assembly
>> from critical sections, for which we control exactly how the update is
>> performed.
>>
>> However, there is one helper function in user-space that updates that value
>> from C through a volatile store, e.g.:
>>
>> static inline void rseq_prepare_unload(void)
>> {
>> __rseq_abi.rseq_cs = 0;
>> }
>
> How about making the field be:
>
> union {
> __u64 rseq_cs;
> struct {
> __u32 rseq_cs_low;
> __u32 rseq_cs_high;
> };
> };
>
> 32-bit user code that cares about performance can just write to rseq_cs_low
> because it already knows that rseq_cs_high == 0.
>
> The header could even supply a static inline helper write_rseq_cs() that
> atomically writes a pointer and just does the right thing for 64-bit, for
> 32-bit BE, and for 32-bit LE.
>
> I think the union really is needed because we can’t rely on user code being
> built with -fno-strict-aliasing. Or the helper could use inline asm.
>
> Anyway, the point is that we get optimal code generation (a single instruction
> write of the correct number of bits) without any compat magic in the kernel.
That works for me! Any objection from anyone else for this approach ?
Thanks,
Mathieu
>
>>
>> Thanks,
>>
>> Mathieu
>>
>>>
>>> Thanks,
>>>
>>> Mathieu
>>>
>>>
>>> --
>>> Mathieu Desnoyers
>>> EfficiOS Inc.
>>> http://www.efficios.com
>>
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
> > http://www.efficios.com
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-03 2:01 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Thomas Gleixner, linux-kernel, linux-api,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon, Michael
In-Reply-To: <459661281.10865.1530580742205.JavaMail.zimbra@efficios.com>
----- On Jul 2, 2018, at 9:19 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
> ----- On Jul 2, 2018, at 7:37 PM, Andy Lutomirski luto@amacapital.net wrote:
>
>>> On Jul 2, 2018, at 4:22 PM, Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>>> wrote:
>>>
>>> ----- On Jul 2, 2018, at 7:16 PM, Mathieu Desnoyers
>>> mathieu.desnoyers@efficios.com wrote:
>>>
>>>> ----- On Jul 2, 2018, at 7:06 PM, Linus Torvalds torvalds@linux-foundation.org
>>>> wrote:
>>>>
>>>>> On Mon, Jul 2, 2018 at 4:00 PM Mathieu Desnoyers
>>>>> <mathieu.desnoyers@efficios.com> wrote:
>>>>>>
>>>>>> Unfortunately, that rseq->rseq_cs field needs to be updated by user-space
>>>>>> with single-copy atomicity. Therefore, we want 32-bit user-space to initialize
>>>>>> the padding with 0, and only update the low bits with single-copy atomicity.
>>>>>
>>>>> Well... It's actually still single-copy atomicity as a 64-bit value.
>>>>>
>>>>> Why? Because it doesn't matter how you write the upper bits. You'll be
>>>>> writing the same value to them (zero) anyway.
>>>>>
>>>>> So who cares if the write ends up being two instructions, because the
>>>>> write to the upper bits doesn't actually *do* anything.
>>>>>
>>>>> Hmm?
>>>>
>>>> Are there any kind of guarantees that a __u64 update on a 32-bit architecture
>>>> won't be torn into something daft like byte-per-byte stores when performed
>>>> from C code ?
>>>>
>>>> I don't worry whether the upper bits get updated or how, but I really care
>>>> about not having store tearing of the low bits update.
>>>
>>> For the records, most updates of those low bits are done in assembly
>>> from critical sections, for which we control exactly how the update is
>>> performed.
>>>
>>> However, there is one helper function in user-space that updates that value
>>> from C through a volatile store, e.g.:
>>>
>>> static inline void rseq_prepare_unload(void)
>>> {
>>> __rseq_abi.rseq_cs = 0;
>>> }
>>
>> How about making the field be:
>>
>> union {
>> __u64 rseq_cs;
>> struct {
>> __u32 rseq_cs_low;
>> __u32 rseq_cs_high;
>> };
>> };
>>
>> 32-bit user code that cares about performance can just write to rseq_cs_low
>> because it already knows that rseq_cs_high == 0.
>>
>> The header could even supply a static inline helper write_rseq_cs() that
>> atomically writes a pointer and just does the right thing for 64-bit, for
>> 32-bit BE, and for 32-bit LE.
>>
>> I think the union really is needed because we can’t rely on user code being
>> built with -fno-strict-aliasing. Or the helper could use inline asm.
>>
>> Anyway, the point is that we get optimal code generation (a single instruction
>> write of the correct number of bits) without any compat magic in the kernel.
>
> That works for me! Any objection from anyone else for this approach ?
One thing to consider is how we will implement the load of that pointer
on the kernel side. Strictly-speaking, the rseq uapi talks about single-copy
atomicity, and does not specify _which_ thread is expected to update that
pointer. So arguably, the common case is that the current thread is updating
it, which would allow the kernel to read it piece-wise. However, nothing
prevents user-space from updating it from another thread with single-copy
atomicity.
So in order to be on the safe side, I prefer to guarantee single-copy
atomicity of the get_user() load from the kernel that reads this pointer.
This means a 32-bit kernel would have to perform two independent loads:
one for low bits, one for high bits.
So it does look like we need some __LP64__ ifdefery even with the union
trick. Therefore, I'm not convinced the union is useful at all.
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Linus Torvalds @ 2018-07-03 2:18 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andy Lutomirski, Thomas Gleixner, Linux Kernel Mailing List,
Linux API, Peter Zijlstra, Paul McKenney, Boqun Feng, Dave Watson,
Paul Turner, Andrew Morton, Russell King - ARM Linux, Ingo Molnar,
Peter Anvin, Andi Kleen, Christoph Lameter, Ben Maurer,
Steven Rostedt, Josh Triplett, Catalin Marinas, Will Deacon
In-Reply-To: <858886246.10882.1530583291379.JavaMail.zimbra@efficios.com>
On Mon, Jul 2, 2018 at 7:01 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> One thing to consider is how we will implement the load of that pointer
> on the kernel side.
Use "get_user()". It works for 64-bit objects too, and it will be
atomic in the 32-bit sub-parts on a 32-bit architecture.
Again: there is no point in trying to be atomic in the full 64 bits
(when you're running on 32-bit). The upper bits don't have to "match"
the lower bits. They just have to be zero. So doing it as two loads is
fine - the same way it's perfectly fine to do it as two stores (since
the store to the upper bits will always be zero).
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-03 2:30 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Thomas Gleixner, linux-kernel, linux-api,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon,
Michael Kerrisk <mtk.ma>
In-Reply-To: <CA+55aFxWMjwx-4TGBHt0H6bh6FXC3NKMsjp=eKGw5cgA4wMBUA@mail.gmail.com>
----- On Jul 2, 2018, at 10:18 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Mon, Jul 2, 2018 at 7:01 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> One thing to consider is how we will implement the load of that pointer
>> on the kernel side.
>
> Use "get_user()". It works for 64-bit objects too, and it will be
> atomic in the 32-bit sub-parts on a 32-bit architecture.
Is it really ? Last time we had this discussion, not all architectures
guaranteed that reading a 64-bit integer would happen in two atomic
32-bit sub-parts. This was the main motivation for the LINUX_FIELD_u32_u64()
macro as it stands today (rather than using a union).
>
> Again: there is no point in trying to be atomic in the full 64 bits
> (when you're running on 32-bit). The upper bits don't have to "match"
> the lower bits. They just have to be zero. So doing it as two loads is
> fine - the same way it's perfectly fine to do it as two stores (since
> the store to the upper bits will always be zero).
I'd be fine with two atomic loads, but I'd rather have a strong
confirmation about this, because last time around there were
architectures where it was not true as far as I recall.
Thanks,
Mathieu
>
> Linus
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Andy Lutomirski @ 2018-07-03 2:33 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Linus Torvalds, Thomas Gleixner, linux-kernel, linux-api,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon, Michael
In-Reply-To: <1776351430.10902.1530585009519.JavaMail.zimbra@efficios.com>
On Mon, Jul 2, 2018 at 7:30 PM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> ----- On Jul 2, 2018, at 10:18 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
>
>> On Mon, Jul 2, 2018 at 7:01 PM Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>>>
>>> One thing to consider is how we will implement the load of that pointer
>>> on the kernel side.
>>
>> Use "get_user()". It works for 64-bit objects too, and it will be
>> atomic in the 32-bit sub-parts on a 32-bit architecture.
>
> Is it really ? Last time we had this discussion, not all architectures
> guaranteed that reading a 64-bit integer would happen in two atomic
> 32-bit sub-parts. This was the main motivation for the LINUX_FIELD_u32_u64()
> macro as it stands today (rather than using a union).
>
If you're nervous, you could do this by open-coding:
#if BITS_PER_LONG == 64
get_user(...)
#else
get_user(...);
get_user(...);
#endif
No need to make the header more complicated just for this.
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Linus Torvalds @ 2018-07-03 2:44 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andy Lutomirski, Thomas Gleixner, Linux Kernel Mailing List,
Linux API, Peter Zijlstra, Paul McKenney, Boqun Feng, Dave Watson,
Paul Turner, Andrew Morton, Russell King - ARM Linux, Ingo Molnar,
Peter Anvin, Andi Kleen, Christoph Lameter, Ben Maurer,
Steven Rostedt, Josh Triplett, Catalin Marinas, Will Deacon
In-Reply-To: <1776351430.10902.1530585009519.JavaMail.zimbra@efficios.com>
On Mon, Jul 2, 2018 at 7:30 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
>
> Is it really ? Last time we had this discussion, not all architectures
> guaranteed that reading a 64-bit integer would happen in two atomic
> 32-bit sub-parts.
All architectures that matter do.
Please don't overdesign this, or try to make a problem out of
something that isn't a problem.
Sure, maybe some toy architecture does a 8-byte "get_user()" as a
"copy_from_user()" one byte at a time, because that's the best way to
do unaligned accesses.
But nobody will ever care about rseq on such a thing anyway. Let it go.
Linus
^ permalink raw reply
* [PATCH 0/6] Introduce struct __kernel_timex
From: Deepa Dinamani @ 2018-07-03 5:44 UTC (permalink / raw)
To: tglx, linux-kernel
Cc: linux-arch, arnd, y2038, catalin.marinas, linux-alpha, netdev,
linux-api, davem
The series introduces struct __kernel_timex as a substitute for
the non y2038 safe struct timex.
The series is based on the original series posted by Arnd Bergmann
in [1].
The overview of the series is as below:
1. Prepare for the compat timex interfaces to be used unconditionally.
2. Introduce struct __kernel_timex.
3. Use struct __kernel_timex in place of struct timex.
4. Switch syscalls to use struct __kernel_timex.
Deepa Dinamani (6):
arm64: Make basic compat_* types always available
sparc: Make thread_info.h available directly
timex: prepare compat helpers for y2038 changes
time: Add struct __kernel_timex
timex: use __kernel_timex internally
timex: change syscalls to use struct __kernel_timex
arch/alpha/kernel/osf_sys.c | 2 +-
arch/arm64/include/asm/compat.h | 22 ++++-----
arch/sparc/include/asm/compat.h | 2 +
drivers/ptp/ptp_clock.c | 2 +-
include/asm-generic/compat.h | 8 +++-
include/linux/compat.h | 33 --------------
include/linux/compat_time.h | 34 ++++++++++++++
include/linux/posix-clock.h | 2 +-
include/linux/syscalls.h | 5 +--
include/linux/timex.h | 9 +++-
include/uapi/linux/timex.h | 41 +++++++++++++++++
kernel/compat.c | 63 --------------------------
kernel/time/ntp.c | 12 ++---
kernel/time/ntp_internal.h | 2 +-
kernel/time/posix-clock.c | 2 +-
kernel/time/posix-timers.c | 14 ++----
kernel/time/posix-timers.h | 2 +-
kernel/time/time.c | 80 ++++++++++++++++++++++++++++++---
kernel/time/timekeeping.c | 4 +-
19 files changed, 198 insertions(+), 141 deletions(-)
base-commit: 69877f06915f1c7a9f1704442993bcc12c13ace2
--
2.17.1
Cc: catalin.marinas@arm.com
Cc: davem@davemloft.net
Cc: linux-alpha@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: netdev@vger.kernel.org
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
^ permalink raw reply
* [PATCH 6/6] timex: change syscalls to use struct __kernel_timex
From: Deepa Dinamani @ 2018-07-03 5:44 UTC (permalink / raw)
To: tglx, linux-kernel; +Cc: arnd, y2038, linux-api
In-Reply-To: <20180703054422.12089-1-deepa.kernel@gmail.com>
struct timex is not y2038 safe.
Switch all the syscall apis to use y2038 safe __kernel_timex.
Note that sys_adjtimex() does not have a y2038 safe solution.
The api is meant to be deprecated on 32 bit machines after y2038.
Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
Cc: linux-api@vger.kernel.org
---
include/linux/syscalls.h | 5 ++---
kernel/time/posix-timers.c | 10 +---------
kernel/time/time.c | 9 +++++++--
3 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 3ee3b3f1302f..54688c7b4dae 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -54,7 +54,6 @@ struct __sysctl_args;
struct sysinfo;
struct timespec;
struct timeval;
-struct timex;
struct timezone;
struct tms;
struct utimbuf;
@@ -677,7 +676,7 @@ asmlinkage long sys_gettimeofday(struct timeval __user *tv,
struct timezone __user *tz);
asmlinkage long sys_settimeofday(struct timeval __user *tv,
struct timezone __user *tz);
-asmlinkage long sys_adjtimex(struct timex __user *txc_p);
+asmlinkage long sys_adjtimex(struct __kernel_timex __user *txc_p);
/* kernel/timer.c */
asmlinkage long sys_getpid(void);
@@ -846,7 +845,7 @@ asmlinkage long sys_open_by_handle_at(int mountdirfd,
struct file_handle __user *handle,
int flags);
asmlinkage long sys_clock_adjtime(clockid_t which_clock,
- struct timex __user *tx);
+ struct __kernel_timex __user *tx);
asmlinkage long sys_syncfs(int fd);
asmlinkage long sys_setns(int fd, int nstype);
asmlinkage long sys_sendmmsg(int fd, struct mmsghdr __user *msg,
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 640e30792413..8ae5d73ecee3 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -1075,7 +1075,7 @@ SYSCALL_DEFINE2(clock_gettime, const clockid_t, which_clock,
}
SYSCALL_DEFINE2(clock_adjtime, const clockid_t, which_clock,
- struct timex __user *, utx)
+ struct __kernel_timex __user *, utx)
{
const struct k_clock *kc = clockid_to_kclock(which_clock);
struct __kernel_timex ktx;
@@ -1150,10 +1150,6 @@ COMPAT_SYSCALL_DEFINE2(clock_gettime, clockid_t, which_clock,
return err;
}
-#endif
-
-#ifdef CONFIG_COMPAT
-
COMPAT_SYSCALL_DEFINE2(clock_adjtime, clockid_t, which_clock,
struct compat_timex __user *, utp)
{
@@ -1178,10 +1174,6 @@ COMPAT_SYSCALL_DEFINE2(clock_adjtime, clockid_t, which_clock,
return err;
}
-#endif
-
-#ifdef CONFIG_COMPAT_32BIT_TIME
-
COMPAT_SYSCALL_DEFINE2(clock_getres, clockid_t, which_clock,
struct compat_timespec __user *, tp)
{
diff --git a/kernel/time/time.c b/kernel/time/time.c
index 2c5afb008b14..a374fdbb368b 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -263,7 +263,10 @@ COMPAT_SYSCALL_DEFINE2(settimeofday, struct compat_timeval __user *, tv,
}
#endif
-SYSCALL_DEFINE1(adjtimex, struct timex __user *, txc_p)
+
+#if !defined(CONFIG_64BIT_TIME) || defined(CONFIG_64BIT)
+
+SYSCALL_DEFINE1(adjtimex, struct __kernel_timex __user *, txc_p)
{
struct __kernel_timex txc; /* Local copy of parameter */
int ret;
@@ -278,7 +281,9 @@ SYSCALL_DEFINE1(adjtimex, struct timex __user *, txc_p)
return copy_to_user(txc_p, &txc, sizeof(struct __kernel_timex)) ? -EFAULT : ret;
}
-#ifdef CONFIG_COMPAT
+#endif
+
+#ifdef CONFIG_COMPAT_32BIT_TIME
COMPAT_SYSCALL_DEFINE1(adjtimex, struct compat_timex __user *, utp)
{
--
2.17.1
^ permalink raw reply related
* Re: [REGRESSION] "Locked" and "Pss" in /proc/*/smaps are the same
From: Vlastimil Babka @ 2018-07-03 7:36 UTC (permalink / raw)
To: Thomas Lindroth, dancol, Andrew Morton; +Cc: linux-api, linux-kernel, linux-mm
In-Reply-To: <69eb77f7-c8cc-fdee-b44f-ad7e522b8467@gmail.com>
+CC
On 07/01/2018 08:31 PM, Thomas Lindroth wrote:
> While looking around in /proc on my v4.14.52 system I noticed that
> all processes got a lot of "Locked" memory in /proc/*/smaps. A lot
> more memory than a regular user can usually lock with mlock().
>
> commit 493b0e9d945fa9dfe96be93ae41b4ca4b6fdb317 (v4.14-rc1) seems
> to have changed the behavior of "Locked".
>
> commit 493b0e9d945fa9dfe96be93ae41b4ca4b6fdb317
> Author: Daniel Colascione <dancol@google.com>
> Date: Wed Sep 6 16:25:08 2017 -0700
>
> mm: add /proc/pid/smaps_rollup
>
> Before that commit the code was like this. Notice the VM_LOCKED
> check.
>
> seq_printf(m,
> "Size: %8lu kB\n"
> "Rss: %8lu kB\n"
> "Pss: %8lu kB\n"
> "Shared_Clean: %8lu kB\n"
> "Shared_Dirty: %8lu kB\n"
> "Private_Clean: %8lu kB\n"
> "Private_Dirty: %8lu kB\n"
> "Referenced: %8lu kB\n"
> "Anonymous: %8lu kB\n"
> "LazyFree: %8lu kB\n"
> "AnonHugePages: %8lu kB\n"
> "ShmemPmdMapped: %8lu kB\n"
> "Shared_Hugetlb: %8lu kB\n"
> "Private_Hugetlb: %7lu kB\n"
> "Swap: %8lu kB\n"
> "SwapPss: %8lu kB\n"
> "KernelPageSize: %8lu kB\n"
> "MMUPageSize: %8lu kB\n"
> "Locked: %8lu kB\n",
> (vma->vm_end - vma->vm_start) >> 10,
> mss.resident >> 10,
> (unsigned long)(mss.pss >> (10 + PSS_SHIFT)),
> mss.shared_clean >> 10,
> mss.shared_dirty >> 10,
> mss.private_clean >> 10,
> mss.private_dirty >> 10,
> mss.referenced >> 10,
> mss.anonymous >> 10,
> mss.lazyfree >> 10,
> mss.anonymous_thp >> 10,
> mss.shmem_thp >> 10,
> mss.shared_hugetlb >> 10,
> mss.private_hugetlb >> 10,
> mss.swap >> 10,
> (unsigned long)(mss.swap_pss >> (10 + PSS_SHIFT)),
> vma_kernel_pagesize(vma) >> 10,
> vma_mmu_pagesize(vma) >> 10,
> (vma->vm_flags & VM_LOCKED) ?
> (unsigned long)(mss.pss >> (10 + PSS_SHIFT)) : 0);
>
> After that commit Locked is now the same as Pss. This looks like a
> mistake.
>
> seq_printf(m,
> "Rss: %8lu kB\n"
> "Pss: %8lu kB\n"
> "Shared_Clean: %8lu kB\n"
> "Shared_Dirty: %8lu kB\n"
> "Private_Clean: %8lu kB\n"
> "Private_Dirty: %8lu kB\n"
> "Referenced: %8lu kB\n"
> "Anonymous: %8lu kB\n"
> "LazyFree: %8lu kB\n"
> "AnonHugePages: %8lu kB\n"
> "ShmemPmdMapped: %8lu kB\n"
> "Shared_Hugetlb: %8lu kB\n"
> "Private_Hugetlb: %7lu kB\n"
> "Swap: %8lu kB\n"
> "SwapPss: %8lu kB\n"
> "Locked: %8lu kB\n",
> mss->resident >> 10,
> (unsigned long)(mss->pss >> (10 + PSS_SHIFT)),
> mss->shared_clean >> 10,
> mss->shared_dirty >> 10,
> mss->private_clean >> 10,
> mss->private_dirty >> 10,
> mss->referenced >> 10,
> mss->anonymous >> 10,
> mss->lazyfree >> 10,
> mss->anonymous_thp >> 10,
> mss->shmem_thp >> 10,
> mss->shared_hugetlb >> 10,
> mss->private_hugetlb >> 10,
> mss->swap >> 10,
> (unsigned long)(mss->swap_pss >> (10 + PSS_SHIFT)),
> (unsigned long)(mss->pss >> (10 + PSS_SHIFT)));
>
> The latest git has changed a bit but the functionality is the
> same.
----8<----
>From fa721521c981167c24ac8f4be446443d293d741e Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Tue, 3 Jul 2018 09:24:27 +0200
Subject: [PATCH] mm: fix Locked field in /proc/pid/smaps*
Thomas reports:
: While looking around in /proc on my v4.14.52 system I noticed that
: all processes got a lot of "Locked" memory in /proc/*/smaps. A lot
: more memory than a regular user can usually lock with mlock().
:
: commit 493b0e9d945fa9dfe96be93ae41b4ca4b6fdb317 (v4.14-rc1) seems
: to have changed the behavior of "Locked".
:
: Before that commit the code was like this. Notice the VM_LOCKED
: check.
:
: (vma->vm_flags & VM_LOCKED) ?
: (unsigned long)(mss.pss >> (10 + PSS_SHIFT)) : 0);
:
: After that commit Locked is now the same as Pss. This looks like a
: mistake.
:
: (unsigned long)(mss->pss >> (10 + PSS_SHIFT)));
Indeed, the commit has added mss->pss_locked with the correct value that
depends on VM_LOCKED, but forgot to actually use it. Fix it.
Fixes: 493b0e9d945f ("mm: add /proc/pid/smaps_rollup")
Reported-by: Thomas Lindroth <thomas.lindroth@gmail.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: stable@vger.kernel.org
---
fs/proc/task_mmu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index e9679016271f..dfd73a4616ce 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -831,7 +831,8 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
SEQ_PUT_DEC(" kB\nSwap: ", mss->swap);
SEQ_PUT_DEC(" kB\nSwapPss: ",
mss->swap_pss >> PSS_SHIFT);
- SEQ_PUT_DEC(" kB\nLocked: ", mss->pss >> PSS_SHIFT);
+ SEQ_PUT_DEC(" kB\nLocked: ",
+ mss->pss_locked >> PSS_SHIFT);
seq_puts(m, " kB\n");
}
if (!rollup_mode) {
--
2.18.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