linux-kernel - Re: Q: sys_futex() && timespec

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]

Date:	Fri, 25 Jun 2010 15:56:51 -0400
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Darren Hart <dvhltc@...ibm.com>
Cc:	Oleg Nesterov <oleg@...hat.com>, Ingo Molnar <mingo@...e.hu>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Thomas Gleixner <tglx@...utronix.de>,
	Andreas Schwab <schwab@...hat.com>,
	Danny Feng <dfeng@...hat.com>,
	Jakub Jelinek <jakub@...hat.com>,
	Ulrich Drepper <drepper@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: Q: sys_futex() && timespec_valid()

* Darren Hart (dvhltc@...ibm.com) wrote:
> On 06/25/2010 12:20 PM, Oleg Nesterov wrote:
>> Hello.
>>
>
> Hi Oleg,
>
>> Another stupid question about the trivial problem I am going to ask,
>> just to report the authoritative answer back to bugzilla. The problem
>> is, personally I am not sure we should/can add the user-visible change
>> required by glibc maintainers, and I am in no position to suggest them
>> to fix the user-space code instead.
>>
>> In short, glibc developers believe that sys_futex(ts) is buggy and
>> needs the fix to return -ETIMEDOUT instead of -EINVAL in case when
>> ts->tv_sec<  0 and the timeout is absolute.
>>
>
> Just a question of semantics I guess. Seems reasonable to me to call a  
> negative timeout invalid. However, I certainly don't feel strongly  
> enough about it to fight for it. Glibc is the principle user of  
> sys_futex(). While there are certainly other users out there (Mathieu  
> Desnoyers' Userspace RCU comes to mind), I doubt any of them depend on  
> -EINVAL for negative timeouts to function properly.

Userspace RCU does not use futex timeouts (the parameter is always NULL). So
this change/fix won't have any effect as far as urcu is concerned.

Thanks,

Mathieu

>
> Unless there is some good reason to object to breaking the API that I am  
> missing, I don't mind changing it to -ETIMEDOUT (although -EINVAL seems  
> more intuitive to me).
>
> --
> Darren "Little Fish" Hart
>
>> Ignoring the possible cleanups/microoptimizations, something like this:
>>
>> --- x/kernel/futex.c
>> +++ x/kernel/futex.c
>> @@ -2625,6 +2625,16 @@ SYSCALL_DEFINE6(futex, u32 __user *, uad
>>   		      cmd == FUTEX_WAIT_REQUEUE_PI)) {
>>   		if (copy_from_user(&ts, utime, sizeof(ts)) != 0)
>>   			return -EFAULT;
>> +
>> +		// absolute timeout
>> +		if (cmd != FUTEX_WAIT) {
>> +			if (ts->tv_nsec>= NSEC_PER_SEC)
>> +				return -EINVAL;
>> +			if (ts->tv_sec<  0)
>> +				return -ETIMEDOUT;
>> +		}
>> +
>> +
>>   		if (!timespec_valid(&ts))
>>   			return -EINVAL;
>>
>> ------------------------------------------------------------------------
>>
>> Otherwise, pthread_rwlock_timedwrlock(ts) hangs spinning in user-space
>> forever if ts->tv_sec<  0.
>>
>> To clarify: this depends on libc version and arch.
>>
>> This happens because pthread_rwlock_timedwrlock(rwlock, ts) on x86_64
>> roughly does:
>>
>> 	for (;;) {
>> 		if (fast_path_succeeds(rwlock))
>> 			return 0;
>>
>> 		if (ts->tv_nsec>= NSEC_PER_SEC)
>> 			return EINVAL;
>>
>> 		errcode = sys_futex(FUTEX_WAIT_BITSET_PRIVATE, ts);
>> 		if (errcode == ETIMEDOUT)
>> 			return ETIMEDOUT;
>> 	}
>>
>> and since the kernel return EINVAL due to !timespec_valid(ts), the
>> code above loops forever.
>>
>> (btw, we have same problem with EFAULT, and this is considered as
>>   a caller's problem).
>>
>> IOW, pthread_rwlock_timedwrlock() assumes that in this case
>> sys_futex() can return nothing interesting except 0 or ETIMEDOUT.
>> I guess pthread_rwlock_timedwrlock() is not alone, but I didn't check.
>>
>>
>>
>> So, the question: do you think we can change sys_futex() to make
>> glibc happy?
>>
>> Or, do you think it is user-space who should check tv_sec<  0 if
>> it wants ETIMEDOUT with the negative timeout ?
>>
>> Thanks,
>>
>> Oleg.
>>
>
>
> -- 
> Darren Hart
> IBM Linux Technology Center
> Real-Time Linux Team

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/