diff mbox series

[v4,1/4] mm: hugetlb_vmemmap: introduce STRUCT_PAGE_SIZE_IS_POWER_OF_2

Message ID 20220318100720.14524-2-songmuchun@bytedance.com (mailing list archive)
State New
Headers show
Series add hugetlb_free_vmemmap sysctl | expand

Commit Message

Muchun Song March 18, 2022, 10:07 a.m. UTC 
If the size of "struct page" is not the power of two and this
feature is enabled, then the vmemmap pages of HugeTLB will be
corrupted after remapping (panic is about to happen in theory).
But this only exists when !CONFIG_MEMCG && !CONFIG_SLUB on
x86_64.  However, it is not a conventional configuration nowadays.
So it is not a real word issue, just the result of a code review.
But we have to prevent anyone from configuring that combined
configuration.  In order to avoid many checks like "is_power_of_2
(sizeof(struct page))" through mm/hugetlb_vmemmap.c.  Introduce
STRUCT_PAGE_SIZE_IS_POWER_OF_2 to detect if the size of struct
page is power of 2 and make this feature depends on this new
config.  Then we could prevent anyone do any unexpected
configuration.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 Kbuild                           | 12 ++++++++++++
 fs/Kconfig                       |  2 +-
 include/linux/mm_types.h         |  2 ++
 mm/Kconfig                       |  3 +++
 mm/hugetlb_vmemmap.c             |  6 ------
 mm/struct_page_size.c            | 19 +++++++++++++++++++
 scripts/check_struct_page_po2.sh | 11 +++++++++++
 7 files changed, 48 insertions(+), 7 deletions(-)
 create mode 100644 mm/struct_page_size.c
 create mode 100755 scripts/check_struct_page_po2.sh

Comments

Luis Chamberlain March 18, 2022, 4:40 p.m. UTC | #1 
On Fri, Mar 18, 2022 at 06:07:17PM +0800, Muchun Song wrote:

You can add Suggested-by tag here.

> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  Kbuild                           | 12 ++++++++++++
>  fs/Kconfig                       |  2 +-
>  include/linux/mm_types.h         |  2 ++
>  mm/Kconfig                       |  3 +++
>  mm/hugetlb_vmemmap.c             |  6 ------
>  mm/struct_page_size.c            | 19 +++++++++++++++++++
>  scripts/check_struct_page_po2.sh | 11 +++++++++++
>  7 files changed, 48 insertions(+), 7 deletions(-)
>  create mode 100644 mm/struct_page_size.c
>  create mode 100755 scripts/check_struct_page_po2.sh
> 
> diff --git a/Kbuild b/Kbuild
> index fa441b98c9f6..6bb97d348d62 100644
> --- a/Kbuild
> +++ b/Kbuild
> @@ -14,6 +14,18 @@ $(bounds-file): kernel/bounds.s FORCE
>  	$(call filechk,offsets,__LINUX_BOUNDS_H__)
>  
>  #####
> +# Generate struct_page_size.h. Must follows bounds.h.
> +
> +struct_page_size-file := include/generated/struct_page_size.h
> +
> +always-y := $(struct_page_size-file)
> +targets := mm/struct_page_size.s
> +
> +$(struct_page_size-file): mm/struct_page_size.s FORCE
> +	$(call filechk,offsets,__LINUX_STRUCT_PAGE_SIZE_H__)
> +	$(Q)$(MAKE) -f $(srctree)/Makefile syncconfig
> +
> +#####

Shouldn't this go into mm/Makefile instead?

> diff --git a/mm/Kconfig b/mm/Kconfig
> index 034d87953600..9314bd34f49e 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -2,6 +2,9 @@
>  
>  menu "Memory Management options"
>  
> +config STRUCT_PAGE_SIZE_IS_POWER_OF_2
> +	def_bool $(success,test "$(shell, $(srctree)/scripts/check_struct_page_po2.sh)" = 1)
> +
>  config SELECT_MEMORY_MODEL
>  	def_bool y
>  	depends on ARCH_SELECT_MEMORY_MODEL
> new file mode 100755
> index 000000000000..9547ad3aca05
> --- /dev/null
> +++ b/scripts/check_struct_page_po2.sh
> @@ -0,0 +1,11 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Check if the size of "struct page" is power of 2
> +
> +file="include/generated/struct_page_size.h"
> +if [ ! -f "$file" ]; then

Does this really work if one is workig off of a very clean build
like make mrproper and then make menuconfig or or mrproper followed
by a defconfig file ? Have you tried it for both cases po2 and npo2?

Because isn't include/generated/struct_page_size.h generated? At
which point does it get generated and why would the condition hole
true that the file exists at a new 'make menuconfig' time?

  Luis
Muchun Song March 19, 2022, 3:56 a.m. UTC | #2 
On Sat, Mar 19, 2022 at 12:40 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Fri, Mar 18, 2022 at 06:07:17PM +0800, Muchun Song wrote:
>
> You can add Suggested-by tag here.

Will do. Sorry for forgetting it.

>
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  Kbuild                           | 12 ++++++++++++
> >  fs/Kconfig                       |  2 +-
> >  include/linux/mm_types.h         |  2 ++
> >  mm/Kconfig                       |  3 +++
> >  mm/hugetlb_vmemmap.c             |  6 ------
> >  mm/struct_page_size.c            | 19 +++++++++++++++++++
> >  scripts/check_struct_page_po2.sh | 11 +++++++++++
> >  7 files changed, 48 insertions(+), 7 deletions(-)
> >  create mode 100644 mm/struct_page_size.c
> >  create mode 100755 scripts/check_struct_page_po2.sh
> >
> > diff --git a/Kbuild b/Kbuild
> > index fa441b98c9f6..6bb97d348d62 100644
> > --- a/Kbuild
> > +++ b/Kbuild
> > @@ -14,6 +14,18 @@ $(bounds-file): kernel/bounds.s FORCE
> >       $(call filechk,offsets,__LINUX_BOUNDS_H__)
> >
> >  #####
> > +# Generate struct_page_size.h. Must follows bounds.h.
> > +
> > +struct_page_size-file := include/generated/struct_page_size.h
> > +
> > +always-y := $(struct_page_size-file)
> > +targets := mm/struct_page_size.s
> > +
> > +$(struct_page_size-file): mm/struct_page_size.s FORCE
> > +     $(call filechk,offsets,__LINUX_STRUCT_PAGE_SIZE_H__)
> > +     $(Q)$(MAKE) -f $(srctree)/Makefile syncconfig
> > +
> > +#####
>
> Shouldn't this go into mm/Makefile instead?
>

We should guarantee that the include/generated/struct_page_size.h
which Kconfig depends on is created before processing Makefile
since processing Kconfig is before Makefile. Right?

> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index 034d87953600..9314bd34f49e 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -2,6 +2,9 @@
> >
> >  menu "Memory Management options"
> >
> > +config STRUCT_PAGE_SIZE_IS_POWER_OF_2
> > +     def_bool $(success,test "$(shell, $(srctree)/scripts/check_struct_page_po2.sh)" = 1)
> > +
> >  config SELECT_MEMORY_MODEL
> >       def_bool y
> >       depends on ARCH_SELECT_MEMORY_MODEL
> > new file mode 100755
> > index 000000000000..9547ad3aca05
> > --- /dev/null
> > +++ b/scripts/check_struct_page_po2.sh
> > @@ -0,0 +1,11 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Check if the size of "struct page" is power of 2
> > +
> > +file="include/generated/struct_page_size.h"
> > +if [ ! -f "$file" ]; then
>
> Does this really work if one is workig off of a very clean build
> like make mrproper and then make menuconfig or or mrproper followed
> by a defconfig file ? Have you tried it for both cases po2 and npo2?
>
> Because isn't include/generated/struct_page_size.h generated? At
> which point does it get generated and why would the condition hole
> true that the file exists at a new 'make menuconfig' time?
>

You are right. include/generated/struct_page_size.h does not exist
in this case, CONFIG_STRUCT_PAGE_SIZE_IS_POWER_OF_2
will be default off in .config. Then it will be switched on/off accordingly
when you build the kernel, for instance make bzImage, which is done via
"make syncconfig" which follows the generation of struct_page_size.h.

While testing this case, I found some bugs. The following patch
could fix this.  Thanks.

--- a/Kbuild
+++ b/Kbuild
@@ -21,6 +21,8 @@ struct_page_size-file := include/generated/struct_page_size.h
 always-y := $(struct_page_size-file)
 targets := mm/struct_page_size.s

+mm/struct_page_size.s: $(timeconst-file) $(bounds-file)
+
 $(struct_page_size-file): mm/struct_page_size.s FORCE
        $(call filechk,offsets,__LINUX_STRUCT_PAGE_SIZE_H__)
        $(Q)$(MAKE) -f $(srctree)/Makefile syncconfig
diff --git a/scripts/check_struct_page_po2.sh b/scripts/check_struct_page_po2.sh
index 9547ad3aca05..1764ef9a4f1d 100755
--- a/scripts/check_struct_page_po2.sh
+++ b/scripts/check_struct_page_po2.sh
@@ -4,8 +4,6 @@
 # Check if the size of "struct page" is power of 2

 file="include/generated/struct_page_size.h"
-if [ ! -f "$file" ]; then
-       exit 1
+if [ -f "$file" ]; then
+       grep STRUCT_PAGE_SIZE_IS_POWER_OF_2 "$file" | cut -d' ' -f3
 fi
-
-grep STRUCT_PAGE_SIZE_IS_POWER_OF_2 "$file" | cut -d' ' -f3
diff mbox series

Patch

diff --git a/Kbuild b/Kbuild
index fa441b98c9f6..6bb97d348d62 100644
--- a/Kbuild
+++ b/Kbuild
@@ -14,6 +14,18 @@  $(bounds-file): kernel/bounds.s FORCE
 	$(call filechk,offsets,__LINUX_BOUNDS_H__)
 
 #####
+# Generate struct_page_size.h. Must follows bounds.h.
+
+struct_page_size-file := include/generated/struct_page_size.h
+
+always-y := $(struct_page_size-file)
+targets := mm/struct_page_size.s
+
+$(struct_page_size-file): mm/struct_page_size.s FORCE
+	$(call filechk,offsets,__LINUX_STRUCT_PAGE_SIZE_H__)
+	$(Q)$(MAKE) -f $(srctree)/Makefile syncconfig
+
+#####
 # Generate timeconst.h
 
 timeconst-file := include/generated/timeconst.h
diff --git a/fs/Kconfig b/fs/Kconfig
index 7f2455e8e18a..b8b722f7f773 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -248,7 +248,7 @@  config HUGETLB_PAGE
 config HUGETLB_PAGE_FREE_VMEMMAP
 	def_bool HUGETLB_PAGE
 	depends on X86_64
-	depends on SPARSEMEM_VMEMMAP
+	depends on SPARSEMEM_VMEMMAP && STRUCT_PAGE_SIZE_IS_POWER_OF_2
 
 config HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON
 	bool "Default freeing vmemmap pages of HugeTLB to on"
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 8834e38c06a4..b4defcea6534 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -223,6 +223,7 @@  struct page {
 #endif
 } _struct_page_alignment;
 
+#ifndef __GENERATING_STRUCT_PAGE_SIZE_IS_POWER_OF_2_H
 /**
  * struct folio - Represents a contiguous set of bytes.
  * @flags: Identical to the page flags.
@@ -844,5 +845,6 @@  enum fault_flag {
 	FAULT_FLAG_INSTRUCTION =	1 << 8,
 	FAULT_FLAG_INTERRUPTIBLE =	1 << 9,
 };
+#endif /* __GENERATING_STRUCT_PAGE_SIZE_IS_POWER_OF_2_H */
 
 #endif /* _LINUX_MM_TYPES_H */
diff --git a/mm/Kconfig b/mm/Kconfig
index 034d87953600..9314bd34f49e 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -2,6 +2,9 @@ 
 
 menu "Memory Management options"
 
+config STRUCT_PAGE_SIZE_IS_POWER_OF_2
+	def_bool $(success,test "$(shell, $(srctree)/scripts/check_struct_page_po2.sh)" = 1)
+
 config SELECT_MEMORY_MODEL
 	def_bool y
 	depends on ARCH_SELECT_MEMORY_MODEL
diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
index 791626983c2e..33ecb77c2b2a 100644
--- a/mm/hugetlb_vmemmap.c
+++ b/mm/hugetlb_vmemmap.c
@@ -194,12 +194,6 @@  EXPORT_SYMBOL(hugetlb_free_vmemmap_enabled_key);
 
 static int __init early_hugetlb_free_vmemmap_param(char *buf)
 {
-	/* We cannot optimize if a "struct page" crosses page boundaries. */
-	if (!is_power_of_2(sizeof(struct page))) {
-		pr_warn("cannot free vmemmap pages because \"struct page\" crosses page boundaries\n");
-		return 0;
-	}
-
 	if (!buf)
 		return -EINVAL;
 
diff --git a/mm/struct_page_size.c b/mm/struct_page_size.c
new file mode 100644
index 000000000000..5749609aa1b3
--- /dev/null
+++ b/mm/struct_page_size.c
@@ -0,0 +1,19 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generate definitions needed by the preprocessor.
+ * This code generates raw asm output which is post-processed
+ * to extract and format the required data.
+ */
+
+#define __GENERATING_STRUCT_PAGE_SIZE_IS_POWER_OF_2_H
+/* Include headers that define the enum constants of interest */
+#include <linux/mm_types.h>
+#include <linux/kbuild.h>
+#include <linux/log2.h>
+
+int main(void)
+{
+	DEFINE(STRUCT_PAGE_SIZE_IS_POWER_OF_2, is_power_of_2(sizeof(struct page)));
+
+	return 0;
+}
diff --git a/scripts/check_struct_page_po2.sh b/scripts/check_struct_page_po2.sh
new file mode 100755
index 000000000000..9547ad3aca05
--- /dev/null
+++ b/scripts/check_struct_page_po2.sh
@@ -0,0 +1,11 @@ 
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# Check if the size of "struct page" is power of 2
+
+file="include/generated/struct_page_size.h"
+if [ ! -f "$file" ]; then
+	exit 1
+fi
+
+grep STRUCT_PAGE_SIZE_IS_POWER_OF_2 "$file" | cut -d' ' -f3