[PATCH 0/4] gracefully handling mmap failures

With an out-of-the-box Linux kernel, I encountered failures at
all of these mmap call sites via "git cat-file --batch" on a
test repo with 100K total alternates.

While upping sys.vm.max_map_count and/or RLIMIT_DATA solves the
problem, not all users have administrative privileges to do so.

Eric Wong (4):
  use_pack: attempt to handle ENOMEM from mmap
  map_loose_object_1: attempt to handle ENOMEM from mmap
  check_packed_git_idx: attempt to handle ENOMEM from mmap
  xmmap: inform Linux users of tuning knobs on ENOMEM

 object-file.c | 16 ++++++++++++++--
 packfile.c    | 17 ++++++++++++-----
 packfile.h    |  2 ++
 3 files changed, 28 insertions(+), 7 deletions(-)

This benefits unprivileged users who lack permissions to raise
the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource
limit.

Multi-threaded callers to map_loose_object_1 (e.g. "git grep")
appear to guard against thread-safety problems.  Other
multi-core aware pieces of code (e.g. parallel-checkout) uses
child processes

Forcing a call to unuse_one_window() once before xmmap_gently()
revealed no test suite failures:

	--- a/object-file.c
	+++ b/object-file.c
	@@ -1197,6 +1197,7 @@ static void *map_loose_object_1(struct repository *r, const char *path,
					return NULL;
				}
				do {
	+				unuse_one_window(NULL);
					map = xmmap_gently(NULL, *size, PROT_READ,
							MAP_PRIVATE, fd, 0);
				} while (map == MAP_FAILED && errno == ENOMEM

Signed-off-by: Eric Wong <redacted>
---
 object-file.c | 8 +++++++-
 packfile.c    | 2 +-
 packfile.h    | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/object-file.c b/object-file.c
index f233b440b2..4c043f1f3c 100644
--- a/object-file.c
+++ b/object-file.c

@@ -1196,7 +1196,13 @@ static void *map_loose_object_1(struct repository *r, const char *path,
 				close(fd);
 				return NULL;
 			}
-			map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
+			do {
+				map = xmmap_gently(NULL, *size, PROT_READ,
+						MAP_PRIVATE, fd, 0);
+			} while (map == MAP_FAILED && errno == ENOMEM
+				&& unuse_one_window(NULL));
+			if (map == MAP_FAILED)
+				die_errno("%s cannot be mapped", path);
 		}
 		close(fd);
 	}

diff --git a/packfile.c b/packfile.c
index a0da790fb4..4bc7fa0f36 100644
--- a/packfile.c
+++ b/packfile.c

@@ -265,7 +265,7 @@ static void scan_windows(struct packed_git *p,
 	}
 }
 
-static int unuse_one_window(struct packed_git *current)
+int unuse_one_window(struct packed_git *current)
 {
 	struct packed_git *p, *lru_p = NULL;
 	struct pack_window *lru_w = NULL, *lru_l = NULL;

diff --git a/packfile.h b/packfile.h
index 3ae117a8ae..da9a061e30 100644
--- a/packfile.h
+++ b/packfile.h

@@ -196,4 +196,6 @@ int is_promisor_object(const struct object_id *oid);
 int load_idx(const char *path, const unsigned int hashsz, void *idx_map,
 	     size_t idx_size, struct packed_git *p);
 
+int unuse_one_window(struct packed_git *);
+
 #endif

Since use_pack() can already safely munmap packs to respect
core.packedGitLimit, attempt to gracefully handle ENOMEM
errors the same way by unmapping a window and retrying.

This benefits unprivileged users who lack permissions to raise
the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource
limit.

I've also verified it is safe to release a pack here by
unconditionally calling unuse_one_window() before
xmmap_gently():

	--- a/packfile.c
	+++ b/packfile.c
	@@ -649,6 +649,7 @@ unsigned char *use_pack(struct packed_git *p,
					&& unuse_one_window(p))
					; /* nothing */
				do {
	+				unuse_one_window(p);
					win->base = xmmap_gently(NULL, win->len,
						PROT_READ, MAP_PRIVATE,
						p->pack_fd, win->offset);

Signed-off-by: Eric Wong <redacted>
---
 packfile.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/packfile.c b/packfile.c
index 755aa7aec5..a0da790fb4 100644
--- a/packfile.c
+++ b/packfile.c

@@ -648,9 +648,12 @@ unsigned char *use_pack(struct packed_git *p,
 			while (packed_git_limit < pack_mapped
 				&& unuse_one_window(p))
 				; /* nothing */
-			win->base = xmmap_gently(NULL, win->len,
-				PROT_READ, MAP_PRIVATE,
-				p->pack_fd, win->offset);
+			do {
+				win->base = xmmap_gently(NULL, win->len,
+					PROT_READ, MAP_PRIVATE,
+					p->pack_fd, win->offset);
+			} while (win->base == MAP_FAILED && errno == ENOMEM
+				&& unuse_one_window(p));
 			if (win->base == MAP_FAILED)
 				die_errno("packfile %s cannot be mapped",
 					  p->pack_name);

This benefits unprivileged users who lack permissions to raise
the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource
limit.

It appears none of our multithreaded code is calling this,
or is doing so while holding appropropriate locks to serialize
access to the object database.

Unconditionally calling unuse_one_window() before xmmap_gently()
revealed no test suite failures:

	--- a/packfile.c
	+++ b/packfile.c
	@@ -98,6 +98,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)
			return error("index file %s is too small", path);
		}
		do {
	+		unuse_one_window(p);
			idx_map = xmmap_gently(NULL, idx_size, PROT_READ, MAP_PRIVATE,
						fd, 0);
		} while (idx_map == MAP_FAILED && errno == ENOMEM

Signed-off-by: Eric Wong <redacted>
---
 packfile.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/packfile.c b/packfile.c
index 4bc7fa0f36..2904560f52 100644
--- a/packfile.c
+++ b/packfile.c

@@ -97,7 +97,11 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)
 		close(fd);
 		return error("index file %s is too small", path);
 	}
-	idx_map = xmmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	do {
+		idx_map = xmmap_gently(NULL, idx_size, PROT_READ, MAP_PRIVATE,
+					fd, 0);
+	} while (idx_map == MAP_FAILED && errno == ENOMEM
+		&& unuse_one_window(p));
 	close(fd);
 
 	ret = load_idx(path, hashsz, idx_map, idx_size, p);

In cases where we can't (or haven't gotten around to) attempting
graceful handling of mmap failures, Linux users may benefit from
additional information on how to avoid the problem.

Signed-off-by: Eric Wong <redacted>
---
 object-file.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/object-file.c b/object-file.c
index 4c043f1f3c..4139a5ebe6 100644
--- a/object-file.c
+++ b/object-file.c

@@ -1027,8 +1027,14 @@ void *xmmap(void *start, size_t length,
 	int prot, int flags, int fd, off_t offset)
 {
 	void *ret = xmmap_gently(start, length, prot, flags, fd, offset);
-	if (ret == MAP_FAILED)
+	if (ret == MAP_FAILED) {
+#if defined(__linux__)
+		if (errno == ENOMEM)
+			die_errno(_(
+"mmap failed, check sys.vm.max_map_count and/or RLIMIT_DATA"));
+#endif /* OS-specific bits */
 		die_errno(_("mmap failed"));
+	}
 	return ret;
 }
 

Eric Wong [off-list ref] wrote:

Oops, I dropped extra error handling here :x

 	close(fd);
 
 	ret = load_idx(path, hashsz, idx_map, idx_size, p);

Eric Wong [off-list ref] writes:

Eric Wong [off-list ref] wrote:

quoted

--- a/packfile.c
+++ b/packfile.c

@@ -97,7 +97,11 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)
 		close(fd);
 		return error("index file %s is too small", path);
 	}
-	idx_map = xmmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	do {
+		idx_map = xmmap_gently(NULL, idx_size, PROT_READ, MAP_PRIVATE,
+					fd, 0);
+	} while (idx_map == MAP_FAILED && errno == ENOMEM
+		&& unuse_one_window(p));

Oops, I dropped extra error handling here :x

quoted

 	close(fd);
 
 	ret = load_idx(path, hashsz, idx_map, idx_size, p);

Something like this, perhaps?  You'd also need _() around the error
message you added to object-file.c in [2/4], I would think.

 packfile.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git c/packfile.c w/packfile.c
index 2904560f52..b31f14ecb7 100644
--- c/packfile.c
+++ w/packfile.c

@@ -102,6 +102,10 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)
 					fd, 0);
 	} while (idx_map == MAP_FAILED && errno == ENOMEM
 		&& unuse_one_window(p));
+	if (idx_map == MAP_FAILED) {
+		close(fd);
+		return error_errno(_("%s cannot be mapped"), path);
+	}
 	close(fd);
 
 	ret = load_idx(path, hashsz, idx_map, idx_size, p);

Junio C Hamano [off-list ref] wrote:

Something like this, perhaps?  You'd also need _() around the error
message you added to object-file.c in [2/4], I would think.

I think I need to drop 1-3 of this series because it seems
impossible to account for the mmap-via-malloc cases that
still bumps into vm.max_map_count limitations under Linux.

This series is now down to a single patch.

I wanted to make things more transparent to users without
privileges to raise sys.vm.max_map_count and/or RLIMIT_DATA;
but it doesn't seem possible to account for libc/zlib/etc. doing
mmap() without our knowledge (usually via malloc).

So I think giving users some information to feed their sysadmins
is the best we can do in this situation:

--------------8<-----------
Subject: [PATCH] xmmap: inform Linux users of tuning knobs on ENOMEM

Linux users may benefit from additional information on how to
avoid ENOMEM from mmap despite the system having enough RAM to
accomodate them.  We can't reliably unmap pack windows to work
around the issue since malloc and other library routines may
mmap without our knowledge.

Signed-off-by: Eric Wong <redacted>
---
 config.c          |  3 ++-
 git-compat-util.h |  1 +
 object-file.c     | 16 +++++++++++++++-
 packfile.c        |  4 ++--
 read-cache.c      |  3 ++-
 5 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/config.c b/config.c
index f9c400ad30..79ae9f2dea 100644
--- a/config.c
+++ b/config.c

@@ -3051,7 +3051,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		if (contents == MAP_FAILED) {
 			if (errno == ENODEV && S_ISDIR(st.st_mode))
 				errno = EISDIR;
-			error_errno(_("unable to mmap '%s'"), config_filename);
+			error_errno(_("unable to mmap '%s'%s"),
+					config_filename, mmap_os_err());
 			ret = CONFIG_INVALID_FILE;
 			contents = NULL;
 			goto out_free;

diff --git a/git-compat-util.h b/git-compat-util.h
index fb6e9af76b..fa6dd92219 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -876,6 +876,7 @@ char *xstrndup(const char *str, size_t len);
 void *xrealloc(void *ptr, size_t size);
 void *xcalloc(size_t nmemb, size_t size);
 void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
+const char *mmap_os_err(void);
 void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);
 int xopen(const char *path, int flags, ...);
 ssize_t xread(int fd, void *buf, size_t len);

diff --git a/object-file.c b/object-file.c
index f233b440b2..b9c3219793 100644
--- a/object-file.c
+++ b/object-file.c

@@ -1023,12 +1023,26 @@ void *xmmap_gently(void *start, size_t length,
 	return ret;
 }
 
+const char *mmap_os_err(void)
+{
+	static const char blank[] = "";
+#if defined(__linux__)
+	if (errno == ENOMEM) {
+		/* this continues an existing error message: */
+		static const char enomem[] =
+", check sys.vm.max_map_count and/or RLIMIT_DATA";
+		return enomem;
+	}
+#endif /* OS-specific bits */
+	return blank;
+}
+
 void *xmmap(void *start, size_t length,
 	int prot, int flags, int fd, off_t offset)
 {
 	void *ret = xmmap_gently(start, length, prot, flags, fd, offset);
 	if (ret == MAP_FAILED)
-		die_errno(_("mmap failed"));
+		die_errno(_("mmap failed%s"), mmap_os_err());
 	return ret;
 }
 

diff --git a/packfile.c b/packfile.c
index 755aa7aec5..9ef6d98292 100644
--- a/packfile.c
+++ b/packfile.c

@@ -652,8 +652,8 @@ unsigned char *use_pack(struct packed_git *p,
 				PROT_READ, MAP_PRIVATE,
 				p->pack_fd, win->offset);
 			if (win->base == MAP_FAILED)
-				die_errno("packfile %s cannot be mapped",
-					  p->pack_name);
+				die_errno(_("packfile %s cannot be mapped%s"),
+					  p->pack_name, mmap_os_err());
 			if (!win->offset && win->len == p->pack_size
 				&& !p->do_not_close)
 				close_pack_fd(p);

diff --git a/read-cache.c b/read-cache.c
index 77961a3885..a80902155c 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -2236,7 +2236,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 
 	mmap = xmmap_gently(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	if (mmap == MAP_FAILED)
-		die_errno(_("%s: unable to map index file"), path);
+		die_errno(_("%s: unable to map index file%s"), path,
+			mmap_os_err());
 	close(fd);
 
 	hdr = (const struct cache_header *)mmap;

-- 
It's probably not safe to feed sysadmins after midnight, though :>

On Tue, Jun 29, 2021 at 08:11:05AM +0000, Eric Wong wrote:

Since use_pack() can already safely munmap packs to respect
core.packedGitLimit, attempt to gracefully handle ENOMEM
errors the same way by unmapping a window and retrying.

This benefits unprivileged users who lack permissions to raise
the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource
limit.

I've also verified it is safe to release a pack here by
unconditionally calling unuse_one_window() before
xmmap_gently():

	--- a/packfile.c
	+++ b/packfile.c
	@@ -649,6 +649,7 @@ unsigned char *use_pack(struct packed_git *p,
					&& unuse_one_window(p))
					; /* nothing */
				do {
	+				unuse_one_window(p);
					win->base = xmmap_gently(NULL, win->len,
						PROT_READ, MAP_PRIVATE,
						p->pack_fd, win->offset);

I don't find that test-diff all that compelling, because we don't know
which window will get unused. I.e., if there is one that will get racily
unused, we might not hit it. I think it would be a lot more interesting
for finding problems if it did:

  while (unuse_one_window(p)) ;

to clear them all.

That said, I think this must be obviously correct because the code above
will potentially have just called unuse_one_window(p) already. So at
least if not obviously correct, no more buggy than the previous code. :)

-Peff

On Tue, Jun 29, 2021 at 08:11:06AM +0000, Eric Wong wrote:

This benefits unprivileged users who lack permissions to raise
the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource
limit.

Multi-threaded callers to map_loose_object_1 (e.g. "git grep")
appear to guard against thread-safety problems.  Other
multi-core aware pieces of code (e.g. parallel-checkout) uses
child processes

This one makes me slightly more nervous than the last, because we don't
know if somebody higher up the call stack might be expecting to use that
window again.

I _think_ this one is OK, because we would never read a loose object in
the process of reading a packed one. I thought we might in some rare
cases with corruption (e.g., B is a delta against A; we fail to read A
because it's corrupt, so we look elsewhere for a copy). I don't think
the current code is quite that clever, though. If B were corrupted, we'd
look elsewhere for a duplicate, but that logic doesn't apply in the
middle of a delta resolution (even though there are corruption cases it
would help recover from).

So I don't think this is introducing any bugs. But it worries me a bit
that it creates an opportunity for a subtle and hard-to-trigger
situation if we do change seemingly-unrelated code.

But looking more carefully at unuse_one_window(), I think the worst case
is a slight performance drop, as we unmap the window and then re-map
when somebody higher up the call-stack needs it again. My real worry was
that we could trigger a case where somebody was holding onto a pointer
to the bytes. But I think use_pack() will mark those bytes via
inuse_cnt, and then unuse_one_window() will never drop a window that has
a non-zero inuse_count.

So in that sense all of your patches should be correct without thinking
too hard no them. Sure, a performance drop from having to re-map the
window later isn't great, but if the alternative in each case is calling
die(), it's a strict improvement.

-Peff

On Wed, Jun 30, 2021 at 12:01:32AM +0000, Eric Wong wrote:

This series is now down to a single patch.

I wanted to make things more transparent to users without
privileges to raise sys.vm.max_map_count and/or RLIMIT_DATA;
but it doesn't seem possible to account for libc/zlib/etc. doing
mmap() without our knowledge (usually via malloc).

Oh, I should have read this one before reviewing the inner parts of v1. :)

In general I agree that trying to manage our map count can never be
foolproof. As you note, other parts of the system may contribute to that
count. But even within Git, we don't have any mechanism for unmapping
many non-packfiles. So if you have 30,000 packs, you may hit the limit
purely via the .idx files (and ditto for the new .rev files, and
probably commit-graph files, etc).

That said, I'm not opposed to handling xmmap() failures more gracefully,
as your series did. It's not foolproof, but it might help in some cases.

So I think giving users some information to feed their sysadmins
is the best we can do in this situation:

This seems OK to me, too. Translators might complain a bit about the
message-lego. I don't have a strong opinion.

-Peff

On Tue, Jun 29 2021, Eric Wong wrote:

This benefits unprivileged users who lack permissions to raise
the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource
limit.

Multi-threaded callers to map_loose_object_1 (e.g. "git grep")
appear to guard against thread-safety problems.  Other
multi-core aware pieces of code (e.g. parallel-checkout) uses
child processes

s/uses/use/ & missing full-stop.

Jeff King [off-list ref] wrote:

On Wed, Jun 30, 2021 at 12:01:32AM +0000, Eric Wong wrote:

quoted

This series is now down to a single patch.

I wanted to make things more transparent to users without
privileges to raise sys.vm.max_map_count and/or RLIMIT_DATA;
but it doesn't seem possible to account for libc/zlib/etc. doing
mmap() without our knowledge (usually via malloc).

Oh, I should have read this one before reviewing the inner parts of v1. :)

In general I agree that trying to manage our map count can never be
foolproof. As you note, other parts of the system may contribute to that
count. But even within Git, we don't have any mechanism for unmapping
many non-packfiles. So if you have 30,000 packs, you may hit the limit
purely via the .idx files (and ditto for the new .rev files, and
probably commit-graph files, etc).

Yeah, the most annoying thing with my original series was when
I hit "inflate: out of memory" once I stopped xmmap from dying.
I suspect that would be a worse far error message for users who
aren't familiar with how malloc works.

That said, I'm not opposed to handling xmmap() failures more gracefully,
as your series did. It's not foolproof, but it might help in some cases.

I've also been wondering if we can maintain a watermark based
on reading the contents of /proc/sys/vm/max_map_count and the
mappings we make.   Then we could start dropping mappings
when we hit half (or some other threshold) of that sysctl.
Similar for RLIMIT_DATA (though that defaults to unlimited
on my Debian system).

OTOH, I also wonder if we're overusing mmap when we could be
just as well served with pread.

I'm not up-to-date on modern mmap performance and maybe CPU
vulnerability mitigations nowadays make mmap more compelling.
However, once upon a time in 2006, pread could be a hair quicker:

https://lore.kernel.org/git/Pine.LNX.4.64.0612182234260.3479@woody.osdl.org/T/#u
(But that info could be out-of-date...)

I'm also somewhat paranoid when it comes to mmap since rogue
processes could be truncating mmap-ed files to cause bus errors.

quoted

So I think giving users some information to feed their sysadmins
is the best we can do in this situation:

This seems OK to me, too. Translators might complain a bit about the
message-lego. I don't have a strong opinion.

*shrug*  I saw my original patches already ended up in `seen'
(commit 7b79212a93c375365c06cab5c0018ab97a4185cf)

On Wed, Jun 30, 2021 at 10:07:22AM +0000, Eric Wong wrote:

quoted

In general I agree that trying to manage our map count can never be
foolproof. As you note, other parts of the system may contribute to that
count. But even within Git, we don't have any mechanism for unmapping
many non-packfiles. So if you have 30,000 packs, you may hit the limit
purely via the .idx files (and ditto for the new .rev files, and
probably commit-graph files, etc).

Yeah, the most annoying thing with my original series was when
I hit "inflate: out of memory" once I stopped xmmap from dying.
I suspect that would be a worse far error message for users who
aren't familiar with how malloc works.

Yep. When I've seen this problem in the wild, that was exactly the
message I hit, too.

quoted

That said, I'm not opposed to handling xmmap() failures more gracefully,
as your series did. It's not foolproof, but it might help in some cases.

I've also been wondering if we can maintain a watermark based
on reading the contents of /proc/sys/vm/max_map_count and the
mappings we make.   Then we could start dropping mappings
when we hit half (or some other threshold) of that sysctl.
Similar for RLIMIT_DATA (though that defaults to unlimited
on my Debian system).

Maybe. You'd still need to teach Git to start closing pack idx files.

One downside of closing any maps is that it leads to races with
simultaneous repacks. Usually if a pack goes away we'll re-scan the
objects directory and look for new packs. But in pack-objects, we commit
ourselves to a certain packed representation. If that pack goes away
(and we aren't holding on to it via an mmap or open descriptor), then we
have to bail.

If we hit a point where the alternative is calling die() because we
couldn't map or allocate something, then risking the race is OK. But
unmapping at a more conservative level opens us up to that race even
when we would have otherwise succeeded.

OTOH, I also wonder if we're overusing mmap when we could be
just as well served with pread.

I'm not up-to-date on modern mmap performance and maybe CPU
vulnerability mitigations nowadays make mmap more compelling.
However, once upon a time in 2006, pread could be a hair quicker:

You can compile with NO_MMAP now to emulate it with pread(), but I doubt
it performs that well. If you shrink the packedGitWindowSize, then I
think use_pack() would bring in and operate on reasonable amounts of
bytes at a time.

But lots of other code will mmap whole files (.idx code, packed-refs
lookup, the index). We'd end up copying those files entirely into
memory, rather than accessing them directly page-wise.  That increases
RAM usage (we are copying into our own heap) and makes operations
inherently O(n) in the file sizes (one of the big reasons we mmap the
packed-refs file is so that we can binary search without having to
process the whole thing).

Those cases could be converted to use pread() more directly. Doing so
for .idx files might not be too bad, as the records are fixed-length and
we know where each step of the binary search will land. For packed-refs
it would get pretty hairy; we jump to a particular offset and then have
to scan around (sometimes backwards) for the beginning of the record.

quoted

quoted

So I think giving users some information to feed their sysadmins
is the best we can do in this situation:

This seems OK to me, too. Translators might complain a bit about the
message-lego. I don't have a strong opinion.

*shrug*  I saw my original patches already ended up in `seen'
(commit 7b79212a93c375365c06cab5c0018ab97a4185cf)

That doesn't necessarily mean much. Only that the topic was "seen" by
the maintainer and didn't look like complete garbage. :)

-Peff