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 <[email protected]>
Signed-off-by: Eric Dumazet <[email protected]>
Reported-by: Tim Smith <[email protected]>
Reported-by: Torsten Hilbrich <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: John Kacur <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
> Signed-off-by: Eric Dumazet <[email protected]>
> Reported-by: Tim Smith <[email protected]>
> Reported-by: Torsten Hilbrich <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: John Kacur <[email protected]>
> Cc: [email protected]
> ---
> 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
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 <[email protected]>
> > Signed-off-by: Eric Dumazet <[email protected]>
> > Reported-by: Tim Smith <[email protected]>
> > Reported-by: Torsten Hilbrich <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: John Kacur <[email protected]>
> > Cc: [email protected]
> > ---
> > 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 ?
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 <[email protected]>
>> Signed-off-by: Eric Dumazet <[email protected]>
>> Reported-by: Tim Smith <[email protected]>
>> Reported-by: Torsten Hilbrich <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Peter Zijlstra <[email protected]>
>> Cc: Ingo Molnar <[email protected]>
>> Cc: John Kacur <[email protected]>
>> Cc: [email protected]
>> ---
>> 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
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.