NetBSD Problem Report #52239

From gson@gson.org  Wed May 17 17:25:00 2017
Return-Path: <gson@gson.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id A23BB7A1AF
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 17 May 2017 17:25:00 +0000 (UTC)
Message-Id: <20170517172454.B257F743D38@guava.gson.org>
Date: Wed, 17 May 2017 20:24:54 +0300 (EEST)
From: gson@gson.org (Andreas Gustafsson)
Reply-To: gson@gson.org (Andreas Gustafsson)
To: gnats-bugs@NetBSD.org
Subject: Changing protections of already mmap'ed region can fail
X-Send-Pr-Version: 3.95

>Number:         52239
>Category:       kern
>Synopsis:       Changing protections of already mmap'ed region can fail
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed May 17 17:25:00 +0000 2017
>Closed-Date:    Wed Apr 06 10:11:28 +0000 2022
>Last-Modified:  Wed Apr 06 10:11:28 +0000 2022
>Originator:     Andreas Gustafsson
>Release:        NetBSD 7.0.2
>Organization:

>Environment:
System: NetBSD
Architecture: x86_64
Machine: amd64
>Description:

As reported in https://github.com/jemalloc/jemalloc/issues/837
recent versions of jemalloc fail on NetBSD.  I have now tracked down
the cause of this problem to an issue with NetBSD's mmap().

With its default setting of "os_overcommits = false", jemalloc will
allocate regions of anonymous memory using

  p = mmap(0, ...PROT_NONE, MAP_ANON|MAP_PRIVATE...)

and then selectively enable read and write access on parts that are
about to actually be used using

  mmap(p, ...PROT_READ|PROT_WRITE, MAP_ANON|MAP_PRIVATE|MAP_FIXED...)

where p is the pointer returned by the first mmap call.

The problem is that if a second thread simultaneously does an
unrelated allocation of anonymous memory using mmap(0, ...), the
memory region whose protections are in the process of being changed by
the first thread can end up being returned by the mmap() of the second
thread instead, and the mmap(...MAP_FIXED...) of the first thread then
fails with ENOMEM.

I have now worked around this issue in the pkgsrc jemalloc package by
setting "os_overcommits = true".

>How-To-Repeat:

Here is a test case.  It can be run with

  cc testcase.c  -lpthread -o testcase && ./testcase

and will print "update mmap: Cannot allocate memory" when it fails.

#include <assert.h>
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <pthread.h>

#include <sys/mman.h>

#define SIZE (13*4096)

#define NITER 10000000

void *thread_main(void *arg) {
    int i, r;
    for (i = 0; i < NITER; i++) {
	// Get some unrelated memory
	void *t = mmap(0, SIZE, PROT_READ|PROT_WRITE, MAP_ANON|MAP_PRIVATE, -1, 0);
	assert(t);
	r = munmap(t, SIZE);
	assert(r == 0);
    }
    return 0;
}

int main(int argc, char **argv) {
    int i, r;

    pthread_t thread;
    r = pthread_create(&thread, 0, thread_main, 0);
    assert(r == 0);

    for (i = 0; i < NITER; i++) {
	// Get a placeholder region
	void *p = mmap(0, SIZE, PROT_NONE, MAP_ANON|MAP_PRIVATE, -1, 0);
	if (p == MAP_FAILED) {
	    printf("mmap: %s\n", strerror(errno));
	}
	assert(p != MAP_FAILED);

	// Upgrade placeholder to read/write access
	void *q = mmap(p, SIZE, PROT_READ|PROT_WRITE, MAP_ANON|MAP_PRIVATE|MAP_FIXED, -1, 0);
	if (q == MAP_FAILED) {
	    printf("update mmap: %s\n", strerror(errno));
	    exit(1);
	}
	assert(q == p);

	// Free it
	r = munmap(q, SIZE);
	if (r != 0) {
	    printf("munmap: %s\n", strerror(errno));
	    exit(1);
	}
    }
    return 0;
}

>Fix:

>Release-Note:

>Audit-Trail:
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, 
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Cc: chuq@netbsd.org
Subject: Re: kern/52239: Changing protections of already mmap'ed region can fail
Date: Wed, 17 May 2017 22:32:17 -0400

 On May 17,  5:25pm, gson@gson.org (Andreas Gustafsson) wrote:
 -- Subject: kern/52239: Changing protections of already mmap'ed region can fa

 This seems to fix it, but I am not sure if it is correct. But that does not
 explain why it fails in the first place since there is plenty of memory
 around. Chuq?

 christos

 Index: uvm_mmap.c
 ===================================================================
 RCS file: /cvsroot/src/sys/uvm/uvm_mmap.c,v
 retrieving revision 1.164
 diff -u -u -r1.164 uvm_mmap.c
 --- uvm_mmap.c	6 May 2017 21:34:52 -0000	1.164
 +++ uvm_mmap.c	18 May 2017 02:28:52 -0000
 @@ -909,7 +909,7 @@
  {
  	vaddr_t align = 0;
  	int error;
 -	uvm_flag_t uvmflag = 0;
 +	uvm_flag_t uvmflag = UVM_FLAG_WAITVA;

  	/*
  	 * check params

From: "Chuck Silvers" <chs@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52239 CVS commit: src/sys/uvm
Date: Fri, 19 May 2017 15:30:19 +0000

 Module Name:	src
 Committed By:	chs
 Date:		Fri May 19 15:30:19 UTC 2017

 Modified Files:
 	src/sys/uvm: uvm_map.c uvm_mmap.c

 Log Message:
 make MAP_FIXED mapping operations atomic. fixes PR 52239.
 previously, unmapping any entries being replaced was done separately
 from entering the new mapping, which allowed another thread doing
 a non-MAP_FIXED mapping to allocate the range out from under the
 MAP_FIXED thread.


 To generate a diff of this commit:
 cvs rdiff -u -r1.346 -r1.347 src/sys/uvm/uvm_map.c
 cvs rdiff -u -r1.164 -r1.165 src/sys/uvm/uvm_mmap.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Fri, 19 May 2017 16:04:14 +0000
State-Changed-Why:
fixed? (should then go into -7)


State-Changed-From-To: feedback->open
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sat, 20 May 2017 05:37:35 +0000
State-Changed-Why:
not yet, this change seems to be causing problems


From: Andreas Gustafsson <gson@gson.org>
To: gnats-bugs@NetBSD.org
Cc: dholland@NetBSD.org, chs@NetBSD.org
Subject: Re: kern/52239 (Changing protections of already mmap'ed region can fail)
Date: Sun, 4 Jun 2017 16:32:43 +0300

 On May 20, dholland@NetBSD.org wrote:
 > not yet, this change seems to be causing problems

 Looks like those problems were fixed already, were they not?

 I am now testing a -current system (source date 2017.05.31.14.41.07)
 and a jemalloc-4.5.0 package (that is, without the change to set
 os_overcommits to true that I added in 4.5.0nb1).  It passes the
 test case in the PR, jemaclloc's own tests, and the tests of a certain
 proprietary application linked with jemalloc that also previously
 failed.

 As far as I'm concerned, the PR can be closed.  Thanks to chs for
 the fix, and to the others who helped.
 -- 
 Andreas Gustafsson, gson@gson.org

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/52239 (Changing protections of already mmap'ed region can
 fail)
Date: Sun, 4 Jun 2017 17:34:23 +0000

 On Sun, Jun 04, 2017 at 01:35:00PM +0000, Andreas Gustafsson wrote:
  >  On May 20, dholland@NetBSD.org wrote:
  >  > not yet, this change seems to be causing problems
  >  
  >  Looks like those problems were fixed already, were they not?

 Yes.

  >  I am now testing a -current system (source date 2017.05.31.14.41.07)
  >  and a jemalloc-4.5.0 package (that is, without the change to set
  >  os_overcommits to true that I added in 4.5.0nb1).  It passes the
  >  test case in the PR, jemaclloc's own tests, and the tests of a certain
  >  proprietary application linked with jemalloc that also previously
  >  failed.
  >  
  >  As far as I'm concerned, the PR can be closed.  Thanks to chs for
  >  the fix, and to the others who helped.

 Good to hear; but shouldn't the fix get into -7?

 -- 
 David A. Holland
 dholland@netbsd.org

From: Andreas Gustafsson <gson@gson.org>
To: dholland-bugs@netbsd.org
Cc: gnats-bugs@NetBSD.org
Subject: Re: kern/52239 (Changing protections of already mmap'ed region can
 fail)
Date: Sun, 4 Jun 2017 22:47:44 +0300

 David Holland wrote:
 > shouldn't the fix get into -7?

 Feel free to submit a pullup request.  I'm not planning to do it
 myself since the work-around in the jemalloc-4.5.0nb1 pkgsrc package
 is sufficient for my purposes and I have higher priority bugs to
 deal with.
 -- 
 Andreas Gustafsson, gson@gson.org

From: "Andreas Gustafsson" <gson@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52239 CVS commit: src/tests/lib/libc/sys
Date: Wed, 6 Apr 2022 10:02:55 +0000

 Module Name:	src
 Committed By:	gson
 Date:		Wed Apr  6 10:02:55 UTC 2022

 Modified Files:
 	src/tests/lib/libc/sys: Makefile t_mmap.c

 Log Message:
 Add a regression test for PR kern/52239, "Changing protections of
 already mmap'ed region can fail", based on the test program in the PR.


 To generate a diff of this commit:
 cvs rdiff -u -r1.70 -r1.71 src/tests/lib/libc/sys/Makefile
 cvs rdiff -u -r1.16 -r1.17 src/tests/lib/libc/sys/t_mmap.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: gson@NetBSD.org
State-Changed-When: Wed, 06 Apr 2022 10:11:28 +0000
State-Changed-Why:
Fixed since 2017, and pullups are no longer relevant as -7 is EOL.


>Unformatted:

Home	PR Database Search