* [PATCH] futex: Check for uaddr alignment as early as possible
@ 2017-12-09 1:06 Darren Hart
2017-12-12 10:23 ` Thomas Gleixner
2017-12-12 10:23 ` Ingo Molnar
0 siblings, 2 replies; 5+ messages in thread
From: Darren Hart @ 2017-12-09 1:06 UTC (permalink / raw)
To: LKML; +Cc: Darren Hart (VMware), Thomas Gleixner, Ingo Molnar, Peter Zijlstra
From: "Darren Hart (VMware)" <dvhart@infradead.org>
uaddr alignment is currently tested by get_futex_key(). We can catch
misalignment earlier in sys_futex and return -EINVAL sooner. This
simplifies get_futex_key() a little, but more importantly exits the
kernel as soon as an invalid parameter is detected.
Passes all selftests/futex testcases on a dual socket Xeon E5-2670, 16
physical cores total, 32 threads total.
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Darren Hart <dvhart@infradead.org>
---
kernel/futex.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index 76ed592..c3ee6c4 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -509,8 +509,6 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
* The futex address must be "naturally" aligned.
*/
key->both.offset = address % PAGE_SIZE;
- if (unlikely((address % sizeof(u32)) != 0))
- return -EINVAL;
address -= key->both.offset;
if (unlikely(!access_ok(rw, uaddr, sizeof(u32))))
@@ -3525,6 +3523,11 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
u32 val2 = 0;
int cmd = op & FUTEX_CMD_MASK;
+ /* Only allow for aligned uaddr variables */
+ if (unlikely((unsigned long)uaddr % sizeof(u32) != 0 ||
+ (unsigned long)uaddr2 % sizeof(u32) != 0))
+ return -EINVAL;
+
if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
cmd == FUTEX_WAIT_BITSET ||
cmd == FUTEX_WAIT_REQUEUE_PI)) {
--
2.9.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] futex: Check for uaddr alignment as early as possible
2017-12-09 1:06 [PATCH] futex: Check for uaddr alignment as early as possible Darren Hart
@ 2017-12-12 10:23 ` Thomas Gleixner
2017-12-12 10:31 ` Ingo Molnar
2017-12-12 10:23 ` Ingo Molnar
1 sibling, 1 reply; 5+ messages in thread
From: Thomas Gleixner @ 2017-12-12 10:23 UTC (permalink / raw)
To: Darren Hart; +Cc: LKML, Ingo Molnar, Peter Zijlstra
On Fri, 8 Dec 2017, Darren Hart wrote:
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
>
> uaddr alignment is currently tested by get_futex_key(). We can catch
> misalignment earlier in sys_futex and return -EINVAL sooner. This
> simplifies get_futex_key() a little, but more importantly exits the
> kernel as soon as an invalid parameter is detected.
>
> Passes all selftests/futex testcases on a dual socket Xeon E5-2670, 16
> physical cores total, 32 threads total.
>
> Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Darren Hart <dvhart@infradead.org>
> ---
> kernel/futex.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 76ed592..c3ee6c4 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -509,8 +509,6 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
> * The futex address must be "naturally" aligned.
> */
> key->both.offset = address % PAGE_SIZE;
> - if (unlikely((address % sizeof(u32)) != 0))
> - return -EINVAL;
> address -= key->both.offset;
>
> if (unlikely(!access_ok(rw, uaddr, sizeof(u32))))
> @@ -3525,6 +3523,11 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
> u32 val2 = 0;
> int cmd = op & FUTEX_CMD_MASK;
>
> + /* Only allow for aligned uaddr variables */
> + if (unlikely((unsigned long)uaddr % sizeof(u32) != 0 ||
> + (unsigned long)uaddr2 % sizeof(u32) != 0))
Errm. How is that supposed to work? uaddr2 is not used by all opcodes.....
Thanks,
tglx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] futex: Check for uaddr alignment as early as possible
2017-12-09 1:06 [PATCH] futex: Check for uaddr alignment as early as possible Darren Hart
2017-12-12 10:23 ` Thomas Gleixner
@ 2017-12-12 10:23 ` Ingo Molnar
1 sibling, 0 replies; 5+ messages in thread
From: Ingo Molnar @ 2017-12-12 10:23 UTC (permalink / raw)
To: Darren Hart; +Cc: LKML, Thomas Gleixner, Ingo Molnar, Peter Zijlstra
* Darren Hart <dvhart@infradead.org> wrote:
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
>
> uaddr alignment is currently tested by get_futex_key(). We can catch
> misalignment earlier in sys_futex and return -EINVAL sooner. This
> simplifies get_futex_key() a little, but more importantly exits the
> kernel as soon as an invalid parameter is detected.
>
> Passes all selftests/futex testcases on a dual socket Xeon E5-2670, 16
> physical cores total, 32 threads total.
>
> Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Darren Hart <dvhart@infradead.org>
> ---
> kernel/futex.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 76ed592..c3ee6c4 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -509,8 +509,6 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
> * The futex address must be "naturally" aligned.
> */
> key->both.offset = address % PAGE_SIZE;
> - if (unlikely((address % sizeof(u32)) != 0))
> - return -EINVAL;
> address -= key->both.offset;
>
> if (unlikely(!access_ok(rw, uaddr, sizeof(u32))))
> @@ -3525,6 +3523,11 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
> u32 val2 = 0;
> int cmd = op & FUTEX_CMD_MASK;
>
> + /* Only allow for aligned uaddr variables */
> + if (unlikely((unsigned long)uaddr % sizeof(u32) != 0 ||
> + (unsigned long)uaddr2 % sizeof(u32) != 0))
> + return -EINVAL;
> +
> if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
> cmd == FUTEX_WAIT_BITSET ||
> cmd == FUTEX_WAIT_REQUEUE_PI)) {
Yeah, so I applied this yesterday, then -tip started regressing sporadically
during distro networking bring-up, and it took me half a day of debugging to
statistically bisect it back to this patch :-/
So it's apparently broken, but I don't see yet how.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] futex: Check for uaddr alignment as early as possible
2017-12-12 10:23 ` Thomas Gleixner
@ 2017-12-12 10:31 ` Ingo Molnar
2017-12-12 15:57 ` Darren Hart
0 siblings, 1 reply; 5+ messages in thread
From: Ingo Molnar @ 2017-12-12 10:31 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Darren Hart, LKML, Ingo Molnar, Peter Zijlstra
* Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 8 Dec 2017, Darren Hart wrote:
>
> > From: "Darren Hart (VMware)" <dvhart@infradead.org>
> >
> > uaddr alignment is currently tested by get_futex_key(). We can catch
> > misalignment earlier in sys_futex and return -EINVAL sooner. This
> > simplifies get_futex_key() a little, but more importantly exits the
> > kernel as soon as an invalid parameter is detected.
> >
> > Passes all selftests/futex testcases on a dual socket Xeon E5-2670, 16
> > physical cores total, 32 threads total.
> >
> > Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Darren Hart <dvhart@infradead.org>
> > ---
> > kernel/futex.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/futex.c b/kernel/futex.c
> > index 76ed592..c3ee6c4 100644
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -509,8 +509,6 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
> > * The futex address must be "naturally" aligned.
> > */
> > key->both.offset = address % PAGE_SIZE;
> > - if (unlikely((address % sizeof(u32)) != 0))
> > - return -EINVAL;
> > address -= key->both.offset;
> >
> > if (unlikely(!access_ok(rw, uaddr, sizeof(u32))))
> > @@ -3525,6 +3523,11 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
> > u32 val2 = 0;
> > int cmd = op & FUTEX_CMD_MASK;
> >
> > + /* Only allow for aligned uaddr variables */
> > + if (unlikely((unsigned long)uaddr % sizeof(u32) != 0 ||
> > + (unsigned long)uaddr2 % sizeof(u32) != 0))
>
> Errm. How is that supposed to work? uaddr2 is not used by all opcodes.....
So to explain the curious timing of the mails from Thomas and me: I told Thomas
about the breakage over IRC and he found the likely bug! ;-)
Thanks,
Ingo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] futex: Check for uaddr alignment as early as possible
2017-12-12 10:31 ` Ingo Molnar
@ 2017-12-12 15:57 ` Darren Hart
0 siblings, 0 replies; 5+ messages in thread
From: Darren Hart @ 2017-12-12 15:57 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Thomas Gleixner, LKML, Ingo Molnar, Peter Zijlstra
On Tue, Dec 12, 2017 at 11:31:02AM +0100, Ingo Molnar wrote:
>
> * Thomas Gleixner <tglx@linutronix.de> wrote:
>
> > On Fri, 8 Dec 2017, Darren Hart wrote:
> >
> > > From: "Darren Hart (VMware)" <dvhart@infradead.org>
> > >
> > > uaddr alignment is currently tested by get_futex_key(). We can catch
> > > misalignment earlier in sys_futex and return -EINVAL sooner. This
> > > simplifies get_futex_key() a little, but more importantly exits the
> > > kernel as soon as an invalid parameter is detected.
> > >
> > > Passes all selftests/futex testcases on a dual socket Xeon E5-2670, 16
> > > physical cores total, 32 threads total.
> > >
> > > Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Ingo Molnar <mingo@redhat.com>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Darren Hart <dvhart@infradead.org>
> > > ---
> > > kernel/futex.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kernel/futex.c b/kernel/futex.c
> > > index 76ed592..c3ee6c4 100644
> > > --- a/kernel/futex.c
> > > +++ b/kernel/futex.c
> > > @@ -509,8 +509,6 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
> > > * The futex address must be "naturally" aligned.
> > > */
> > > key->both.offset = address % PAGE_SIZE;
> > > - if (unlikely((address % sizeof(u32)) != 0))
> > > - return -EINVAL;
> > > address -= key->both.offset;
> > >
> > > if (unlikely(!access_ok(rw, uaddr, sizeof(u32))))
> > > @@ -3525,6 +3523,11 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
> > > u32 val2 = 0;
> > > int cmd = op & FUTEX_CMD_MASK;
> > >
> > > + /* Only allow for aligned uaddr variables */
> > > + if (unlikely((unsigned long)uaddr % sizeof(u32) != 0 ||
> > > + (unsigned long)uaddr2 % sizeof(u32) != 0))
> >
> > Errm. How is that supposed to work? uaddr2 is not used by all opcodes.....
>
> So to explain the curious timing of the mails from Thomas and me: I told Thomas
> about the breakage over IRC and he found the likely bug! ;-)
My thinking had been that while uaddr2 is not use by all the op-codes,
it is not ever used as anything but a userspace address and when it
isn't used, it shouldn't be getting garbage passed in.
So if it's failing due to a uaddr2 not being aligned for an op-code that
doesn't use it... that would have to be on FUTEX_WAIT*, FUTEX_WAKE,
FUTEX_WAKE_BITSET, or FUTEX_*_PI.
If it's failing on distro boot, the PI ops are unlikely candidates.
I'm curious what a valid use of this would be, and why my own tests
didn't catch it.
I can move the uaddr2 test under a conditional on the cmd for now. Would
that be acceptable?
Then I can add a WARNON for the other ops for my own testing, but not
for upstream.
--
Darren Hart
VMware Open Source Technology Center
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-12-12 15:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-09 1:06 [PATCH] futex: Check for uaddr alignment as early as possible Darren Hart
2017-12-12 10:23 ` Thomas Gleixner
2017-12-12 10:31 ` Ingo Molnar
2017-12-12 15:57 ` Darren Hart
2017-12-12 10:23 ` Ingo Molnar
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).