Skip to contentSkip to conversation

mm/hugetlb: possible temporary resv underflow

BUG Guillaume Morin 3 participants 9 messages Jul 10 – Jul 21

Subsystems (3)
Memory management Kselftest MM misc
lore
Share & tools
lore

Guillaume Morin

author
#0 lore raw

Hi,

We've encountered a temporary underflow in the reserved count after a fork when a parent unmaps a faulted hugetlb page before a child process.

After a reservation is consumed (i.e the page is faulted), if there is a fork() and the parent munmaps the page before the child, the reserved count underflows. The count is restored when the child unmaps the page/exits. I have reproduced the issue in 6.12.95 and 6.18.38. I believe it's still present in 7.2-rc2 but didn't try to reproduce.

The patch that seems to have introduced the issue is df7a6d1f6405, "mm/hugetlb:
restore the reservation if needed"

I am not sure what the best fix is but checking folio_mapcount() == 0 in __unmap_hugepage_range() seems to do the trick

                adjust_reservation = false;
 
                spin_lock_irq(&hugetlb_lock);
                if (!h->surplus_huge_pages && __vma_private_lock(vma) &&
-                   folio_test_anon(folio)) {
+                   folio_test_anon(folio) &&
+                   folio_mapcount(folio) == 0) {
                        folio_set_hugetlb_restore_reserve(page_folio(page));
                        /* Reservation to be adjusted after the spin lock */
                        adjust_reservation = true;

Reproducer (it only works if HugePages_Rsvd is 0 at the beginning of the test)

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdint.h>
#include <ctype.h>
#include <errno.h>
#include <limits.h>
#include <unistd.h>
#include <sys/mman.h>
#include <sys/wait.h>

static unsigned long num_reserved()

{
	const char* field = "HugePages_Rsvd: ";
	FILE *f = fopen("/proc/meminfo", "r");
	char line[256];
	long v;

	if (!f) {
		perror("Could not open /proc/meminfo");
		exit(1);
	}
	while (fgets(line, sizeof(line), f)) {
		if (strncmp(line, field, strlen(field)))
			continue;
		const char *p = line + strlen(field);
		while (*p && isspace(*p))
			++p;
		if (!*p) {
			printf("could not parse /proc/meminfo line: %s", line);
			exit(1);
		}
		v = strtoul(p, NULL, 10);
		fclose(f);
		return v;
	}
	fclose(f);
	puts("could not parse /proc/meminfo line");
	exit(1);
}

int main(void)

{
	const size_t sz = 2048UL * 1024;
	int pipe_fds[2];
	pid_t pid;
	char *addr;
	long nr;

	nr = num_reserved();
	if (nr != 0) {
		puts("For the test, HugePages_Rsvd must be 0");
		return 1;
	}
	addr = mmap(NULL, sz, PROT_READ | PROT_WRITE,
		MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB, -1, 0);
	if (addr == MAP_FAILED) {
		perror("Could not allocate a huge page");
		return 1;
	}
	if (num_reserved() != 1) {
		puts("After mmap, HugePages_Rsvd must be 1");
		return 1;
	}

	/* Fault */
	*addr = 0;
	if (num_reserved() != 0) {
		puts("After faulting, HugePages_Rsvd must be 0");
		return 1;
	}

	if (pipe(pipe_fds) != 0) {
		perror("pipe failed");
		return 1;
	}

	pid = fork();
	if (pid < 0) {
		perror("fork failed");
		return 1;
	}

	if (pid == 0) {
		/* Child: Simply wait for the parent */
		char b;
		close(pipe_fds[1]);
		if (read(pipe_fds[0], &b, 1) < 0) {
			puts("Child: read failed");
			return 1;
		}
		return 0;
	}

	/* Parent */
	close(pipe_fds[0]);

	/* First unmap, this will close the vma */
	if (munmap(addr, sz) != 0) {
		perror("munmap");
		kill(pid, SIGKILL);
		return 1;
	}

	nr = num_reserved();
	if (nr == ULONG_MAX) {
		puts("After the munmap, HugePages_Rsvd underflowed!");
	} else if (nr == 0) {
		puts("Underflow not present!");
	} else {
		printf("Unexpected HugePages_Rsvd=%ld after munmap, should be "
			"0 or -1. Repeat the test\n", nr);
	}
	/* Make the child exit, this should restore HugePages_Rsvd to 0 */
	if (write(pipe_fds[1], &nr, 1) < 0) {
		perror("write failed");
		kill(pid, SIGKILL);
		return 1;
	}
	close(pipe_fds[1]);
	if (waitpid(pid, NULL, 0) <= 0) {
		perror("waitpid failed");
		return 1;
	}

	nr = num_reserved();
	if (nr == 0) {
		puts("After the child dies, HugePages_Rsvd is properly "
		     "set to 0");
	} else {
		printf("Unexpected HugePages_Rsvd=%ld after the child "
		       "termination munmap, should be 0 or -1. "
		       "Repeat the test\n", nr);
	}

	return 0;
}
Signature · 1 line
Guillaume Morin <[email protected]>
#1 lore raw

Hi Guillaume,

Quoted 10 lines · Guillaume Morin

Hi,

We've encountered a temporary underflow in the reserved count after a fork when a parent unmaps a faulted hugetlb page before a child process.

After a reservation is consumed (i.e the page is faulted), if there is a fork() and the parent munmaps the page before the child, the reserved count underflows. The count is restored when the child unmaps the page/exits. I have reproduced the issue in 6.12.95 and 6.18.38. I believe it's still present in 7.2-rc2 but didn't try to reproduce.

Thanks for the detailed report and the reproducer.

I reproduced the underflow on current linux-next (7.2.0-rc2-next-20260710) with your program — it fires on every run. I also confirmed your folio_mapcount() == 0 check fixes it: with it applied the underflow is gone across many runs, and the existing hugetlb reservation selftests still pass.

Quoted 10 lines

The patch that seems to have introduced the issue is df7a6d1f6405, "mm/hugetlb:
restore the reservation if needed"

I am not sure what the best fix is but checking folio_mapcount() == 0 in __unmap_hugepage_range() seems to do the trick

                adjust_reservation = false;
 
                spin_lock_irq(&hugetlb_lock);
                if (!h->surplus_huge_pages && __vma_private_lock(vma) &&

Review

Quoted 6 lines
(quoted diff) +2 -1
-                   folio_test_anon(folio)) {
+                   folio_test_anon(folio) &&
+                   folio_mapcount(folio) == 0) {
                        folio_set_hugetlb_restore_reserve(page_folio(page));
                        /* Reservation to be adjusted after the spin lock */
                        adjust_reservation = true;

The fix looks correct to me.

Quoted 1 line

Reproducer (it only works if HugePages_Rsvd is 0 at the beginning of the test)

Would you be up for turning your reproducer into an expansion of the selftest I've added a while ago for the fix I've added a while ago:

tools/testing/selftests/mm/hugetlb_madv_vs_map.c

Your case is a nice complement to it, so it'd be great to have it there.

#2 lore raw

Hello Breno,

Quoted 36 lines · Breno Leitao
Quoted 10 lines · Guillaume Morin

Hi,

We've encountered a temporary underflow in the reserved count after a fork when a parent unmaps a faulted hugetlb page before a child process.

After a reservation is consumed (i.e the page is faulted), if there is a fork() and the parent munmaps the page before the child, the reserved count underflows. The count is restored when the child unmaps the page/exits. I have reproduced the issue in 6.12.95 and 6.18.38. I believe it's still present in 7.2-rc2 but didn't try to reproduce.

Thanks for the detailed report and the reproducer.

I reproduced the underflow on current linux-next (7.2.0-rc2-next-20260710) with your program — it fires on every run. I also confirmed your folio_mapcount() == 0 check fixes it: with it applied the underflow is gone across many runs, and the existing hugetlb reservation selftests still pass.

Quoted 10 lines

The patch that seems to have introduced the issue is df7a6d1f6405, "mm/hugetlb:
restore the reservation if needed"

I am not sure what the best fix is but checking folio_mapcount() == 0 in __unmap_hugepage_range() seems to do the trick

                adjust_reservation = false;
 
                spin_lock_irq(&hugetlb_lock);
                if (!h->surplus_huge_pages && __vma_private_lock(vma) &&

Review

Quoted 6 lines
(quoted diff) +2 -1
-                   folio_test_anon(folio)) {
+                   folio_test_anon(folio) &&
+                   folio_mapcount(folio) == 0) {
                        folio_set_hugetlb_restore_reserve(page_folio(page));
                        /* Reservation to be adjusted after the spin lock */
                        adjust_reservation = true;
Quoted 1 line

The fix looks correct to me.

Excellent! Will you be submitting a patch or do you need me to do something there?

Quoted 8 lines
Quoted 1 line

Reproducer (it only works if HugePages_Rsvd is 0 at the beginning of the test)

Would you be up for turning your reproducer into an expansion of the selftest I've added a while ago for the fix I've added a while ago:

tools/testing/selftests/mm/hugetlb_madv_vs_map.c

Your case is a nice complement to it, so it'd be great to have it there.

I've never used the selftest API but I gave it a shot. Hopefully it's a decent first try

tools/testing/selftests/mm/hugepage_settings.c +29 -0
index 2eab2110ac6a..02323389df39 100644
@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
+#include <ctype.h>
 #include <dirent.h>
 #include <fcntl.h>
 #include <limits.h>

@@ -449,6 +450,34 @@ unsigned long hugetlb_free_pages(unsigned long size)
 	return read_num(path);
 }
 
+bool hugetlb_nr_resv_pages(unsigned long *num)
+{
+	const char* field = "HugePages_Rsvd: ";
+	FILE *f = fopen("/proc/meminfo", "r");
+	char line[256];
+
+	if (!f) {
+		ksft_perror("Could not open /proc/meminfo");
+		return false;
+	}
+	while (fgets(line, sizeof(line), f)) {
+		if (strncmp(line, field, strlen(field)))
+			continue;
+		const char *p = line + strlen(field);
+		while (*p && isspace(*p))
+			++p;
+		if (!*p) {
+			break;
+		}
+		*num = strtoul(p, NULL, 10);
+		fclose(f);
+		return true;
+	}
+	fclose(f);
+	ksft_perror("could not parse /proc/meminfo");
+	return false;
+}
+
 static bool __hugetlb_setup(unsigned long size, unsigned long nr)
 {
 	unsigned long free = hugetlb_free_pages(size);
tools/testing/selftests/mm/hugepage_settings.h +1 -0
index 726c73c43c05..a73146c8e4a4 100644
@@ -98,6 +98,7 @@ unsigned long default_huge_page_size(void);
 unsigned long hugetlb_nr_pages(unsigned long size);
 void hugetlb_set_nr_pages(unsigned long size, unsigned long nr);
 unsigned long hugetlb_free_pages(unsigned long size);
+bool hugetlb_nr_resv_pages(unsigned long *num);
 
 static inline void hugetlb_save_settings(void)
 {
tools/testing/selftests/mm/hugetlb_madv_vs_map.c +91 -1

116 lines not on this page · raw message

Signature · 1 line
Guillaume Morin <[email protected]>
author
#3 lore raw
In reply to Breno Leitao

Sorry for the resend, an old send hook put an old email address. Fixed now.

Hello Breno,

Quoted 36 lines · Breno Leitao
Quoted 10 lines · Guillaume Morin

Hi,

We've encountered a temporary underflow in the reserved count after a fork when a parent unmaps a faulted hugetlb page before a child process.

After a reservation is consumed (i.e the page is faulted), if there is a fork() and the parent munmaps the page before the child, the reserved count underflows. The count is restored when the child unmaps the page/exits. I have reproduced the issue in 6.12.95 and 6.18.38. I believe it's still present in 7.2-rc2 but didn't try to reproduce.

Thanks for the detailed report and the reproducer.

I reproduced the underflow on current linux-next (7.2.0-rc2-next-20260710) with your program — it fires on every run. I also confirmed your folio_mapcount() == 0 check fixes it: with it applied the underflow is gone across many runs, and the existing hugetlb reservation selftests still pass.

Quoted 10 lines

The patch that seems to have introduced the issue is df7a6d1f6405, "mm/hugetlb:
restore the reservation if needed"

I am not sure what the best fix is but checking folio_mapcount() == 0 in __unmap_hugepage_range() seems to do the trick

                adjust_reservation = false;
 
                spin_lock_irq(&hugetlb_lock);
                if (!h->surplus_huge_pages && __vma_private_lock(vma) &&

Review

Quoted 6 lines
(quoted diff) +2 -1
-                   folio_test_anon(folio)) {
+                   folio_test_anon(folio) &&
+                   folio_mapcount(folio) == 0) {
                        folio_set_hugetlb_restore_reserve(page_folio(page));
                        /* Reservation to be adjusted after the spin lock */
                        adjust_reservation = true;
Quoted 1 line

The fix looks correct to me.

Excellent! Will you be submitting a patch or do you need me to do something there?

Quoted 8 lines
Quoted 1 line

Reproducer (it only works if HugePages_Rsvd is 0 at the beginning of the test)

Would you be up for turning your reproducer into an expansion of the selftest I've added a while ago for the fix I've added a while ago:

tools/testing/selftests/mm/hugetlb_madv_vs_map.c

Your case is a nice complement to it, so it'd be great to have it there.

I've never used the selftest API but I gave it a shot. Hopefully it's a decent first try

tools/testing/selftests/mm/hugepage_settings.c +29 -0
index 2eab2110ac6a..02323389df39 100644
@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
+#include <ctype.h>
 #include <dirent.h>
 #include <fcntl.h>
 #include <limits.h>

@@ -449,6 +450,34 @@ unsigned long hugetlb_free_pages(unsigned long size)
 	return read_num(path);
 }
 
+bool hugetlb_nr_resv_pages(unsigned long *num)
+{
+	const char* field = "HugePages_Rsvd: ";
+	FILE *f = fopen("/proc/meminfo", "r");
+	char line[256];
+
+	if (!f) {
+		ksft_perror("Could not open /proc/meminfo");
+		return false;
+	}
+	while (fgets(line, sizeof(line), f)) {
+		if (strncmp(line, field, strlen(field)))
+			continue;
+		const char *p = line + strlen(field);
+		while (*p && isspace(*p))
+			++p;
+		if (!*p) {
+			break;
+		}
+		*num = strtoul(p, NULL, 10);
+		fclose(f);
+		return true;
+	}
+	fclose(f);
+	ksft_perror("could not parse /proc/meminfo");
+	return false;
+}
+
 static bool __hugetlb_setup(unsigned long size, unsigned long nr)
 {
 	unsigned long free = hugetlb_free_pages(size);
tools/testing/selftests/mm/hugepage_settings.h +1 -0
index 726c73c43c05..a73146c8e4a4 100644
@@ -98,6 +98,7 @@ unsigned long default_huge_page_size(void);
 unsigned long hugetlb_nr_pages(unsigned long size);
 void hugetlb_set_nr_pages(unsigned long size, unsigned long nr);
 unsigned long hugetlb_free_pages(unsigned long size);
+bool hugetlb_nr_resv_pages(unsigned long *num);
 
 static inline void hugetlb_save_settings(void)
 {
tools/testing/selftests/mm/hugetlb_madv_vs_map.c +91 -1

116 lines not on this page · raw message

Signature · 1 line
Guillaume Morin <[email protected]>
#4 lore raw
Quoted 14 lines · Guillaume Morin
Quoted 14 lines
Quoted 13 lines

I am not sure what the best fix is but checking folio_mapcount() == 0 in __unmap_hugepage_range() seems to do the trick

                adjust_reservation = false;
 
                spin_lock_irq(&hugetlb_lock);
                if (!h->surplus_huge_pages && __vma_private_lock(vma) &&
(quoted diff) +2 -1
-                   folio_test_anon(folio)) {
+                   folio_test_anon(folio) &&
+                   folio_mapcount(folio) == 0) {
                        folio_set_hugetlb_restore_reserve(page_folio(page));
                        /* Reservation to be adjusted after the spin lock */
                        adjust_reservation = true;
Quoted 4 lines
Quoted 1 line

The fix looks correct to me.

Excellent! Will you be submitting a patch or do you need me to do something there?

Up to you. You can either send it, or, I can do it myself.

author
#5 lore raw
Quoted 16 lines · Breno Leitao
Quoted 14 lines · Guillaume Morin
Quoted 14 lines
Quoted 13 lines

I am not sure what the best fix is but checking folio_mapcount() == 0 in __unmap_hugepage_range() seems to do the trick

                adjust_reservation = false;
 
                spin_lock_irq(&hugetlb_lock);
                if (!h->surplus_huge_pages && __vma_private_lock(vma) &&
(quoted diff) +2 -1
-                   folio_test_anon(folio)) {
+                   folio_test_anon(folio) &&
+                   folio_mapcount(folio) == 0) {
                        folio_set_hugetlb_restore_reserve(page_folio(page));
                        /* Reservation to be adjusted after the spin lock */
                        adjust_reservation = true;
Quoted 6 lines
Quoted 4 lines
Quoted 1 line

The fix looks correct to me.

Excellent! Will you be submitting a patch or do you need me to do something there?

Up to you. You can either send it, or, I can do it myself.

I just sent a patch, hopefully it does the job.

Did you have the chance of looking at the selftest version of the repro? Should I spin up a patch for this as well?

Guillaume.

Signature · 1 line
Guillaume Morin <[email protected]>
#6 lore raw
Quoted 2 lines · Guillaume Morin

Did you have the chance of looking at the selftest version of the repro? Should I spin up a patch for this as well?

I've skimmed over it, and it seems fine. please send it in a proper patch and I will review it.

Thanks
--breno

author
#7 lore raw
Quoted 7 lines · Breno Leitao
Quoted 2 lines · Guillaume Morin

Did you have the chance of looking at the selftest version of the repro? Should I spin up a patch for this as well?

I've skimmed over it, and it seems fine. please send it in a proper patch and I will review it.

Will do once the fix is reviewed, no point in adding a selftest which fails everytime I think but let me know if you feel otherwise

I do not know if you had the chance to take a look at the patch:
https://lore.kernel.org/linux-mm/[email protected]/

Signature · 1 line
Guillaume Morin <[email protected]>
#8 lore raw
Quoted 6 lines · Guillaume Morin
Quoted 5 lines · Breno Leitao
Quoted 2 lines · Guillaume Morin

Did you have the chance of looking at the selftest version of the repro? Should I spin up a patch for this as well?

Quoted 8 lines
Quoted 2 lines

I've skimmed over it, and it seems fine. please send it in a proper patch and I will review it.

Will do once the fix is reviewed, no point in adding a selftest which fails everytime I think but let me know if you feel otherwise

I do not know if you had the chance to take a look at the patch:
https://lore.kernel.org/linux-mm/[email protected]/

Yes, I've looked at it today, and it seems correct, just a few nits.

--breno