linux-kernel - Re: [PATCH] futex: Fix ZERO

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: <1261647565.4937.177.camel@laptop>
Date:	Thu, 24 Dec 2009 10:39:25 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
Cc:	Hugh Dickins <hugh.dickins@...cali.co.uk>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Nick Piggin <npiggin@...e.de>, Ingo Molnar <mingo@...e.hu>,
	LKML <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Darren Hart <dvhltc@...ibm.com>
Subject: Re: [PATCH] futex: Fix ZERO_PAGE cause infinite loop

Added tglx and dvhart to the CC.

On Thu, 2009-12-24 at 18:29 +0900, KOSAKI Motohiro wrote:
> This patch need both futex and zero-page developer's ack.
> ==========================================================
> 
> commit a13ea5b7 (mm: reinstate ZERO_PAGE) made the unfortunate regression.
> following test program never finish and waste 100% cpu time.
> 
> At the making commit 38d47c1b7 (rely on get_user_pages() for shared
> futexes). There isn't zero page in linux kernel. then, futex developers
> thought gup retry is safe. but we reinstated zero page later...
> 
> This patch fixes it.
> 
> futex-zero.c
> ---------------------------------------------------------------------
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <sys/mman.h>
>  #include <syscall.h>
>  #include <sys/time.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <errno.h>
>  #include <linux/futex.h>
>  #include <pthread.h>
> 
> int main(int argc, char **argv)
> {
>         long page_size;
>         int ret;
>         void *buf;
> 
>         page_size = sysconf(_SC_PAGESIZE);
> 
>         buf = mmap(NULL, page_size, PROT_READ|PROT_WRITE,
> 			MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
>         if (buf == (void *)-1) {
>                 perror("mmap error.\n");
>                 exit(1);
>         }
> 
>         fprintf(stderr, "futex wait\n");
>         ret = syscall( SYS_futex, buf, FUTEX_WAIT, 1, NULL, NULL, NULL);
>         if (ret != 0 && errno != EWOULDBLOCK) {
>                 perror("futex error.\n");
>                 exit(1);
>         }
>         fprintf(stderr, "futex_wait: ret = %d, errno = %d\n", ret, errno);
> 
>         return 0;
> }
> ---------------------------------------------------------------------
> 
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
> Cc: Hugh Dickins <hugh.dickins@...cali.co.uk>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
> Cc: Nick Piggin <npiggin@...e.de>
> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Cc: Ingo Molnar <mingo@...e.hu>
> ---
>  include/linux/mm.h |   16 ++++++++++++++++
>  kernel/futex.c     |    3 +++
>  mm/memory.c        |   14 --------------
>  3 files changed, 19 insertions(+), 14 deletions(-)


> diff --git a/kernel/futex.c b/kernel/futex.c
> index 8e3c3ff..79b89cc 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -254,6 +254,8 @@ again:
>  
>  	page = compound_head(page);
>  	lock_page(page);
> +	if (is_zero_pfn(page_to_pfn(page)))
> +		goto anon_key;
>  	if (!page->mapping) {
>  		unlock_page(page);
>  		put_page(page);
> @@ -268,6 +270,7 @@ again:
>  	 * the object not the particular process.
>  	 */
>  	if (PageAnon(page)) {
> + anon_key:
>  		key->both.offset |= FUT_OFF_MMSHARED; /* ref taken on mm */
>  		key->private.mm = mm;
>  		key->private.address = address;

Doesn't it make more sense to simply fail the futex op?

What I mean is, all (?) futex ops assume userspace actually wrote
something to the address in question to begin with, therefore it can
never be the zero page.

So it being the zero page means someone goofed up, and we should bail,
no?

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