[PATCH] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT

[PATCH] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT

[Darren Hart]

The FLAGS_HAS_TIMEOUT flag was not getting set, causing the restart_block to
restart futex_wait() without a timeout after a signal.

Signed-off-by: Darren Hart <dvhart@linux.intel.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: Eric Dumazet <eric.dumazet@gmail.com>
CC: John Kacur <jkacur@redhat.com>
---
 kernel/futex.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index bda4157..6eac6b6 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2589,6 +2589,9 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 			return -ENOSYS;
 	}
 
+	if (timeout)
+		flags |= FLAGS_HAS_TIMEOUT;
+
 	switch (cmd) {
 	case FUTEX_WAIT:
 		val3 = FUTEX_BITSET_MATCH_ANY;
-- 
1.7.1

Re: [PATCH] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT

[Eric Dumazet]

Le jeudi 14 avril 2011 à 09:58 -0700, Darren Hart a écrit :
> The FLAGS_HAS_TIMEOUT flag was not getting set, causing the restart_block to
> restart futex_wait() without a timeout after a signal.
> 
> Signed-off-by: Darren Hart <dvhart@linux.intel.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: Eric Dumazet <eric.dumazet@gmail.com>
> CC: John Kacur <jkacur@redhat.com>
> ---
>  kernel/futex.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index bda4157..6eac6b6 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -2589,6 +2589,9 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
>  			return -ENOSYS;
>  	}
>  
> +	if (timeout)
> +		flags |= FLAGS_HAS_TIMEOUT;
> +
>  	switch (cmd) {
>  	case FUTEX_WAIT:
>  		val3 = FUTEX_BITSET_MATCH_ANY;



Hmm, could you please add in the changelog the commit id introducing the
bug ?

I suspect its b41277dc7a18ee332d, added in 2.6.38 ?

Thanks

[PATCH V2] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT

[Darren Hart]

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. It makes more sense to set the flag earlier during
do_futex() where the other flags are set, although futex_wait is the only user
of FLAGS_HAS_TIMEOUT as it is the only op using a relative timeout (an
unfortunately preexisting condition).

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=32922

V2: Added references to commit message.

Signed-off-by: Darren Hart <dvhart@linux.intel.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: Eric Dumazet <eric.dumazet@gmail.com>
CC: John Kacur <jkacur@redhat.com>
---
 kernel/futex.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index bda4157..6eac6b6 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2589,6 +2589,9 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 			return -ENOSYS;
 	}
 
+	if (timeout)
+		flags |= FLAGS_HAS_TIMEOUT;
+
 	switch (cmd) {
 	case FUTEX_WAIT:
 		val3 = FUTEX_BITSET_MATCH_ANY;
-- 
1.7.1

Re: [PATCH V2] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT

[Eric Dumazet]

Le jeudi 14 avril 2011 à 11:45 -0700, Darren Hart a écrit :
> 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. It makes more sense to set the flag earlier during
> do_futex() where the other flags are set, although futex_wait is the only user
> of FLAGS_HAS_TIMEOUT as it is the only op using a relative timeout (an
> unfortunately preexisting condition).
> 
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=32922
> 
> V2: Added references to commit message.
> 
> Signed-off-by: Darren Hart <dvhart@linux.intel.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: Eric Dumazet <eric.dumazet@gmail.com>
> CC: John Kacur <jkacur@redhat.com>
> ---
>  kernel/futex.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index bda4157..6eac6b6 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -2589,6 +2589,9 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
>  			return -ENOSYS;
>  	}
>  
> +	if (timeout)
> +		flags |= FLAGS_HAS_TIMEOUT;
> +
>  	switch (cmd) {
>  	case FUTEX_WAIT:
>  		val3 = FUTEX_BITSET_MATCH_ANY;


I dont agree with your fix. You add yet another test in futex fastpath.

Prior commit  b41277dc7a18ee332d, this was done only in case of restart.

Could we instead use 

diff --git a/kernel/futex.c b/kernel/futex.c
index dfb924f..fe28dc2 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;
 

Re: [PATCH V2] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT

[Darren Hart]

On 04/14/2011 11:56 AM, Eric Dumazet wrote:
> Le jeudi 14 avril 2011 à 11:45 -0700, Darren Hart a écrit :
>> 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. It makes more sense to set the flag earlier during
>> do_futex() where the other flags are set, although futex_wait is the only user
>> of FLAGS_HAS_TIMEOUT as it is the only op using a relative timeout (an
>> unfortunately preexisting condition).
>>
>> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=32922
>>
>> V2: Added references to commit message.
>>
>> Signed-off-by: Darren Hart <dvhart@linux.intel.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: Eric Dumazet <eric.dumazet@gmail.com>
>> CC: John Kacur <jkacur@redhat.com>
>> ---
>>  kernel/futex.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/futex.c b/kernel/futex.c
>> index bda4157..6eac6b6 100644
>> --- a/kernel/futex.c
>> +++ b/kernel/futex.c
>> @@ -2589,6 +2589,9 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
>>  			return -ENOSYS;
>>  	}
>>  
>> +	if (timeout)
>> +		flags |= FLAGS_HAS_TIMEOUT;
>> +
>>  	switch (cmd) {
>>  	case FUTEX_WAIT:
>>  		val3 = FUTEX_BITSET_MATCH_ANY;
> 
> 
> I dont agree with your fix. You add yet another test in futex fastpath.

I would say anything calling SYS_FUTEX is the futex slow path. The fast
path is cmpxchg in user space.

> 
> Prior commit  b41277dc7a18ee332d, this was done only in case of restart.

It was. My thinking was that it was inconsistent to have the
FLAGS_HAS_TIMEOUT only available if a signal was received and a restart
was required. This is the only place it is currently needed, but the
inconsistency concerns me.

> 
> Could we instead use 
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index dfb924f..fe28dc2 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;

How about the following, it reuses an existing if block and ensure the
FLAGS_HAS_TIMEOUT is always set if a timeout is used. It means the
FLAG_HAS_TIMEOUT is not available in the other futex_* routines with
timeouts (futex_lock_pi and futex_wait_requeue_pi), but they use absolute
timeouts and don't need it for restart - I can agree to that, although
I'm not keen on FLAG_HAS_TIMEOUT not being set whenever timeout is. That
could be added in the same way to the other functions if needed in the
future.


diff --git a/kernel/futex.c b/kernel/futex.c
index 6eac6b6..0a7eefd 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1846,6 +1846,7 @@ static int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
 		hrtimer_init_sleeper(to, current);
 		hrtimer_set_expires_range_ns(&to->timer, *abs_time,
 					     current->timer_slack_ns);
+		flags |= FLAGS_HAS_TIMEOUT;
 	}
 
 retry:



-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH V2] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT

[Eric Dumazet]

Le jeudi 14 avril 2011 à 12:11 -0700, Darren Hart a écrit :

> I would say anything calling SYS_FUTEX is the futex slow path. The fast
> path is cmpxchg in user space.
> 

Thats not a good reason to make it slower than necessary...

> It was. My thinking was that it was inconsistent to have the
> FLAGS_HAS_TIMEOUT only available if a signal was received and a restart
> was required. This is the only place it is currently needed, but the
> inconsistency concerns me.
> 

I dont call this inconsistency, but right place for the code.

> How about the following, it reuses an existing if block and ensure the
> FLAGS_HAS_TIMEOUT is always set if a timeout is used. It means the
> FLAG_HAS_TIMEOUT is not available in the other futex_* routines with
> timeouts (futex_lock_pi and futex_wait_requeue_pi), but they use absolute
> timeouts and don't need it for restart - I can agree to that, although
> I'm not keen on FLAG_HAS_TIMEOUT not being set whenever timeout is. That
> could be added in the same way to the other functions if needed in the
> future.

I dont understand why you insist setting in fast path a flag that is
useless, unless we hit restart logic [ What I call the slow path in
futex syscall ]

It seems more natural and efficient to me to go back to previous code.

Maybe rename FLAG_HAS_TIMEOUT to FLAG_HAS_TIMEOUT_ON_RESTART if you
want, to make clear what is the meaning of this flag.

Now if you have plans to use this flag in futex code, outside of restart
logic, please share them with us :)

Re: [PATCH V2] futex: set FLAGS_HAS_TIMEOUT during demux for FUTEX_WAIT

[Darren Hart]

On 04/14/2011 12:48 PM, Eric Dumazet wrote:
> Le jeudi 14 avril 2011 à 12:11 -0700, Darren Hart a écrit :
> 
>> I would say anything calling SYS_FUTEX is the futex slow path. The fast
>> path is cmpxchg in user space.
>>
> 
> Thats not a good reason to make it slower than necessary...
> 
>> It was. My thinking was that it was inconsistent to have the
>> FLAGS_HAS_TIMEOUT only available if a signal was received and a restart
>> was required. This is the only place it is currently needed, but the
>> inconsistency concerns me.
>>
> 
> I dont call this inconsistency, but right place for the code.
> 
>> How about the following, it reuses an existing if block and ensure the
>> FLAGS_HAS_TIMEOUT is always set if a timeout is used. It means the
>> FLAG_HAS_TIMEOUT is not available in the other futex_* routines with
>> timeouts (futex_lock_pi and futex_wait_requeue_pi), but they use absolute
>> timeouts and don't need it for restart - I can agree to that, although
>> I'm not keen on FLAG_HAS_TIMEOUT not being set whenever timeout is. That
>> could be added in the same way to the other functions if needed in the
>> future.
> 
> I dont understand why you insist setting in fast path a flag that is
> useless, unless we hit restart logic [ What I call the slow path in
> futex syscall ]

I'm not particularly attached to this approach, I just felt it made more
sense. Your initial objection was to the test in the do_futex(), so I
avoided the test by moving it into futex_wait().

The addition of an |= to an existing test block didn't seem significant
to me in this path. But, it isn't important enough to me to argue the point.

> 
> It seems more natural and efficient to me to go back to previous code.
> 
> Maybe rename FLAG_HAS_TIMEOUT to FLAG_HAS_TIMEOUT_ON_RESTART if you
> want, to make clear what is the meaning of this flag.
> 
> Now if you have plans to use this flag in futex code, outside of restart
> logic, please share them with us :)

Nope, no plans, and there is value in simply restoring the original
behavior, especially as this should go to stable as well. I've
resubmitted the patch with the "restart-block only approach" and
included "stable".

Thanks for the feedback,

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel