* [PATCH V5] futex: set FLAGS_HAS_TIMEOUT during futex_wait restart setup
@ 2011-04-14 22:41 Darren Hart
2011-04-15 8:33 ` Thomas Gleixner
0 siblings, 1 reply; 5+ messages in thread
From: Darren Hart @ 2011-04-14 22:41 UTC (permalink / raw)
To: Linux Kernel Mailing List
Cc: Darren Hart, Eric Dumazet, Thomas Gleixner, Peter Zijlstra,
Ingo Molnar, John Kacur, stable
The FLAGS_HAS_TIMEOUT flag was not getting set, causing the restart_block to
restart futex_wait() without a timeout after a signal.
Commit b41277dc7a18ee332d in 2.6.38 introduced the regression by accidentally
removing the the FLAGS_HAS_TIMEOUT assignment from futex_wait() during the setup
of the restart block. Restore the originaly behavior.
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=32922
V2: Added references to commit message.
V3: Set flag during restart block instead of do_futex()
V4: Correct stupid order of assignment mistake pointed out by Eric
V5: Correct subject to match implementation, correct stable submission
Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Reported-by: Tim Smith <tsmith201104@yahoo.com>
Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: John Kacur <jkacur@redhat.com>
Cc: stable@kernel.org
---
kernel/futex.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index bda4157..abd5324 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1886,7 +1886,7 @@ retry:
restart->futex.val = val;
restart->futex.time = abs_time->tv64;
restart->futex.bitset = bitset;
- restart->futex.flags = flags;
+ restart->futex.flags = flags | FLAGS_HAS_TIMEOUT;
ret = -ERESTART_RESTARTBLOCK;
--
1.7.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH V5] futex: set FLAGS_HAS_TIMEOUT during futex_wait restart setup
2011-04-14 22:41 [PATCH V5] futex: set FLAGS_HAS_TIMEOUT during futex_wait restart setup Darren Hart
@ 2011-04-15 8:33 ` Thomas Gleixner
2011-04-15 9:27 ` Eric Dumazet
2011-04-15 14:07 ` Darren Hart
0 siblings, 2 replies; 5+ messages in thread
From: Thomas Gleixner @ 2011-04-15 8:33 UTC (permalink / raw)
To: Darren Hart
Cc: Linux Kernel Mailing List, Eric Dumazet, Peter Zijlstra,
Ingo Molnar, John Kacur, stable
On Thu, 14 Apr 2011, Darren Hart wrote:
> The FLAGS_HAS_TIMEOUT flag was not getting set, causing the restart_block to
> restart futex_wait() without a timeout after a signal.
>
> Commit b41277dc7a18ee332d in 2.6.38 introduced the regression by accidentally
> removing the the FLAGS_HAS_TIMEOUT assignment from futex_wait() during the setup
> of the restart block. Restore the originaly behavior.
>
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=32922
>
> V2: Added references to commit message.
> V3: Set flag during restart block instead of do_futex()
> V4: Correct stupid order of assignment mistake pointed out by Eric
> V5: Correct subject to match implementation, correct stable submission
>
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Reported-by: Tim Smith <tsmith201104@yahoo.com>
> Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: John Kacur <jkacur@redhat.com>
> Cc: stable@kernel.org
> ---
> kernel/futex.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index bda4157..abd5324 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1886,7 +1886,7 @@ retry:
> restart->futex.val = val;
> restart->futex.time = abs_time->tv64;
> restart->futex.bitset = bitset;
> - restart->futex.flags = flags;
> + restart->futex.flags = flags | FLAGS_HAS_TIMEOUT;
We only get here when a timeout is pending. So why don't we just do
the obvious:
--- linux-2.6.orig/kernel/futex.c
+++ linux-2.6/kernel/futex.c
@@ -1902,16 +1902,13 @@ out:
static long futex_wait_restart(struct restart_block *restart)
{
u32 __user *uaddr = restart->futex.uaddr;
- ktime_t t, *tp = NULL;
+ ktime_t t;
- if (restart->futex.flags & FLAGS_HAS_TIMEOUT) {
- t.tv64 = restart->futex.time;
- tp = &t;
- }
+ t.tv64 = restart->futex.time;
restart->fn = do_no_restart_syscall;
return (long)futex_wait(uaddr, restart->futex.flags,
- restart->futex.val, tp, restart->futex.bitset);
+ restart->futex.val, &t, restart->futex.bitset);
}
Thanks,
tglx
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V5] futex: set FLAGS_HAS_TIMEOUT during futex_wait restart setup
2011-04-15 8:33 ` Thomas Gleixner
@ 2011-04-15 9:27 ` Eric Dumazet
2011-04-15 14:07 ` Darren Hart
1 sibling, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2011-04-15 9:27 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Darren Hart, Linux Kernel Mailing List, Peter Zijlstra,
Ingo Molnar, John Kacur, stable
Le vendredi 15 avril 2011 à 10:33 +0200, Thomas Gleixner a écrit :
> On Thu, 14 Apr 2011, Darren Hart wrote:
>
> > The FLAGS_HAS_TIMEOUT flag was not getting set, causing the restart_block to
> > restart futex_wait() without a timeout after a signal.
> >
> > Commit b41277dc7a18ee332d in 2.6.38 introduced the regression by accidentally
> > removing the the FLAGS_HAS_TIMEOUT assignment from futex_wait() during the setup
> > of the restart block. Restore the originaly behavior.
> >
> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=32922
> >
> > V2: Added references to commit message.
> > V3: Set flag during restart block instead of do_futex()
> > V4: Correct stupid order of assignment mistake pointed out by Eric
> > V5: Correct subject to match implementation, correct stable submission
> >
> > Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > Reported-by: Tim Smith <tsmith201104@yahoo.com>
> > Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: John Kacur <jkacur@redhat.com>
> > Cc: stable@kernel.org
> > ---
> > kernel/futex.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/kernel/futex.c b/kernel/futex.c
> > index bda4157..abd5324 100644
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -1886,7 +1886,7 @@ retry:
> > restart->futex.val = val;
> > restart->futex.time = abs_time->tv64;
> > restart->futex.bitset = bitset;
> > - restart->futex.flags = flags;
> > + restart->futex.flags = flags | FLAGS_HAS_TIMEOUT;
>
> We only get here when a timeout is pending. So why don't we just do
> the obvious:
>
> --- linux-2.6.orig/kernel/futex.c
> +++ linux-2.6/kernel/futex.c
> @@ -1902,16 +1902,13 @@ out:
> static long futex_wait_restart(struct restart_block *restart)
> {
> u32 __user *uaddr = restart->futex.uaddr;
> - ktime_t t, *tp = NULL;
> + ktime_t t;
>
> - if (restart->futex.flags & FLAGS_HAS_TIMEOUT) {
> - t.tv64 = restart->futex.time;
> - tp = &t;
> - }
> + t.tv64 = restart->futex.time;
> restart->fn = do_no_restart_syscall;
>
> return (long)futex_wait(uaddr, restart->futex.flags,
> - restart->futex.val, tp, restart->futex.bitset);
> + restart->futex.val, &t, restart->futex.bitset);
> }
>
Because its not suitable for stable ?
This is basically reverting a72188d8a64ebe74, so why do you keep
FUTEX_HAS_TIMEOUT definition ?
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V5] futex: set FLAGS_HAS_TIMEOUT during futex_wait restart setup
2011-04-15 8:33 ` Thomas Gleixner
2011-04-15 9:27 ` Eric Dumazet
@ 2011-04-15 14:07 ` Darren Hart
2011-04-15 14:30 ` Thomas Gleixner
1 sibling, 1 reply; 5+ messages in thread
From: Darren Hart @ 2011-04-15 14:07 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Linux Kernel Mailing List, Eric Dumazet, Peter Zijlstra,
Ingo Molnar, John Kacur, stable
On 04/15/2011 01:33 AM, Thomas Gleixner wrote:
> On Thu, 14 Apr 2011, Darren Hart wrote:
>
>> The FLAGS_HAS_TIMEOUT flag was not getting set, causing the restart_block to
>> restart futex_wait() without a timeout after a signal.
>>
>> Commit b41277dc7a18ee332d in 2.6.38 introduced the regression by accidentally
>> removing the the FLAGS_HAS_TIMEOUT assignment from futex_wait() during the setup
>> of the restart block. Restore the originaly behavior.
>>
>> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=32922
>>
>> V2: Added references to commit message.
>> V3: Set flag during restart block instead of do_futex()
>> V4: Correct stupid order of assignment mistake pointed out by Eric
>> V5: Correct subject to match implementation, correct stable submission
>>
>> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> Reported-by: Tim Smith <tsmith201104@yahoo.com>
>> Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@elte.hu>
>> Cc: John Kacur <jkacur@redhat.com>
>> Cc: stable@kernel.org
>> ---
>> kernel/futex.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/futex.c b/kernel/futex.c
>> index bda4157..abd5324 100644
>> --- a/kernel/futex.c
>> +++ b/kernel/futex.c
>> @@ -1886,7 +1886,7 @@ retry:
>> restart->futex.val = val;
>> restart->futex.time = abs_time->tv64;
>> restart->futex.bitset = bitset;
>> - restart->futex.flags = flags;
>> + restart->futex.flags = flags | FLAGS_HAS_TIMEOUT;
>
> We only get here when a timeout is pending. So why don't we just do
> the obvious:
>
> --- linux-2.6.orig/kernel/futex.c
> +++ linux-2.6/kernel/futex.c
> @@ -1902,16 +1902,13 @@ out:
> static long futex_wait_restart(struct restart_block *restart)
> {
> u32 __user *uaddr = restart->futex.uaddr;
> - ktime_t t, *tp = NULL;
> + ktime_t t;
>
> - if (restart->futex.flags & FLAGS_HAS_TIMEOUT) {
> - t.tv64 = restart->futex.time;
> - tp = &t;
> - }
> + t.tv64 = restart->futex.time;
> restart->fn = do_no_restart_syscall;
>
> return (long)futex_wait(uaddr, restart->futex.flags,
> - restart->futex.val, tp, restart->futex.bitset);
> + restart->futex.val, &t, restart->futex.bitset);
> }
I believe I asked you the same question when I was adding the
FLAGS_HAS_TIMEOUT. :-) The problem is distinguishing between no timeout
and an expired timer. The above always passes a non-null address for
abs_time to futex_wait(), which will then always schedule a timer.
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V5] futex: set FLAGS_HAS_TIMEOUT during futex_wait restart setup
2011-04-15 14:07 ` Darren Hart
@ 2011-04-15 14:30 ` Thomas Gleixner
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Gleixner @ 2011-04-15 14:30 UTC (permalink / raw)
To: Darren Hart
Cc: Linux Kernel Mailing List, Eric Dumazet, Peter Zijlstra,
Ingo Molnar, John Kacur, stable
On Fri, 15 Apr 2011, Darren Hart wrote:
> I believe I asked you the same question when I was adding the
> FLAGS_HAS_TIMEOUT. :-) The problem is distinguishing between no timeout
> and an expired timer. The above always passes a non-null address for
> abs_time to futex_wait(), which will then always schedule a timer.
Duh, correct.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-04-15 14:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-14 22:41 [PATCH V5] futex: set FLAGS_HAS_TIMEOUT during futex_wait restart setup Darren Hart
2011-04-15 8:33 ` Thomas Gleixner
2011-04-15 9:27 ` Eric Dumazet
2011-04-15 14:07 ` Darren Hart
2011-04-15 14:30 ` Thomas Gleixner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.