Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

["Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>]

Hi Darren,

On 06/23/2016 06:48 AM, Darren Hart wrote:
> On Mon, Jun 20, 2016 at 04:26:52PM +0200, Matthieu CASTET wrote:
>> Hi,
>>
>> the commit 337f13046ff03717a9e99675284a817527440a49 is saying that it
>> change to syscall to an equivalent to FUTEX_WAIT_BITSET |
>> FUTEX_CLOCK_REALTIME with a bitset of FUTEX_BITSET_MATCH_ANY.
>>
>> It seems wrong to me, because in case of FUTEX_WAIT, in
>> "SYSCALL_DEFINE6(futex", we convert relative timeout to absolute
>> timeout [1].
>>
>> So FUTEX_CLOCK_REALTIME | FUTEX_WAIT is expecting a relative timeout
>> when FUTEX_WAIT_BITSET take an absolute timeout.
>>
>> To make it work you have to use something like the (untested) attached
>> patch.
>
> +Eric Dumazet
>
> Thanks for reporting Matthieu,
>
> FUTEX_WAIT traditionally used a relative timeout with CLOCK_MONOTONIC while
> FUTEX_WAIT_BITSET could use either ??? based on the FUTEX_CLOCK_ flag used. The
> man page is not particularly clear on this:
>
> http://man7.org/linux/man-pages/man2/futex.2.html
>
> "
> The FUTEX_WAIT_BITSET operation also interprets the timeout argument
> differently from FUTEX_WAIT.  See the discussion of FUTEX_CLOCK_REALTIME,
> above.
> "
>
> Matthew Kerrisk:
> I think this language could be removed now that we support the
> FUTEX_CLOCK_REALTIME flag for both futex ops.

Done.

> As for the intended behavior of the FUTEX_CLOCK_REALTIME flag:
>
>
> FUTEX_CLOCK_REALTIME (since Linux 2.6.28)
>     This option bit can be employed only with the FUTEX_WAIT_BITSET,
>     FUTEX_WAIT_REQUEUE_PI, and FUTEX_WAIT (since Linux 4.5) operations.
>
> (NOTE: FUTEX_WAIT was recently added after the patch in question here)
>
>     If this option is set, the kernel treats timeout as an absolute time based
>     on CLOCK_REALTIME.
>
>     If this option is not set, the kernel treats timeout as a relative time,
>     measured against the CLOCK_MONOTONIC clock.
>
>
> This supports your argument Matthieu. The assumption of a relative timeout for
> FUTEX_WAIT in SYSCALL_DEFINE6 needs to be updated to account for FUTEX_WAIT now
> honoring the FUTEX_CLOCK_REALTIME flag, which treats the timeout as absolute.
>
> However, I don't think the patch below is correct. The existing logic
> determines the type of timeout based on the futex_op when it should instead
> determine the type of timeout based on the FUTEX_CLOCK_REALTIME flag.
>
> My reading of the man page is that FUTEX_WAIT_BITSET abides by the timeout
> interpretation defined by the FUTEX_CLOCK_REALTIME attribute, so
> SYSCALL_DEFINE6 was misbehaving for FUTEX_WAIT|FUTEX_CLOCK_REALTIME (where the
> timeout should have been treated as absolute) as well as for
> FUTEX_WAIT_BITSET|FUTEX_CLOCK_MONOTONIC (where the timeout should have been
> treated as relative).
>
> Consider the following:
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 33664f7..fa2af29 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -3230,7 +3230,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
>  			return -EINVAL;
>
>  		t = timespec_to_ktime(ts);
> -		if (cmd == FUTEX_WAIT)
> +		if (!(cmd & FUTEX_CLOCK_REALTIME))
>  			t = ktime_add_safe(ktime_get(), t);
>  		tp = &t;
>  	}
>
> The concern for me is whether the code is incorrect, or if the man page is
> incorrect. Does existing userspace code expect the FUTEX_WAIT_BITSET op to
> always use an absolute timeout, regardless of the CLOCK used?

So, there clearly seem to be some things broken in the man page. See the
reply I sent to tglx.

Cheers,

Michael


>> [1]
>>         if (cmd == FUTEX_WAIT)
>>             t = ktime_add_safe(ktime_get(), t);
>
>> diff --git a/kernel/futex.c b/kernel/futex.c
>> index 33664f7..4bee915 100644
>> --- a/kernel/futex.c
>> +++ b/kernel/futex.c
>> @@ -3230,7 +3230,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
>>  			return -EINVAL;
>>
>>  		t = timespec_to_ktime(ts);
>> -		if (cmd == FUTEX_WAIT)
>> +		if (cmd == FUTEX_WAIT && !(op & FUTEX_CLOCK_REALTIME))
>>  			t = ktime_add_safe(ktime_get(), t);
>>  		tp = &t;
>>  	}
>
>


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/