linux-kernel - Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.11.1606230913570.5839@nanos>
Date:	Thu, 23 Jun 2016 09:18:42 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Darren Hart <dvhart@...radead.org>
cc:	Matthieu CASTET <matthieu.castet@...rot.com>,
	linux-kernel@...r.kernel.org,
	Michael Kerrisk <mtk.manpages@...il.com>,
	Darren Hart <dvhart@...ux.intel.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Davidlohr Bueso <dave@...olabs.net>,
	Eric Dumazet <dada1@...mosbay.com>
Subject: Re: futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op

On Wed, 22 Jun 2016, Darren Hart wrote:
> 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.

No.
 
> 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);

That breaks LOCK_PI, REQUEUE_PI and FUTEX_WAIT_BITSET

> 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?

FUTEX_WAIT_BITSET, LOCK_PI and REQUEUE_PI always expect absolute time in
CLOCK_REALTIME independent of the CLOCK_REALTIME flag.

The flag was explicitely added to allow FUTEX_WAIT to hand in absolute time.

> > 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;
> >  	}

So this patch is correct and if the man page is unclear about it then we need
to fix that.

Thanks,

	tglx