[PATCH 0/3] page table check default to warn instead of panic

[PATCH 0/3] page table check default to warn instead of panic

[Pasha Tatashin]

From: Pasha Tatashin <tatashin@google.com>

Page table check when detects errors panics the kernel. Let instead,
print a warning, and panic only when specifically requested via kernel
parameter:

	page_table_check=panic

The discussion about using panic vs. warn is here:
https://lore.kernel.org/linux-mm/20220902232732.12358-1-rick.p.edgecombe@intel.com

Pasha Tatashin (2):
  mm/page_table_check: Do WARN_ON instead of BUG_ON by default
  doc/vm: add information about page_table_check=panic

Rick Edgecombe (1):
  mm/page_table_check: Check writable zero page in page table check

 Documentation/mm/page_table_check.rst | 16 ++++----
 mm/page_table_check.c                 | 53 ++++++++++++++++++++-------
 2 files changed, 49 insertions(+), 20 deletions(-)

-- 
2.37.2.789.g6183377224-goog

[PATCH 1/3] mm/page_table_check: Check writable zero page in page table check

[Pasha Tatashin]

From: Rick Edgecombe <rick.p.edgecombe@intel.com>

The zero page should remain all zero, so that it can be mapped as
read-only for read faults of memory that should be zeroed. If it is ever
mapped writable to userspace, it could become non-zero and so other apps
would unexpectedly get non-zero data. So the zero page should never be
mapped writable to userspace. Check for this condition in
page_table_check_set().

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
---
 mm/page_table_check.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/page_table_check.c b/mm/page_table_check.c
index e2062748791a..665ece0d55d4 100644
--- a/mm/page_table_check.c
+++ b/mm/page_table_check.c
@@ -102,6 +102,8 @@ static void page_table_check_set(struct mm_struct *mm, unsigned long addr,
 	if (!pfn_valid(pfn))
 		return;
 
+	BUG_ON(is_zero_pfn(pfn) && rw);
+
 	page = pfn_to_page(pfn);
 	page_ext = lookup_page_ext(page);
 	anon = PageAnon(page);
-- 
2.37.2.789.g6183377224-goog

[PATCH 2/3] mm/page_table_check: Do WARN_ON instead of BUG_ON by default

[Pasha Tatashin]

Currently, page_table_check when detects errors panics kernel. Instead,
print a warning, and panic only when specifically requested via kernel
parameter:

	page_table_check=panic

Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
---
 mm/page_table_check.c | 53 +++++++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 14 deletions(-)

diff --git a/mm/page_table_check.c b/mm/page_table_check.c
index 665ece0d55d4..881f19d0714c 100644
--- a/mm/page_table_check.c
+++ b/mm/page_table_check.c
@@ -17,13 +17,37 @@ struct page_table_check {
 
 static bool __page_table_check_enabled __initdata =
 				IS_ENABLED(CONFIG_PAGE_TABLE_CHECK_ENFORCED);
+static bool __page_table_check_panic;
 
 DEFINE_STATIC_KEY_TRUE(page_table_check_disabled);
 EXPORT_SYMBOL(page_table_check_disabled);
 
+#define PAGE_TABLE_CHECK_BUG(v)							\
+	do {									\
+		bool __bug = !!(v);						\
+										\
+		if (__page_table_check_panic)					\
+			BUG_ON(__bug);						\
+		else if (WARN_ON_ONCE(__bug))					\
+			static_branch_enable(&page_table_check_disabled);	\
+	} while (false)
+
 static int __init early_page_table_check_param(char *buf)
 {
-	return strtobool(buf, &__page_table_check_enabled);
+	int rc = strtobool(buf, &__page_table_check_enabled);
+
+	if (rc) {
+		if (!strcmp(buf, "panic")) {
+			__page_table_check_enabled = true;
+			__page_table_check_panic = true;
+			rc = 0;
+		}
+	}
+
+	if (rc)
+		pr_warn("Invalid option string: '%s'\n", buf);
+
+	return rc;
 }
 
 early_param("page_table_check", early_page_table_check_param);
@@ -48,7 +72,8 @@ struct page_ext_operations page_table_check_ops = {
 
 static struct page_table_check *get_page_table_check(struct page_ext *page_ext)
 {
-	BUG_ON(!page_ext);
+	PAGE_TABLE_CHECK_BUG(!page_ext);
+
 	return (void *)(page_ext) + page_table_check_ops.offset;
 }
 
@@ -75,11 +100,11 @@ static void page_table_check_clear(struct mm_struct *mm, unsigned long addr,
 		struct page_table_check *ptc = get_page_table_check(page_ext);
 
 		if (anon) {
-			BUG_ON(atomic_read(&ptc->file_map_count));
-			BUG_ON(atomic_dec_return(&ptc->anon_map_count) < 0);
+			PAGE_TABLE_CHECK_BUG(atomic_read(&ptc->file_map_count));
+			PAGE_TABLE_CHECK_BUG(atomic_dec_return(&ptc->anon_map_count) < 0);
 		} else {
-			BUG_ON(atomic_read(&ptc->anon_map_count));
-			BUG_ON(atomic_dec_return(&ptc->file_map_count) < 0);
+			PAGE_TABLE_CHECK_BUG(atomic_read(&ptc->anon_map_count));
+			PAGE_TABLE_CHECK_BUG(atomic_dec_return(&ptc->file_map_count) < 0);
 		}
 		page_ext = page_ext_next(page_ext);
 	}
@@ -102,7 +127,7 @@ static void page_table_check_set(struct mm_struct *mm, unsigned long addr,
 	if (!pfn_valid(pfn))
 		return;
 
-	BUG_ON(is_zero_pfn(pfn) && rw);
+	PAGE_TABLE_CHECK_BUG(!is_zero_pfn(pfn) && rw);
 
 	page = pfn_to_page(pfn);
 	page_ext = lookup_page_ext(page);
@@ -112,11 +137,11 @@ static void page_table_check_set(struct mm_struct *mm, unsigned long addr,
 		struct page_table_check *ptc = get_page_table_check(page_ext);
 
 		if (anon) {
-			BUG_ON(atomic_read(&ptc->file_map_count));
-			BUG_ON(atomic_inc_return(&ptc->anon_map_count) > 1 && rw);
+			PAGE_TABLE_CHECK_BUG(atomic_read(&ptc->file_map_count));
+			PAGE_TABLE_CHECK_BUG(atomic_inc_return(&ptc->anon_map_count) > 1 && rw);
 		} else {
-			BUG_ON(atomic_read(&ptc->anon_map_count));
-			BUG_ON(atomic_inc_return(&ptc->file_map_count) < 0);
+			PAGE_TABLE_CHECK_BUG(atomic_read(&ptc->anon_map_count));
+			PAGE_TABLE_CHECK_BUG(atomic_inc_return(&ptc->file_map_count) < 0);
 		}
 		page_ext = page_ext_next(page_ext);
 	}
@@ -131,12 +156,12 @@ void __page_table_check_zero(struct page *page, unsigned int order)
 	struct page_ext *page_ext = lookup_page_ext(page);
 	unsigned long i;
 
-	BUG_ON(!page_ext);
+	PAGE_TABLE_CHECK_BUG(!page_ext);
 	for (i = 0; i < (1ul << order); i++) {
 		struct page_table_check *ptc = get_page_table_check(page_ext);
 
-		BUG_ON(atomic_read(&ptc->anon_map_count));
-		BUG_ON(atomic_read(&ptc->file_map_count));
+		PAGE_TABLE_CHECK_BUG(atomic_read(&ptc->anon_map_count));
+		PAGE_TABLE_CHECK_BUG(atomic_read(&ptc->file_map_count));
 		page_ext = page_ext_next(page_ext);
 	}
 }
-- 
2.37.2.789.g6183377224-goog

[PATCH 3/3] doc/vm: add information about page_table_check=panic

[Pasha Tatashin]

The default behavior of page table check was changed from panicking
kernel to printing a warning.

Add a note how to still panic the kernel when error is detected.

Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
---
 Documentation/mm/page_table_check.rst | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/Documentation/mm/page_table_check.rst b/Documentation/mm/page_table_check.rst
index 1a09472f10a3..9306cd75647c 100644
--- a/Documentation/mm/page_table_check.rst
+++ b/Documentation/mm/page_table_check.rst
@@ -16,13 +16,13 @@ Page table check performs extra verifications at the time when new pages become
 accessible from the userspace by getting their page table entries (PTEs PMDs
 etc.) added into the table.
 
-In case of detected corruption, the kernel is crashed. There is a small
-performance and memory overhead associated with the page table check. Therefore,
-it is disabled by default, but can be optionally enabled on systems where the
-extra hardening outweighs the performance costs. Also, because page table check
-is synchronous, it can help with debugging double map memory corruption issues,
-by crashing kernel at the time wrong mapping occurs instead of later which is
-often the case with memory corruptions bugs.
+In case of detected corruption, a warning is printed or kernel is crashed. There
+is a small performance and memory overhead associated with the page table check.
+Therefore, it is disabled by default, but can be optionally enabled on systems
+where the extra hardening outweighs the performance costs. Also, because page
+table check is synchronous, it can help with debugging double map memory
+corruption issues, by crashing kernel at the time wrong mapping occurs instead
+of later which is often the case with memory corruptions bugs.
 
 Double mapping detection logic
 ==============================
@@ -52,5 +52,7 @@ Build kernel with:
 
 - Boot with 'page_table_check=on' kernel parameter.
 
+- Boot with 'page_table_check=panic' in order to panic when error is detected.
+
 Optionally, build kernel with PAGE_TABLE_CHECK_ENFORCED in order to have page
 table support without extra kernel parameter.
-- 
2.37.2.789.g6183377224-goog

Re: [PATCH 2/3] mm/page_table_check: Do WARN_ON instead of BUG_ON by default

[Matthew Wilcox]

On Sun, Sep 11, 2022 at 09:59:22AM +0000, Pasha Tatashin wrote:
> Currently, page_table_check when detects errors panics kernel. Instead,
> print a warning, and panic only when specifically requested via kernel
> parameter:
> 
> 	page_table_check=panic

Why are the page table checks so special that they deserve their own
command line parameter?  Why shouldn't this be controlled by the usual
panic_on_warn option?

Re: [PATCH 2/3] mm/page_table_check: Do WARN_ON instead of BUG_ON by default

[Pasha Tatashin]

On Sun, Sep 11, 2022 at 12:08 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Sun, Sep 11, 2022 at 09:59:22AM +0000, Pasha Tatashin wrote:
> > Currently, page_table_check when detects errors panics kernel. Instead,
> > print a warning, and panic only when specifically requested via kernel
> > parameter:
> >
> >       page_table_check=panic
>
> Why are the page table checks so special that they deserve their own
> command line parameter?  Why shouldn't this be controlled by the usual
> panic_on_warn option?

page_table_check can be used as a security feature preventing false
page sharing between address spaces. For example, at Google we want it
to keep enabled on production systems, yet we do not want to enable
panic_on_warn as it would cause panics for many other reasons which
are security unrelated.

Pasha

Re: [PATCH 1/3] mm/page_table_check: Check writable zero page in page table check

[Edgecombe, Rick P]

On Sun, 2022-09-11 at 09:59 +0000, Pasha Tatashin wrote:
> From: Rick Edgecombe <rick.p.edgecombe@intel.com>
> 
> The zero page should remain all zero, so that it can be mapped as
> read-only for read faults of memory that should be zeroed. If it is
> ever
> mapped writable to userspace, it could become non-zero and so other
> apps
> would unexpectedly get non-zero data. So the zero page should never
> be
> mapped writable to userspace. Check for this condition in
> page_table_check_set().
> 
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> ---
>  mm/page_table_check.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
Thanks. Should we put this at the end, in order to not add any more
BUG_ON()'s to the kernel? Or I can just send a follow up and add the
docs you asked for.

Re: [PATCH 0/3] page table check default to warn instead of panic

[Andrew Morton]

On Sun, 11 Sep 2022 09:59:20 +0000 Pasha Tatashin <pasha.tatashin@soleen.com> wrote:

> From: Pasha Tatashin <tatashin@google.com>
> 
> Page table check when detects errors panics the kernel. Let instead,
> print a warning, and panic only when specifically requested via kernel
> parameter:
> 
> 	page_table_check=panic
> 
> The discussion about using panic vs. warn is here:
> https://lore.kernel.org/linux-mm/20220902232732.12358-1-rick.p.edgecombe@intel.com

The changelog doesn't actually describe the reason for making this
change.  Somebody obviously wants pagetable check errors to no longer
panic the kernel, but why??  (The same can be said of the [2/3]
changelog).

Also, should we be changing the default?  People who like the panic
will get a big surprise when they find out that they should have added
a kernel parameter to get the old behaviour back.  It would be less
disruptive to default to panic unless page_table_check=warn was added.

If there's a solid reason for changing the default, it should be
changelogged.  And if that reason is generally agreed to, perhaps the
kernel should print a warning at boot if neither page_table_check=panic
nor page_table_check=warn were provided.  To tell people that the
default has been changed.

Re: [PATCH 0/3] page table check default to warn instead of panic

[Pasha Tatashin]

On Mon, Sep 12, 2022 at 4:23 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Sun, 11 Sep 2022 09:59:20 +0000 Pasha Tatashin <pasha.tatashin@soleen.com> wrote:
>
> > From: Pasha Tatashin <tatashin@google.com>
> >
> > Page table check when detects errors panics the kernel. Let instead,
> > print a warning, and panic only when specifically requested via kernel
> > parameter:
> >
> >       page_table_check=panic
> >
> > The discussion about using panic vs. warn is here:
> > https://lore.kernel.org/linux-mm/20220902232732.12358-1-rick.p.edgecombe@intel.com
>
> The changelog doesn't actually describe the reason for making this
> change.  Somebody obviously wants pagetable check errors to no longer
> panic the kernel, but why??  (The same can be said of the [2/3]
> changelog).

This came from the discussion listed above. There seems to be a
consensus that we should reduce the number of BUG_ON() in the kernel,
and replace them with WARN_ON_ONCE() when possible to recover. In the
case of page_table_check we can recover, but for some it may be unsafe
because of security implications. Therefore, I would like to keep  an
option of being able to panic only because of page table check errors,
but not keeping it enabled by default.

I will add more info to the commit message.

>
> Also, should we be changing the default?  People who like the panic
> will get a big surprise when they find out that they should have added
> a kernel parameter to get the old behaviour back.  It would be less
> disruptive to default to panic unless page_table_check=warn was added.

I was thinking about this as well. I decided to change the default:
the old users will still get a warning, but going forward we will be
inline with the rest of the kernel: warn on by default, and optionally
panic.

>
> If there's a solid reason for changing the default, it should be
> changelogged.  And if that reason is generally agreed to, perhaps the
> kernel should print a warning at boot if neither page_table_check=panic
> nor page_table_check=warn were provided.  To tell people that the
> default has been changed.

I am not sure that is needed, and when do we remove that extra boot
message? This is a relatively new feature, and existing users would
still get an ugly warning about incorrect page table mappings.

Thank you,
Pasha

>
>

Re: [PATCH 1/3] mm/page_table_check: Check writable zero page in page table check

[David Hildenbrand]

On 11.09.22 11:59, Pasha Tatashin wrote:
> From: Rick Edgecombe <rick.p.edgecombe@intel.com>
> 
> The zero page should remain all zero, so that it can be mapped as
> read-only for read faults of memory that should be zeroed. If it is ever
> mapped writable to userspace, it could become non-zero and so other apps
> would unexpectedly get non-zero data. So the zero page should never be
> mapped writable to userspace. Check for this condition in
> page_table_check_set().
> 
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> ---
>   mm/page_table_check.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/mm/page_table_check.c b/mm/page_table_check.c
> index e2062748791a..665ece0d55d4 100644
> --- a/mm/page_table_check.c
> +++ b/mm/page_table_check.c
> @@ -102,6 +102,8 @@ static void page_table_check_set(struct mm_struct *mm, unsigned long addr,
>   	if (!pfn_valid(pfn))
>   		return;
>   
> +	BUG_ON(is_zero_pfn(pfn) && rw);

We most probably don't want that:

https://lkml.kernel.org/r/20220923113426.52871-2-david@redhat.com

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/3] mm/page_table_check: Do WARN_ON instead of BUG_ON by default

[David Hildenbrand]

On 11.09.22 18:08, Matthew Wilcox wrote:
> On Sun, Sep 11, 2022 at 09:59:22AM +0000, Pasha Tatashin wrote:
>> Currently, page_table_check when detects errors panics kernel. Instead,
>> print a warning, and panic only when specifically requested via kernel
>> parameter:
>>
>> 	page_table_check=panic
> 
> Why are the page table checks so special that they deserve their own
> command line parameter?  Why shouldn't this be controlled by the usual
> panic_on_warn option?
> 

I agree.

https://lkml.kernel.org/r/20220923113426.52871-2-david@redhat.com

Use of WARN_ON_ONCE is the way to go nowadays.

-- 
Thanks,

David / dhildenb