[PATCH 0/3] rseq: minor optimizations

[PATCH 0/3] rseq: minor optimizations

[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

[PATCH 1/3] rseq: optimize rseq_update_cpu_id()

[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

Re: [PATCH 1/3] rseq: optimize rseq_update_cpu_id()

[Mathieu Desnoyers]

----- 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

Re: [PATCH 1/3] rseq: optimize rseq_update_cpu_id()

[Eric Dumazet]

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.

[PATCH 2/3] rseq: remove redundant access_ok()

[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

Re: [PATCH 2/3] rseq: remove redundant access_ok()

[Mathieu Desnoyers]

----- 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

Re: [PATCH 2/3] rseq: remove redundant access_ok()

[Eric Dumazet]

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

[PATCH 3/3] rseq: optimise for 64bit arches

[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

Re: [PATCH 3/3] rseq: optimise for 64bit arches

[Peter Zijlstra]

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;
 

RE: [PATCH 3/3] rseq: optimise for 64bit arches

[David Laight]

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)

Re: [PATCH 3/3] rseq: optimise for 64bit arches

[Mathieu Desnoyers]

----- 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

RE: [PATCH 3/3] rseq: optimise for 64bit arches

[David Laight]

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)

Re: [PATCH 3/3] rseq: optimise for 64bit arches

[Mathieu Desnoyers]

----- 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