linux-kernel - Re: [PATCH] wait: fix false timeouts when using wait_event

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] [day] [month] [year] [list]

Date:	Thu, 6 Jun 2013 20:47:03 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	Imre Deak <imre.deak@...el.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Dave Jones <davej@...hat.com>,
	David Howells <dhowells@...hat.com>,
	Jens Axboe <axboe@...nel.dk>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Lukas Czerner <lczerner@...hat.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] wait: fix false timeouts when using
	wait_event_timeout()

Hi Tejun,

On 06/05, Tejun Heo wrote:
>
> Heh, yeah, this looks good to me and a lot better than trying to do
> the same thing over and over again and ending up with subtle
> differences.

Yes, this is the goal. Of course we could fix wait_event_timout() and
wait_event_interruptible_timeout() without unification, but this would
add more copy-and-paste.

> > Hmm. I compiled the kernel with the patch below,
> >
> > 		$ size vmlinux
> > 		   text	   data	    bss	    dec	    hex	filename
> > 	-	4978601	2935080	10104832	18018513	112f0d1	vmlinux
> > 	+	4977769	2930984	10104832	18013585	112dd91	vmlinux
>
> Nice.  Provided you went over assembly outputs of at least some
> combinations,

I did, and that is why I am surprized by the numbers above.

Yes, gcc optimizes out the unwanted checks but the generated code
differs, of course. And sometimes the difference looks "random" to me.

Simplest example:

	extern wait_queue_head_t WQ;
	extern int COND;

	void we(void)
	{
		wait_event(WQ, COND);
	}

before:
	we:
		pushq	%rbp	#
		movq	%rsp, %rbp	#,
		pushq	%rbx	#
		subq	$56, %rsp	#,
		call	mcount
		movl	COND(%rip), %edx	# COND,
		testl	%edx, %edx	#
		jne	.L5	#,
		leaq	-64(%rbp), %rbx	#, tmp66
		movq	$0, -64(%rbp)	#, __wait
		movq	$autoremove_wake_function, -48(%rbp)	#, __wait.func
	#APP
	# 14 "/home/tmp/K/misc/arch/x86/include/asm/current.h" 1
		movq %gs:current_task,%rax	#, pfo_ret__
	# 0 "" 2
	#NO_APP
		movq	%rax, -56(%rbp)	# pfo_ret__, __wait.private
		leaq	24(%rbx), %rax	#, tmp61
		movq	%rax, -40(%rbp)	# tmp61, __wait.task_list.next
		movq	%rax, -32(%rbp)	# tmp61, __wait.task_list.prev
		.p2align 4,,10
		.p2align 3
	.L4:
		movl	$2, %edx	#,
		movq	%rbx, %rsi	# tmp66,
		movq	$WQ, %rdi	#,
		call	prepare_to_wait	#
		movl	COND(%rip), %eax	# COND,
		testl	%eax, %eax	#
		jne	.L3	#,
		call	schedule	#
		jmp	.L4	#
		.p2align 4,,10
		.p2align 3
	.L3:
		movq	%rbx, %rsi	# tmp66,
		movq	$WQ, %rdi	#,
		call	finish_wait	#
	.L5:
		addq	$56, %rsp	#,
		popq	%rbx	#
		leave
		ret

after:
	we:
		pushq	%rbp	#
		movq	%rsp, %rbp	#,
		pushq	%rbx	#
		subq	$56, %rsp	#,
		call	mcount
		movl	COND(%rip), %edx	# COND,
		testl	%edx, %edx	#
		jne	.L5	#,
		leaq	-64(%rbp), %rbx	#, tmp66
		movq	$0, -64(%rbp)	#, __wait
		movq	$autoremove_wake_function, -48(%rbp)	#, __wait.func
	#APP
	# 14 "/home/tmp/K/misc/arch/x86/include/asm/current.h" 1
		movq %gs:current_task,%rax	#, pfo_ret__
	# 0 "" 2
	#NO_APP
		movq	%rax, -56(%rbp)	# pfo_ret__, __wait.private
		leaq	24(%rbx), %rax	#, tmp61
		movq	%rax, -40(%rbp)	# tmp61, __wait.task_list.next
		movq	%rax, -32(%rbp)	# tmp61, __wait.task_list.prev
		.p2align 4,,10
		.p2align 3
	.L4:
		movl	$2, %edx	#,
		movq	%rbx, %rsi	# tmp66,
		movq	$WQ, %rdi	#,
		call	prepare_to_wait	#
		movl	COND(%rip), %eax	# COND,
		testl	%eax, %eax	#
		je	.L3	#,
		movq	%rbx, %rsi	# tmp66,
		movq	$WQ, %rdi	#,
		call	finish_wait	#
		jmp	.L5	#
		.p2align 4,,10
		.p2align 3
	.L3:
		call	schedule	#
		.p2align 4,,8
		.p2align 3
		jmp	.L4	#
		.p2align 4,,10
		.p2align 3
	.L5:
		addq	$56, %rsp	#,
		popq	%rbx	#
		leave
		.p2align 4,,4
		.p2align 3
		ret

As you can see, with this patch gcc generates a bit more code. But
only because reorderes finish_wait() and inserts the additional nops,
otherwise code the same. I think this difference is not "reliable"
and can be ignored.

But, the code like "return wait_even_timeout(true, non_const_timeout)"
really generates more code and this is correct: the patch tries to fix
the bug (at least I think this is a bug) and the additional code ensures
that __ret != 0.

>  Reviewed-by: Tejun Heo <tj@...nel.org>

Thanks! I'll send this patch soon.

Oleg.

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