[LTP] [PATCH v2] Hugetlb: Test to detect bug with freeing gigantic hugetlb pages

[LTP] [PATCH v2] Hugetlb: Test to detect bug with freeing gigantic hugetlb pages

[Tarun Sahu]

Before kernel version 5.10-rc7, there was a bug that resulted in a
"Bad Page State" error when freeing gigantic hugepages. This happened
because the struct page entry compound_nr, which overlapped with
page->mapping in the first tail page, was not cleared, causing the
error. To ensure that this issue does not reoccur as struct page keeps
changes and some fields are managed by folio, this test checks that
freeing gigantic hugepages does not produce the above-mentioned error.

Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
---
 .../kernel/mem/hugetlb/hugemmap/hugemmap32.c  | 85 +++++++++++++++++++
 testcases/kernel/mem/hugetlb/lib/hugetlb.h    |  6 ++
 2 files changed, 91 insertions(+)
 create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap32.c

diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap32.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap32.c
new file mode 100644
index 000000000..6b90d2546
--- /dev/null
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap32.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023, IBM Corporation.
+ * Author: Tarun Sahu
+ */
+
+/*\
+ * [Description]
+ *
+ * Before kernel version 5.10-rc7, there was a bug that resulted in a "Bad Page
+ * State" error when freeing gigantic hugepages. This happened because the
+ * struct page entry compound_nr, which overlapped with page->mapping in the
+ * first tail page, was not cleared, causing the error. To ensure that this
+ * issue does not reoccur as struct page keeps changing and some fields are
+ * managed by folio, this test checks that freeing gigantic hugepages does not
+ * produce the above-mentioned error.
+ */
+
+#define _GNU_SOURCE
+#include <dirent.h>
+
+#include <stdio.h>
+
+#include "hugetlb.h"
+
+#define PATH_GIGANTIC_HUGEPAGE "/sys/kernel/mm/hugepages"
+#define GIGANTIC_MIN_ORDER 10
+
+static int org_g_hpages;
+static char g_hpage_path[4096];
+
+static void run_test(void)
+{
+	if (FILE_PRINTF(g_hpage_path, "%d", 1))
+		tst_brk(TCONF, "Can't update the gigantic hugepages.");
+	SAFE_FILE_PRINTF(g_hpage_path, "%d", 0);
+
+	if (tst_taint_check())
+		tst_res(TFAIL, "Freeing Gigantic pages resulted in Bad Page State bug.");
+	else
+		tst_res(TPASS, "Successfully freed the gigantic hugepages");
+}
+
+static void setup(void)
+{
+	DIR *dir;
+	struct dirent *ent;
+	unsigned long hpage_size;
+
+	dir = SAFE_OPENDIR(PATH_GIGANTIC_HUGEPAGE);
+	while ((ent = SAFE_READDIR(dir))) {
+		if (strstr(ent->d_name, "hugepages-") != NULL) {
+			if ((sscanf(ent->d_name, "hugepages-%lukB", &hpage_size) == 1) &&
+				is_hugetlb_gigantic(hpage_size * 1024)) {
+				sprintf(g_hpage_path, "%s/%s/%s", PATH_GIGANTIC_HUGEPAGE,
+						ent->d_name, "nr_hugepages");
+				break;
+			}
+		}
+	}
+	if (!g_hpage_path[0])
+		tst_brk(TCONF, "Gigantic hugepages not supported");
+
+	SAFE_CLOSEDIR(dir);
+	SAFE_FILE_LINES_SCANF(g_hpage_path, "%d", &org_g_hpages);
+}
+
+static void cleanup(void)
+{
+	if (g_hpage_path[0])
+		SAFE_FILE_PRINTF(g_hpage_path, "%d", org_g_hpages);
+}
+
+static struct tst_test test = {
+	.tags = (struct tst_tag[]) {
+	    {"linux-git", "ba9c1201beaa"},
+	    {"linux-git", "7118fc2906e9"},
+	    {}
+	},
+	.needs_root = 1,
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = run_test,
+	.taint_check = TST_TAINT_B,
+};
diff --git a/testcases/kernel/mem/hugetlb/lib/hugetlb.h b/testcases/kernel/mem/hugetlb/lib/hugetlb.h
index 241dab708..34fe08c24 100644
--- a/testcases/kernel/mem/hugetlb/lib/hugetlb.h
+++ b/testcases/kernel/mem/hugetlb/lib/hugetlb.h
@@ -39,6 +39,12 @@
 # endif
 #endif
 
+/* Check if hugetlb page is gigantic */
+static inline int is_hugetlb_gigantic(unsigned long hpage_size)
+{
+	return (hpage_size / getpagesize()) >> 11;
+}
+
 /*
  * to get the lower nine permission bits
  * from shmid_ds.ipc_perm.mode
-- 
2.31.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] Hugetlb: Test to detect bug with freeing gigantic hugetlb pages

[Cyril Hrubis]

Hi!
> +#define _GNU_SOURCE
> +#include <dirent.h>
> +
> +#include <stdio.h>
> +
> +#include "hugetlb.h"
> +
> +#define PATH_GIGANTIC_HUGEPAGE "/sys/kernel/mm/hugepages"
> +#define GIGANTIC_MIN_ORDER 10
> +
> +static int org_g_hpages;
> +static char g_hpage_path[4096];
> +
> +static void run_test(void)
> +{
> +	if (FILE_PRINTF(g_hpage_path, "%d", 1))
> +		tst_brk(TCONF, "Can't update the gigantic hugepages.");
> +	SAFE_FILE_PRINTF(g_hpage_path, "%d", 0);
> +
> +	if (tst_taint_check())
> +		tst_res(TFAIL, "Freeing Gigantic pages resulted in Bad Page State bug.");
> +	else
> +		tst_res(TPASS, "Successfully freed the gigantic hugepages");
> +}
> +
> +static void setup(void)
> +{
> +	DIR *dir;
> +	struct dirent *ent;
> +	unsigned long hpage_size;
> +
> +	dir = SAFE_OPENDIR(PATH_GIGANTIC_HUGEPAGE);
                                 ^
				 This is very minor, but there is
				 nothing gigantic about the path, that's
				 just sysfs hugepates directory, so I
				 suppose that it should be just
				 PATH_HUGEPAGE

> +	while ((ent = SAFE_READDIR(dir))) {
> +		if (strstr(ent->d_name, "hugepages-") != NULL) {

Isn't the strstr() here reduntant?

I as far as I can tell if the line in sscanf() will not match the call
will simply return 0.

> +			if ((sscanf(ent->d_name, "hugepages-%lukB", &hpage_size) == 1) &&
> +				is_hugetlb_gigantic(hpage_size * 1024)) {
> +				sprintf(g_hpage_path, "%s/%s/%s", PATH_GIGANTIC_HUGEPAGE,
> +						ent->d_name, "nr_hugepages");
> +				break;
> +			}
> +		}
> +	}
> +	if (!g_hpage_path[0])
> +		tst_brk(TCONF, "Gigantic hugepages not supported");
> +
> +	SAFE_CLOSEDIR(dir);
> +	SAFE_FILE_LINES_SCANF(g_hpage_path, "%d", &org_g_hpages);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (g_hpage_path[0])
> +		SAFE_FILE_PRINTF(g_hpage_path, "%d", org_g_hpages);
> +}
> +
> +static struct tst_test test = {
> +	.tags = (struct tst_tag[]) {
> +	    {"linux-git", "ba9c1201beaa"},
> +	    {"linux-git", "7118fc2906e9"},
                            ^
			    This has appears to be wrong. Shouldn't the
			    last digit be 2 instead of 9?
> +	    {}
> +	},
> +	.needs_root = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run_test,
> +	.taint_check = TST_TAINT_B,
> +};
> diff --git a/testcases/kernel/mem/hugetlb/lib/hugetlb.h b/testcases/kernel/mem/hugetlb/lib/hugetlb.h
> index 241dab708..34fe08c24 100644
> --- a/testcases/kernel/mem/hugetlb/lib/hugetlb.h
> +++ b/testcases/kernel/mem/hugetlb/lib/hugetlb.h
> @@ -39,6 +39,12 @@
>  # endif
>  #endif
>  
> +/* Check if hugetlb page is gigantic */
> +static inline int is_hugetlb_gigantic(unsigned long hpage_size)
> +{
> +	return (hpage_size / getpagesize()) >> 11;
> +}
> +
>  /*
>   * to get the lower nine permission bits
>   * from shmid_ds.ipc_perm.mode
> -- 
> 2.31.1
> 

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] Hugetlb: Test to detect bug with freeing gigantic hugetlb pages

[Tarun Sahu]

Hi Cyril,

Thanks for looking at it.
I agree to your comments, sent the v3. Also second commit tag was wrong.
I changed it too.

~Tarun

> Hi!
>> +#define _GNU_SOURCE
>> +#include <dirent.h>
>> +
>> +#include <stdio.h>
>> +
>> +#include "hugetlb.h"
>> +
>> +#define PATH_GIGANTIC_HUGEPAGE "/sys/kernel/mm/hugepages"
>> +#define GIGANTIC_MIN_ORDER 10
>> +
>> +static int org_g_hpages;
>> +static char g_hpage_path[4096];
>> +
>> +static void run_test(void)
>> +{
>> +	if (FILE_PRINTF(g_hpage_path, "%d", 1))
>> +		tst_brk(TCONF, "Can't update the gigantic hugepages.");
>> +	SAFE_FILE_PRINTF(g_hpage_path, "%d", 0);
>> +
>> +	if (tst_taint_check())
>> +		tst_res(TFAIL, "Freeing Gigantic pages resulted in Bad Page State bug.");
>> +	else
>> +		tst_res(TPASS, "Successfully freed the gigantic hugepages");
>> +}
>> +
>> +static void setup(void)
>> +{
>> +	DIR *dir;
>> +	struct dirent *ent;
>> +	unsigned long hpage_size;
>> +
>> +	dir = SAFE_OPENDIR(PATH_GIGANTIC_HUGEPAGE);
>                                  ^
> 				 This is very minor, but there is
> 				 nothing gigantic about the path, that's
> 				 just sysfs hugepates directory, so I
> 				 suppose that it should be just
> 				 PATH_HUGEPAGE
>
>> +	while ((ent = SAFE_READDIR(dir))) {
>> +		if (strstr(ent->d_name, "hugepages-") != NULL) {
>
> Isn't the strstr() here reduntant?
>
> I as far as I can tell if the line in sscanf() will not match the call
> will simply return 0.
>
>> +			if ((sscanf(ent->d_name, "hugepages-%lukB", &hpage_size) == 1) &&
>> +				is_hugetlb_gigantic(hpage_size * 1024)) {
>> +				sprintf(g_hpage_path, "%s/%s/%s", PATH_GIGANTIC_HUGEPAGE,
>> +						ent->d_name, "nr_hugepages");
>> +				break;
>> +			}
>> +		}
>> +	}
>> +	if (!g_hpage_path[0])
>> +		tst_brk(TCONF, "Gigantic hugepages not supported");
>> +
>> +	SAFE_CLOSEDIR(dir);
>> +	SAFE_FILE_LINES_SCANF(g_hpage_path, "%d", &org_g_hpages);
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	if (g_hpage_path[0])
>> +		SAFE_FILE_PRINTF(g_hpage_path, "%d", org_g_hpages);
>> +}
>> +
>> +static struct tst_test test = {
>> +	.tags = (struct tst_tag[]) {
>> +	    {"linux-git", "ba9c1201beaa"},
>> +	    {"linux-git", "7118fc2906e9"},
>                             ^
> 			    This has appears to be wrong. Shouldn't the
> 			    last digit be 2 instead of 9?
>> +	    {}
>> +	},
>> +	.needs_root = 1,
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.test_all = run_test,
>> +	.taint_check = TST_TAINT_B,
>> +};
>> diff --git a/testcases/kernel/mem/hugetlb/lib/hugetlb.h b/testcases/kernel/mem/hugetlb/lib/hugetlb.h
>> index 241dab708..34fe08c24 100644
>> --- a/testcases/kernel/mem/hugetlb/lib/hugetlb.h
>> +++ b/testcases/kernel/mem/hugetlb/lib/hugetlb.h
>> @@ -39,6 +39,12 @@
>>  # endif
>>  #endif
>>  
>> +/* Check if hugetlb page is gigantic */
>> +static inline int is_hugetlb_gigantic(unsigned long hpage_size)
>> +{
>> +	return (hpage_size / getpagesize()) >> 11;
>> +}
>> +
>>  /*
>>   * to get the lower nine permission bits
>>   * from shmid_ds.ipc_perm.mode
>> -- 
>> 2.31.1
>> 
>
> -- 
> Cyril Hrubis
> chrubis@suse.cz
>
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp