From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: David Laight <David.Laight@ACULAB.COM>,
Peter Zijlstra <peterz@infradead.org>
Cc: Eric Dumazet <eric.dumazet@gmail.com>,
Ingo Molnar <mingo@kernel.org>, paulmck <paulmck@kernel.org>,
Boqun Feng <boqun.feng@gmail.com>,
Arjun Roy <arjunroy@google.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
Eric Dumazet <edumazet@google.com>
Subject: Re: [PATCH 3/3] rseq: optimise for 64bit arches
Date: Tue, 13 Apr 2021 10:21:48 -0400 (EDT) [thread overview]
Message-ID: <644332839.71291.1618323708305.JavaMail.zimbra@efficios.com> (raw)
In-Reply-To: <fbf1a4449b0148b5b9c3baa32088c32a@AcuMS.aculab.com>
----- 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
next prev parent reply other threads:[~2021-04-13 14:21 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2021-04-13 15:06 ` David Laight
2021-04-13 15:08 ` Mathieu Desnoyers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=644332839.71291.1618323708305.JavaMail.zimbra@efficios.com \
--to=mathieu.desnoyers@efficios.com \
--cc=David.Laight@ACULAB.COM \
--cc=arjunroy@google.com \
--cc=boqun.feng@gmail.com \
--cc=edumazet@google.com \
--cc=eric.dumazet@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).