* [PATCH 0/3] rseq: minor optimizations
@ 2021-04-13 7:36 Eric Dumazet
2021-04-13 7:36 ` [PATCH 1/3] rseq: optimize rseq_update_cpu_id() Eric Dumazet
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Eric Dumazet @ 2021-04-13 7:36 UTC (permalink / raw)
To: Ingo Molnar, Mathieu Desnoyers, Peter Zijlstra
Cc: Paul E . McKenney, Boqun Feng, Arjun Roy, linux-kernel,
Eric Dumazet, Eric Dumazet
From: Eric Dumazet <edumazet@google.com>
rseq is a heavy user of copy to/from user data in fast paths.
This series tries to reduce the cost.
Eric Dumazet (3):
rseq: optimize rseq_update_cpu_id()
rseq: remove redundant access_ok()
rseq: optimise for 64bit arches
kernel/rseq.c | 26 ++++++++++++++++++++------
1 file changed, 20 insertions(+), 6 deletions(-)
--
2.31.1.295.g9ea45b61b8-goog
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] rseq: optimize rseq_update_cpu_id()
2021-04-13 7:36 [PATCH 0/3] rseq: minor optimizations Eric Dumazet
@ 2021-04-13 7:36 ` Eric Dumazet
2021-04-13 14:29 ` Mathieu Desnoyers
2021-04-13 7:36 ` [PATCH 2/3] rseq: remove redundant access_ok() Eric Dumazet
2021-04-13 7:36 ` [PATCH 3/3] rseq: optimise for 64bit arches Eric Dumazet
2 siblings, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2021-04-13 7:36 UTC (permalink / raw)
To: Ingo Molnar, Mathieu Desnoyers, Peter Zijlstra
Cc: Paul E . McKenney, Boqun Feng, Arjun Roy, linux-kernel,
Eric Dumazet, Eric Dumazet
From: Eric Dumazet <edumazet@google.com>
Two put_user() in rseq_update_cpu_id() are replaced
by a pair of unsafe_put_user() with appropriate surroundings.
This removes one stac/clac pair on x86 in fast path.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Arjun Roy <arjunroy@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
kernel/rseq.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/kernel/rseq.c b/kernel/rseq.c
index a4f86a9d6937cdfa2f13d1dcc9be863c1943d06f..d2689ccbb132c0fc8ec0924008771e5ee1ca855e 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -84,13 +84,20 @@
static int rseq_update_cpu_id(struct task_struct *t)
{
u32 cpu_id = raw_smp_processor_id();
+ struct rseq *r = t->rseq;
- if (put_user(cpu_id, &t->rseq->cpu_id_start))
- return -EFAULT;
- if (put_user(cpu_id, &t->rseq->cpu_id))
- return -EFAULT;
+ if (!user_write_access_begin(r, sizeof(*r)))
+ goto efault;
+ unsafe_put_user(cpu_id, &r->cpu_id_start, efault_end);
+ unsafe_put_user(cpu_id, &r->cpu_id, efault_end);
+ user_write_access_end();
trace_rseq_update(t);
return 0;
+
+efault_end:
+ user_write_access_end();
+efault:
+ return -EFAULT;
}
static int rseq_reset_rseq_cpu_id(struct task_struct *t)
--
2.31.1.295.g9ea45b61b8-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] rseq: remove redundant access_ok()
2021-04-13 7:36 [PATCH 0/3] rseq: minor optimizations Eric Dumazet
2021-04-13 7:36 ` [PATCH 1/3] rseq: optimize rseq_update_cpu_id() Eric Dumazet
@ 2021-04-13 7:36 ` Eric Dumazet
2021-04-13 14:34 ` Mathieu Desnoyers
2021-04-13 7:36 ` [PATCH 3/3] rseq: optimise for 64bit arches Eric Dumazet
2 siblings, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2021-04-13 7:36 UTC (permalink / raw)
To: Ingo Molnar, Mathieu Desnoyers, Peter Zijlstra
Cc: Paul E . McKenney, Boqun Feng, Arjun Roy, linux-kernel,
Eric Dumazet, Eric Dumazet
From: Eric Dumazet <edumazet@google.com>
After commit 8f2817701492 ("rseq: Use get_user/put_user rather
than __get_user/__put_user") we no longer need
an access_ok() call from __rseq_handle_notify_resume()
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Arjun Roy <arjunroy@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
kernel/rseq.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/kernel/rseq.c b/kernel/rseq.c
index d2689ccbb132c0fc8ec0924008771e5ee1ca855e..57344f9abb43905c7dd2b6081205ff508d963e1e 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -273,8 +273,6 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
if (unlikely(t->flags & PF_EXITING))
return;
- if (unlikely(!access_ok(t->rseq, sizeof(*t->rseq))))
- goto error;
ret = rseq_ip_fixup(regs);
if (unlikely(ret < 0))
goto error;
--
2.31.1.295.g9ea45b61b8-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] rseq: optimise for 64bit arches
2021-04-13 7:36 [PATCH 0/3] rseq: minor optimizations Eric Dumazet
2021-04-13 7:36 ` [PATCH 1/3] rseq: optimize rseq_update_cpu_id() Eric Dumazet
2021-04-13 7:36 ` [PATCH 2/3] rseq: remove redundant access_ok() Eric Dumazet
@ 2021-04-13 7:36 ` Eric Dumazet
2021-04-13 9:10 ` Peter Zijlstra
2 siblings, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2021-04-13 7:36 UTC (permalink / raw)
To: Ingo Molnar, Mathieu Desnoyers, Peter Zijlstra
Cc: Paul E . McKenney, Boqun Feng, Arjun Roy, linux-kernel,
Eric Dumazet, Eric Dumazet
From: Eric Dumazet <edumazet@google.com>
Commit ec9c82e03a74 ("rseq: uapi: Declare rseq_cs field as union,
update includes") added regressions for our servers.
Using copy_from_user() and clear_user() for 64bit values
on 64bit arches is suboptimal.
We might revisit this patch once all 32bit arches support
get_user() and/or put_user() for 8 bytes values.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Arjun Roy <arjunroy@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
kernel/rseq.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 57344f9abb43905c7dd2b6081205ff508d963e1e..18a75a804008d2f564d1f7789f09216f1a8760bd 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -127,8 +127,13 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
u32 sig;
int ret;
+#ifdef CONFIG_64BIT
+ if (get_user(ptr, &t->rseq->rseq_cs.ptr64))
+ return -EFAULT;
+#else
if (copy_from_user(&ptr, &t->rseq->rseq_cs.ptr64, sizeof(ptr)))
return -EFAULT;
+#endif
if (!ptr) {
memset(rseq_cs, 0, sizeof(*rseq_cs));
return 0;
@@ -211,9 +216,13 @@ static int clear_rseq_cs(struct task_struct *t)
*
* Set rseq_cs to NULL.
*/
+#ifdef CONFIG_64BIT
+ return put_user(0ULL, &t->rseq->rseq_cs.ptr64);
+#else
if (clear_user(&t->rseq->rseq_cs.ptr64, sizeof(t->rseq->rseq_cs.ptr64)))
return -EFAULT;
return 0;
+#endif
}
/*
--
2.31.1.295.g9ea45b61b8-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] rseq: optimise for 64bit arches
2021-04-13 7:36 ` [PATCH 3/3] rseq: optimise for 64bit arches Eric Dumazet
@ 2021-04-13 9:10 ` Peter Zijlstra
2021-04-13 10:36 ` David Laight
0 siblings, 1 reply; 13+ messages in thread
From: Peter Zijlstra @ 2021-04-13 9:10 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ingo Molnar, Mathieu Desnoyers, Paul E . McKenney, Boqun Feng,
Arjun Roy, linux-kernel, Eric Dumazet
On Tue, Apr 13, 2021 at 12:36:57AM -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Commit ec9c82e03a74 ("rseq: uapi: Declare rseq_cs field as union,
> update includes") added regressions for our servers.
>
> Using copy_from_user() and clear_user() for 64bit values
> on 64bit arches is suboptimal.
>
> We might revisit this patch once all 32bit arches support
> get_user() and/or put_user() for 8 bytes values.
Argh, what a mess :/ afaict only nios32 lacks put_user_8, but get_user_8
is missing in a fair number of archs.
That said; 32bit archs never have to actually set the top bits in that
word, so they _could_ only set the low 32 bits. That works provided
userspace itself keeps the high bits clear.
So I suppose that if we're going to #ifdef this, we might as well do the
whole thing.
Mathieu; did I forget a reason why this cannot work?
diff --git a/kernel/rseq.c b/kernel/rseq.c
index a4f86a9d6937..94006190b8eb 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -115,20 +115,25 @@ static int rseq_reset_rseq_cpu_id(struct task_struct *t)
static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
{
struct rseq_cs __user *urseq_cs;
- u64 ptr;
+ unsigned long ptr;
u32 __user *usig;
u32 sig;
int ret;
- if (copy_from_user(&ptr, &t->rseq->rseq_cs.ptr64, sizeof(ptr)))
+#ifdef CONFIG_64BIT
+ if (get_user(ptr, &t->rseq->rseq_cs.ptr64))
return -EFAULT;
+#else
+ if (get_user(ptr, &t->rseq->rseq_cs.ptr32))
+ return -EFAULT;
+#endif
if (!ptr) {
memset(rseq_cs, 0, sizeof(*rseq_cs));
return 0;
}
if (ptr >= TASK_SIZE)
return -EINVAL;
- urseq_cs = (struct rseq_cs __user *)(unsigned long)ptr;
+ urseq_cs = (struct rseq_cs __user *)ptr;
if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
return -EFAULT;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* RE: [PATCH 3/3] rseq: optimise for 64bit arches
2021-04-13 9:10 ` Peter Zijlstra
@ 2021-04-13 10:36 ` David Laight
2021-04-13 14:21 ` Mathieu Desnoyers
0 siblings, 1 reply; 13+ messages in thread
From: David Laight @ 2021-04-13 10:36 UTC (permalink / raw)
To: 'Peter Zijlstra', Eric Dumazet
Cc: Ingo Molnar, Mathieu Desnoyers, Paul E . McKenney, Boqun Feng,
Arjun Roy, linux-kernel, Eric Dumazet
From: Peter Zijlstra
> Sent: 13 April 2021 10:10
>
> On Tue, Apr 13, 2021 at 12:36:57AM -0700, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Commit ec9c82e03a74 ("rseq: uapi: Declare rseq_cs field as union,
> > update includes") added regressions for our servers.
> >
> > Using copy_from_user() and clear_user() for 64bit values
> > on 64bit arches is suboptimal.
> >
> > We might revisit this patch once all 32bit arches support
> > get_user() and/or put_user() for 8 bytes values.
>
> Argh, what a mess :/ afaict only nios32 lacks put_user_8, but get_user_8
> is missing in a fair number of archs.
>
> That said; 32bit archs never have to actually set the top bits in that
> word, so they _could_ only set the low 32 bits. That works provided
> userspace itself keeps the high bits clear.
Does that work for 32bit BE ?
David
> So I suppose that if we're going to #ifdef this, we might as well do the
> whole thing.
>
> Mathieu; did I forget a reason why this cannot work?
>
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index a4f86a9d6937..94006190b8eb 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -115,20 +115,25 @@ static int rseq_reset_rseq_cpu_id(struct task_struct *t)
> static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
> {
> struct rseq_cs __user *urseq_cs;
> - u64 ptr;
> + unsigned long ptr;
> u32 __user *usig;
> u32 sig;
> int ret;
>
> - if (copy_from_user(&ptr, &t->rseq->rseq_cs.ptr64, sizeof(ptr)))
> +#ifdef CONFIG_64BIT
> + if (get_user(ptr, &t->rseq->rseq_cs.ptr64))
> return -EFAULT;
> +#else
> + if (get_user(ptr, &t->rseq->rseq_cs.ptr32))
> + return -EFAULT;
> +#endif
> if (!ptr) {
> memset(rseq_cs, 0, sizeof(*rseq_cs));
> return 0;
> }
> if (ptr >= TASK_SIZE)
> return -EINVAL;
> - urseq_cs = (struct rseq_cs __user *)(unsigned long)ptr;
> + urseq_cs = (struct rseq_cs __user *)ptr;
> if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
> return -EFAULT;
>
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] rseq: optimise for 64bit arches
2021-04-13 10:36 ` David Laight
@ 2021-04-13 14:21 ` Mathieu Desnoyers
2021-04-13 15:06 ` David Laight
0 siblings, 1 reply; 13+ messages in thread
From: Mathieu Desnoyers @ 2021-04-13 14:21 UTC (permalink / raw)
To: David Laight, Peter Zijlstra
Cc: Eric Dumazet, Ingo Molnar, paulmck, Boqun Feng, Arjun Roy,
linux-kernel, Eric Dumazet
----- On Apr 13, 2021, at 6:36 AM, David Laight David.Laight@ACULAB.COM wrote:
> From: Peter Zijlstra
>> Sent: 13 April 2021 10:10
>>
>> On Tue, Apr 13, 2021 at 12:36:57AM -0700, Eric Dumazet wrote:
>> > From: Eric Dumazet <edumazet@google.com>
>> >
>> > Commit ec9c82e03a74 ("rseq: uapi: Declare rseq_cs field as union,
>> > update includes") added regressions for our servers.
>> >
>> > Using copy_from_user() and clear_user() for 64bit values
>> > on 64bit arches is suboptimal.
>> >
>> > We might revisit this patch once all 32bit arches support
>> > get_user() and/or put_user() for 8 bytes values.
>>
>> Argh, what a mess :/ afaict only nios32 lacks put_user_8, but get_user_8
>> is missing in a fair number of archs.
>>
>> That said; 32bit archs never have to actually set the top bits in that
>> word, so they _could_ only set the low 32 bits. That works provided
>> userspace itself keeps the high bits clear.
>
> Does that work for 32bit BE ?
Yes, because uapi/linux/rseq.h defines the ptr32 as:
#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || defined(__BIG_ENDIAN)
__u32 padding; /* Initialized to zero. */
__u32 ptr32;
#else /* LITTLE */
__u32 ptr32;
__u32 padding; /* Initialized to zero. */
#endif /* ENDIAN */
which takes care of BE vs LE.
>
> David
>
>> So I suppose that if we're going to #ifdef this, we might as well do the
>> whole thing.
>>
>> Mathieu; did I forget a reason why this cannot work?
The only difference it brings on 32-bit is that the truncation of high bits
will be done before the following validation:
if (!ptr) {
memset(rseq_cs, 0, sizeof(*rseq_cs));
return 0;
}
if (ptr >= TASK_SIZE)
return -EINVAL;
The question is whether we really want to issue a segmentation fault if 32-bit
user-space has set non-zero high bits, or if silently ignoring those high
bits is acceptable.
Nits below:
>>
>> diff --git a/kernel/rseq.c b/kernel/rseq.c
>> index a4f86a9d6937..94006190b8eb 100644
>> --- a/kernel/rseq.c
>> +++ b/kernel/rseq.c
>> @@ -115,20 +115,25 @@ static int rseq_reset_rseq_cpu_id(struct task_struct *t)
>> static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
>> {
>> struct rseq_cs __user *urseq_cs;
>> - u64 ptr;
>> + unsigned long ptr;
I am always reluctant to use long/unsigned long type as type for the get/put_user
(x) argument, because it hides the cast deep within architecture-specific macros.
I understand that in this specific case it happens that on 64-bit archs we end up
casting a u64 to unsigned long (same size), and on 32-bit archs we end up casting a
u32 to unsigned long (also same size), so there is no practical concern about type
promotion and sign-extension, but I think it would be better to have something
explicit, e.g.:
#ifdef CONFIG_64BIT
static int rseq_get_cs_ptr(struct rseq_cs __user **uptrp, struct rseq_cs *rseq_cs)
{
u64 ptr;
if (get_user(ptr, &rseq_cs->ptr64))
return -EFAULT;
*ptrp = (struct rseq_cs __user *)ptr;
return 0;
}
#else
static int rseq_get_cs_ptr(struct rseq_cs __user **uptrp, struct rseq_cs *rseq_cs)
{
u32 ptr;
if (get_user(ptr, &rseq_cs->ptr.ptr32))
return -EFAULT;
*ptrp = (struct rseq_cs __user *)ptr;
return 0;
}
#endif
And use those helpers to get the ptr value.
>> u32 __user *usig;
>> u32 sig;
>> int ret;
>>
>> - if (copy_from_user(&ptr, &t->rseq->rseq_cs.ptr64, sizeof(ptr)))
>> +#ifdef CONFIG_64BIT
>> + if (get_user(ptr, &t->rseq->rseq_cs.ptr64))
>> return -EFAULT;
>> +#else
>> + if (get_user(ptr, &t->rseq->rseq_cs.ptr32))
Note that this is also not right. It should be &t->rseq->rseq_cs.ptr.ptr32.
Thanks,
Mathieu
>> + return -EFAULT;
>> +#endif
>> if (!ptr) {
>> memset(rseq_cs, 0, sizeof(*rseq_cs));
>> return 0;
>> }
>> if (ptr >= TASK_SIZE)
>> return -EINVAL;
>> - urseq_cs = (struct rseq_cs __user *)(unsigned long)ptr;
>> + urseq_cs = (struct rseq_cs __user *)ptr;
>> if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
>> return -EFAULT;
>>
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
> UK
> Registration No: 1397386 (Wales)
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] rseq: optimize rseq_update_cpu_id()
2021-04-13 7:36 ` [PATCH 1/3] rseq: optimize rseq_update_cpu_id() Eric Dumazet
@ 2021-04-13 14:29 ` Mathieu Desnoyers
2021-04-13 15:24 ` Eric Dumazet
0 siblings, 1 reply; 13+ messages in thread
From: Mathieu Desnoyers @ 2021-04-13 14:29 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ingo Molnar, Peter Zijlstra, paulmck, Boqun Feng, Arjun Roy,
linux-kernel, Eric Dumazet
----- On Apr 13, 2021, at 3:36 AM, Eric Dumazet eric.dumazet@gmail.com wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Two put_user() in rseq_update_cpu_id() are replaced
> by a pair of unsafe_put_user() with appropriate surroundings.
>
> This removes one stac/clac pair on x86 in fast path.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Arjun Roy <arjunroy@google.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> ---
> kernel/rseq.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index
> a4f86a9d6937cdfa2f13d1dcc9be863c1943d06f..d2689ccbb132c0fc8ec0924008771e5ee1ca855e
> 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -84,13 +84,20 @@
> static int rseq_update_cpu_id(struct task_struct *t)
> {
> u32 cpu_id = raw_smp_processor_id();
> + struct rseq *r = t->rseq;
AFAIU the variable above should be a struct rseq __user *.
Elsewhere in the file we use "rseq" rather than "r" for struct rseq __user *
variable name, it would be better to keep the naming consistent across the file
if possible.
Thanks,
Mathieu
>
> - if (put_user(cpu_id, &t->rseq->cpu_id_start))
> - return -EFAULT;
> - if (put_user(cpu_id, &t->rseq->cpu_id))
> - return -EFAULT;
> + if (!user_write_access_begin(r, sizeof(*r)))
> + goto efault;
> + unsafe_put_user(cpu_id, &r->cpu_id_start, efault_end);
> + unsafe_put_user(cpu_id, &r->cpu_id, efault_end);
> + user_write_access_end();
> trace_rseq_update(t);
> return 0;
> +
> +efault_end:
> + user_write_access_end();
> +efault:
> + return -EFAULT;
> }
>
> static int rseq_reset_rseq_cpu_id(struct task_struct *t)
> --
> 2.31.1.295.g9ea45b61b8-goog
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] rseq: remove redundant access_ok()
2021-04-13 7:36 ` [PATCH 2/3] rseq: remove redundant access_ok() Eric Dumazet
@ 2021-04-13 14:34 ` Mathieu Desnoyers
2021-04-13 15:19 ` Eric Dumazet
0 siblings, 1 reply; 13+ messages in thread
From: Mathieu Desnoyers @ 2021-04-13 14:34 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ingo Molnar, Peter Zijlstra, paulmck, Boqun Feng, Arjun Roy,
linux-kernel, Eric Dumazet
----- On Apr 13, 2021, at 3:36 AM, Eric Dumazet eric.dumazet@gmail.com wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> After commit 8f2817701492 ("rseq: Use get_user/put_user rather
> than __get_user/__put_user") we no longer need
> an access_ok() call from __rseq_handle_notify_resume()
While we are doing that, should we also remove the access_ok() check in
rseq_syscall() ? It look like it can also be removed for the exact same
reason outlined here.
Thanks,
Mathieu
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Arjun Roy <arjunroy@google.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> ---
> kernel/rseq.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index
> d2689ccbb132c0fc8ec0924008771e5ee1ca855e..57344f9abb43905c7dd2b6081205ff508d963e1e
> 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -273,8 +273,6 @@ void __rseq_handle_notify_resume(struct ksignal *ksig,
> struct pt_regs *regs)
>
> if (unlikely(t->flags & PF_EXITING))
> return;
> - if (unlikely(!access_ok(t->rseq, sizeof(*t->rseq))))
> - goto error;
> ret = rseq_ip_fixup(regs);
> if (unlikely(ret < 0))
> goto error;
> --
> 2.31.1.295.g9ea45b61b8-goog
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 3/3] rseq: optimise for 64bit arches
2021-04-13 14:21 ` Mathieu Desnoyers
@ 2021-04-13 15:06 ` David Laight
2021-04-13 15:08 ` Mathieu Desnoyers
0 siblings, 1 reply; 13+ messages in thread
From: David Laight @ 2021-04-13 15:06 UTC (permalink / raw)
To: 'Mathieu Desnoyers', Peter Zijlstra
Cc: Eric Dumazet, Ingo Molnar, paulmck, Boqun Feng, Arjun Roy,
linux-kernel, Eric Dumazet
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Sent: 13 April 2021 15:22
...
> > David
> >
> >> So I suppose that if we're going to #ifdef this, we might as well do the
> >> whole thing.
> >>
> >> Mathieu; did I forget a reason why this cannot work?
>
> The only difference it brings on 32-bit is that the truncation of high bits
> will be done before the following validation:
>
> if (!ptr) {
> memset(rseq_cs, 0, sizeof(*rseq_cs));
> return 0;
> }
> if (ptr >= TASK_SIZE)
> return -EINVAL;
>
> The question is whether we really want to issue a segmentation fault if 32-bit
> user-space has set non-zero high bits, or if silently ignoring those high
> bits is acceptable.
Well, 32bit programs running on 64bit kernels better zero the 'padding'.
It is a shame that the 32bit compilers don't have an attribute(64bit)
for pointers.
> ...
> I am always reluctant to use long/unsigned long type as type for the get/put_user
> (x) argument, because it hides the cast deep within architecture-specific macros.
> I understand that in this specific case it happens that on 64-bit archs we end up
> casting a u64 to unsigned long (same size), and on 32-bit archs we end up casting a
> u32 to unsigned long (also same size), so there is no practical concern about type
> promotion and sign-extension, but I think it would be better to have something
> explicit, e.g.:
If the ABI passes small structures in registers (most modern ones)
then user pointers probably ought to be encapsulated in a structure.
Possibly as a pointer to an unknown structure.
That would force all the required type checking.
Unfortunately the fallout/churn would be massive.
> #ifdef CONFIG_64BIT
> static int rseq_get_cs_ptr(struct rseq_cs __user **uptrp, struct rseq_cs *rseq_cs)
> {
> u64 ptr;
>
> if (get_user(ptr, &rseq_cs->ptr64))
> return -EFAULT;
> *ptrp = (struct rseq_cs __user *)ptr;
> return 0;
> }
> #else
> static int rseq_get_cs_ptr(struct rseq_cs __user **uptrp, struct rseq_cs *rseq_cs)
> {
> u32 ptr;
>
> if (get_user(ptr, &rseq_cs->ptr.ptr32))
> return -EFAULT;
> *ptrp = (struct rseq_cs __user *)ptr;
> return 0;
> }
> #endif
Hmmm... too much replication.
You could do:
#ifdef CONFIG_64BIT
#define PTR_TYPE u64
#define PTR_FLD ptr64
#else
#define PTR_TYPE u32
#define PTR_FLD ptr32
#endif
Then have one copy of the code and the #undefs.
...
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] rseq: optimise for 64bit arches
2021-04-13 15:06 ` David Laight
@ 2021-04-13 15:08 ` Mathieu Desnoyers
0 siblings, 0 replies; 13+ messages in thread
From: Mathieu Desnoyers @ 2021-04-13 15:08 UTC (permalink / raw)
To: David Laight
Cc: Peter Zijlstra, Eric Dumazet, Ingo Molnar, paulmck, Boqun Feng,
Arjun Roy, linux-kernel, Eric Dumazet
----- On Apr 13, 2021, at 11:06 AM, David Laight David.Laight@ACULAB.COM wrote:
[...]
>
> Hmmm... too much replication.
> You could do:
> #ifdef CONFIG_64BIT
> #define PTR_TYPE u64
> #define PTR_FLD ptr64
> #else
> #define PTR_TYPE u32
> #define PTR_FLD ptr32
> #endif
>
> Then have one copy of the code and the #undefs.
Good point, agreed.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] rseq: remove redundant access_ok()
2021-04-13 14:34 ` Mathieu Desnoyers
@ 2021-04-13 15:19 ` Eric Dumazet
0 siblings, 0 replies; 13+ messages in thread
From: Eric Dumazet @ 2021-04-13 15:19 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Eric Dumazet, Ingo Molnar, Peter Zijlstra, paulmck, Boqun Feng,
Arjun Roy, linux-kernel
On Tue, Apr 13, 2021 at 4:34 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> ----- On Apr 13, 2021, at 3:36 AM, Eric Dumazet eric.dumazet@gmail.com wrote:
>
> > From: Eric Dumazet <edumazet@google.com>
> >
> > After commit 8f2817701492 ("rseq: Use get_user/put_user rather
> > than __get_user/__put_user") we no longer need
> > an access_ok() call from __rseq_handle_notify_resume()
>
> While we are doing that, should we also remove the access_ok() check in
> rseq_syscall() ? It look like it can also be removed for the exact same
> reason outlined here.
Yes, good idea.
I was focusing in __rseq_handle_notify_resume() paths but
rseq_syscall() can get the same.
> Thanks,
>
> Mathieu
>
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Arjun Roy <arjunroy@google.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > ---
> > kernel/rseq.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/kernel/rseq.c b/kernel/rseq.c
> > index
> > d2689ccbb132c0fc8ec0924008771e5ee1ca855e..57344f9abb43905c7dd2b6081205ff508d963e1e
> > 100644
> > --- a/kernel/rseq.c
> > +++ b/kernel/rseq.c
> > @@ -273,8 +273,6 @@ void __rseq_handle_notify_resume(struct ksignal *ksig,
> > struct pt_regs *regs)
> >
> > if (unlikely(t->flags & PF_EXITING))
> > return;
> > - if (unlikely(!access_ok(t->rseq, sizeof(*t->rseq))))
> > - goto error;
> > ret = rseq_ip_fixup(regs);
> > if (unlikely(ret < 0))
> > goto error;
> > --
> > 2.31.1.295.g9ea45b61b8-goog
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] rseq: optimize rseq_update_cpu_id()
2021-04-13 14:29 ` Mathieu Desnoyers
@ 2021-04-13 15:24 ` Eric Dumazet
0 siblings, 0 replies; 13+ messages in thread
From: Eric Dumazet @ 2021-04-13 15:24 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Eric Dumazet, Ingo Molnar, Peter Zijlstra, paulmck, Boqun Feng,
Arjun Roy, linux-kernel
On Tue, Apr 13, 2021 at 4:29 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> ----- On Apr 13, 2021, at 3:36 AM, Eric Dumazet eric.dumazet@gmail.com wrote:
>
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Two put_user() in rseq_update_cpu_id() are replaced
> > by a pair of unsafe_put_user() with appropriate surroundings.
> >
> > This removes one stac/clac pair on x86 in fast path.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Arjun Roy <arjunroy@google.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > ---
> > kernel/rseq.c | 15 +++++++++++----
> > 1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/rseq.c b/kernel/rseq.c
> > index
> > a4f86a9d6937cdfa2f13d1dcc9be863c1943d06f..d2689ccbb132c0fc8ec0924008771e5ee1ca855e
> > 100644
> > --- a/kernel/rseq.c
> > +++ b/kernel/rseq.c
> > @@ -84,13 +84,20 @@
> > static int rseq_update_cpu_id(struct task_struct *t)
> > {
> > u32 cpu_id = raw_smp_processor_id();
> > + struct rseq *r = t->rseq;
>
> AFAIU the variable above should be a struct rseq __user *.
>
> Elsewhere in the file we use "rseq" rather than "r" for struct rseq __user *
> variable name, it would be better to keep the naming consistent across the file
> if possible.
Absolutely, thanks for the feedback.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-04-13 15:25 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13 7:36 [PATCH 0/3] rseq: minor optimizations Eric Dumazet
2021-04-13 7:36 ` [PATCH 1/3] rseq: optimize rseq_update_cpu_id() Eric Dumazet
2021-04-13 14:29 ` Mathieu Desnoyers
2021-04-13 15:24 ` Eric Dumazet
2021-04-13 7:36 ` [PATCH 2/3] rseq: remove redundant access_ok() Eric Dumazet
2021-04-13 14:34 ` Mathieu Desnoyers
2021-04-13 15:19 ` Eric Dumazet
2021-04-13 7:36 ` [PATCH 3/3] rseq: optimise for 64bit arches Eric Dumazet
2021-04-13 9:10 ` Peter Zijlstra
2021-04-13 10:36 ` David Laight
2021-04-13 14:21 ` Mathieu Desnoyers
2021-04-13 15:06 ` David Laight
2021-04-13 15:08 ` Mathieu Desnoyers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).