diff mbox series

[UPDATE] mm: shmem: don't truncate page if memory failure happens

Message ID 20211114053221.315753-1-shy828301@gmail.com (mailing list archive)
State New
Headers show
Series [UPDATE] mm: shmem: don't truncate page if memory failure happens | expand

Commit Message

Yang Shi Nov. 14, 2021, 5:32 a.m. UTC 
The current behavior of memory failure is to truncate the page cache
regardless of dirty or clean.  If the page is dirty the later access
will get the obsolete data from disk without any notification to the
users.  This may cause silent data loss.  It is even worse for shmem
since shmem is in-memory filesystem, truncating page cache means
discarding data blocks.  The later read would return all zero.

The right approach is to keep the corrupted page in page cache, any
later access would return error for syscalls or SIGBUS for page fault,
until the file is truncated, hole punched or removed.  The regular
storage backed filesystems would be more complicated so this patch is
focused on shmem.  This also unblock the support for soft offlining
shmem THP.

[arnd@arndb.de: fix uninitialized variable use in me_pagecache_clean()]
  Link: https://lkml.kernel.org/r/20211022064748.4173718-1-arnd@kernel.org
[Fix invalid pointer dereference in shmem_read_mapping_page_gfp() with a
 slight different implementation from what Ajay Garg <ajaygargnsit@gmail.com>
 and Muchun Song <songmuchun@bytedance.com> proposed and reworked the
 error handling of shmem_write_begin() suggested by Linus]
  Link: https://lore.kernel.org/linux-mm/20211111084617.6746-1-ajaygargnsit@gmail.com/

Link: https://lkml.kernel.org/r/20211020210755.23964-6-shy828301@gmail.com
Signed-off-by: Yang Shi <shy828301@gmail.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Hugh Dickins <hughd@google.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Naoya Horiguchi <naoya.horiguchi@nec.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Peter Xu <peterx@redhat.com>
Cc: Ajay Garg <ajaygargnsit@gmail.com>
Cc: Muchun Song <songmuchun@bytedance.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
Fixed pointer dereference oops reported by Ajay Garg and reworked the
error handling of shmem_write_begin() suggested by Linus.

 mm/memory-failure.c | 14 ++++++++++---
 mm/shmem.c          | 48 +++++++++++++++++++++++++++++++++++++++------
 mm/userfaultfd.c    |  5 +++++
 3 files changed, 58 insertions(+), 9 deletions(-)

Comments

Matthew Wilcox Nov. 14, 2021, 3:28 p.m. UTC | #1 
On Sat, Nov 13, 2021 at 09:32:21PM -0800, Yang Shi wrote:
> @@ -2466,7 +2467,18 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
>  			return -EPERM;
>  	}
>  
> -	return shmem_getpage(inode, index, pagep, SGP_WRITE);
> +	ret = shmem_getpage(inode, index, pagep, SGP_WRITE);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (*pagep && PageHWPoison(*pagep)) {
> +		unlock_page(*pagep);
> +		put_page(*pagep);
> +		ret = -EIO;

You definitely need to add:

		*pagep = NULL;

I'm not entirely convinced that you need the conditional on '*pagep'.
If we returned 0, there had better be a page at pagep!

I also think this would be clearer if written as:

	if (PageHWPoison(*pagep)) {
		unlock_page(*pagep);
		put_page(*pagep);
		*pagep = NULL;
		return -EIO;
	}

	return 0;

instead of re-using ret.  Sometimes that can make the code flow clearer,
but here, I don't think it does.

> @@ -4168,9 +4201,12 @@ struct page *shmem_read_mapping_page_gfp(struct address_space *mapping,
>  	error = shmem_getpage_gfp(inode, index, &page, SGP_CACHE,
>  				  gfp, NULL, NULL, NULL);
>  	if (error)
> -		page = ERR_PTR(error);
> -	else
> -		unlock_page(page);
> +		return ERR_PTR(error);
> +
> +	unlock_page(page);
> +	if (PageHWPoison(page))
> +		return ERR_PTR(-EIO);

Do we need to put_page() the page in this error case?
Yang Shi Nov. 14, 2021, 4:54 p.m. UTC | #2 
On Sun, Nov 14, 2021 at 7:28 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Sat, Nov 13, 2021 at 09:32:21PM -0800, Yang Shi wrote:
> > @@ -2466,7 +2467,18 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
> >                       return -EPERM;
> >       }
> >
> > -     return shmem_getpage(inode, index, pagep, SGP_WRITE);
> > +     ret = shmem_getpage(inode, index, pagep, SGP_WRITE);
> > +
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (*pagep && PageHWPoison(*pagep)) {
> > +             unlock_page(*pagep);
> > +             put_page(*pagep);
> > +             ret = -EIO;
>
> You definitely need to add:
>
>                 *pagep = NULL;

Thanks, will do it.

>
> I'm not entirely convinced that you need the conditional on '*pagep'.
> If we returned 0, there had better be a page at pagep!

For SGP_WRITE, yes, it has a page at pagep if ret is 0, but
shmem_getpage() could return 0 with NULL page at pagep for SGP_READ.

In the other thread that Linus elaborated why this commit was reverted
and needed some rework, the discussion about not relying on
implementation detail for error handling taught me it may be not a
robust implementation to assume it is never NULL.

We might refactor shmem_getpage() code in the future to make sure when
it returns success there must be a valid pagep so that we just need to
care about the return value for error handling.

>
> I also think this would be clearer if written as:
>
>         if (PageHWPoison(*pagep)) {
>                 unlock_page(*pagep);
>                 put_page(*pagep);
>                 *pagep = NULL;
>                 return -EIO;
>         }
>
>         return 0;
>
> instead of re-using ret.  Sometimes that can make the code flow clearer,
> but here, I don't think it does.

Sure.

>
> > @@ -4168,9 +4201,12 @@ struct page *shmem_read_mapping_page_gfp(struct address_space *mapping,
> >       error = shmem_getpage_gfp(inode, index, &page, SGP_CACHE,
> >                                 gfp, NULL, NULL, NULL);
> >       if (error)
> > -             page = ERR_PTR(error);
> > -     else
> > -             unlock_page(page);
> > +             return ERR_PTR(error);
> > +
> > +     unlock_page(page);
> > +     if (PageHWPoison(page))
> > +             return ERR_PTR(-EIO);
>
> Do we need to put_page() the page in this error case?

Aha, yes. Sorry for missing this. I was fooled by shmem_pin_map() in
i915 driver which does put page, but I realized it just puts the valid
pages pinned *before* meeting error page by second look
.
>
Yang Shi Nov. 15, 2021, 7 p.m. UTC | #3 
On Sun, Nov 14, 2021 at 8:54 AM Yang Shi <shy828301@gmail.com> wrote:
>
> On Sun, Nov 14, 2021 at 7:28 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Sat, Nov 13, 2021 at 09:32:21PM -0800, Yang Shi wrote:
> > > @@ -2466,7 +2467,18 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
> > >                       return -EPERM;
> > >       }
> > >
> > > -     return shmem_getpage(inode, index, pagep, SGP_WRITE);
> > > +     ret = shmem_getpage(inode, index, pagep, SGP_WRITE);
> > > +
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     if (*pagep && PageHWPoison(*pagep)) {
> > > +             unlock_page(*pagep);
> > > +             put_page(*pagep);
> > > +             ret = -EIO;
> >
> > You definitely need to add:
> >
> >                 *pagep = NULL;
>
> Thanks, will do it.
>
> >
> > I'm not entirely convinced that you need the conditional on '*pagep'.
> > If we returned 0, there had better be a page at pagep!
>
> For SGP_WRITE, yes, it has a page at pagep if ret is 0, but
> shmem_getpage() could return 0 with NULL page at pagep for SGP_READ.
>
> In the other thread that Linus elaborated why this commit was reverted
> and needed some rework, the discussion about not relying on
> implementation detail for error handling taught me it may be not a
> robust implementation to assume it is never NULL.
>
> We might refactor shmem_getpage() code in the future to make sure when
> it returns success there must be a valid pagep so that we just need to
> care about the return value for error handling.
>
> >
> > I also think this would be clearer if written as:
> >
> >         if (PageHWPoison(*pagep)) {
> >                 unlock_page(*pagep);
> >                 put_page(*pagep);
> >                 *pagep = NULL;
> >                 return -EIO;
> >         }
> >
> >         return 0;

By rethinking the code, I do prefer the above. "if (*pagep &&
PageHWPoison(*pagep))"  does give extra protection from NULL pointer
dereference for the future, but on the opposite side it seems
confusing to *not* have error handling for NULL page. I bet it may
incur more confusion than the protection for future, anyway it can't
be NULL if ret == 0 and !SGP_READ.

shmem_read_mapping_page_gfp() doesn't check page pointer either. IIUC
only SGP_READ case has NULL page pointer checked in shmem code, so I'd
like to follow the convention in this patch. If we think it is not
good, we could refactor the code (for example, guarantee page have
valid page as long as ret value is not error) in a perarate patch.

> >
> > instead of re-using ret.  Sometimes that can make the code flow clearer,
> > but here, I don't think it does.
>
> Sure.
>
> >
> > > @@ -4168,9 +4201,12 @@ struct page *shmem_read_mapping_page_gfp(struct address_space *mapping,
> > >       error = shmem_getpage_gfp(inode, index, &page, SGP_CACHE,
> > >                                 gfp, NULL, NULL, NULL);
> > >       if (error)
> > > -             page = ERR_PTR(error);
> > > -     else
> > > -             unlock_page(page);
> > > +             return ERR_PTR(error);
> > > +
> > > +     unlock_page(page);
> > > +     if (PageHWPoison(page))
> > > +             return ERR_PTR(-EIO);
> >
> > Do we need to put_page() the page in this error case?
>
> Aha, yes. Sorry for missing this. I was fooled by shmem_pin_map() in
> i915 driver which does put page, but I realized it just puts the valid
> pages pinned *before* meeting error page by second look
> .
> >
diff mbox series

Patch

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 07c875fdeaf0..f64ebb6226cb 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -58,6 +58,7 @@ 
 #include <linux/ratelimit.h>
 #include <linux/page-isolation.h>
 #include <linux/pagewalk.h>
+#include <linux/shmem_fs.h>
 #include "internal.h"
 #include "ras/ras_event.h"
 
@@ -867,6 +868,7 @@  static int me_pagecache_clean(struct page_state *ps, struct page *p)
 {
 	int ret;
 	struct address_space *mapping;
+	bool extra_pins;
 
 	delete_from_lru_cache(p);
 
@@ -895,18 +897,24 @@  static int me_pagecache_clean(struct page_state *ps, struct page *p)
 		goto out;
 	}
 
+	/*
+	 * The shmem page is kept in page cache instead of truncating
+	 * so is expected to have an extra refcount after error-handling.
+	 */
+	extra_pins = shmem_mapping(mapping);
+
 	/*
 	 * Truncation is a bit tricky. Enable it per file system for now.
 	 *
 	 * Open: to take i_rwsem or not for this? Right now we don't.
 	 */
 	ret = truncate_error_page(p, page_to_pfn(p), mapping);
+	if (has_extra_refcount(ps, p, extra_pins))
+		ret = MF_FAILED;
+
 out:
 	unlock_page(p);
 
-	if (has_extra_refcount(ps, p, false))
-		ret = MF_FAILED;
-
 	return ret;
 }
 
diff --git a/mm/shmem.c b/mm/shmem.c
index dc038ce78700..a50be6486509 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2456,6 +2456,7 @@  shmem_write_begin(struct file *file, struct address_space *mapping,
 	struct inode *inode = mapping->host;
 	struct shmem_inode_info *info = SHMEM_I(inode);
 	pgoff_t index = pos >> PAGE_SHIFT;
+	int ret = 0;
 
 	/* i_rwsem is held by caller */
 	if (unlikely(info->seals & (F_SEAL_GROW |
@@ -2466,7 +2467,18 @@  shmem_write_begin(struct file *file, struct address_space *mapping,
 			return -EPERM;
 	}
 
-	return shmem_getpage(inode, index, pagep, SGP_WRITE);
+	ret = shmem_getpage(inode, index, pagep, SGP_WRITE);
+
+	if (ret)
+		return ret;
+
+	if (*pagep && PageHWPoison(*pagep)) {
+		unlock_page(*pagep);
+		put_page(*pagep);
+		ret = -EIO;
+	}
+
+	return ret;
 }
 
 static int
@@ -2553,6 +2565,12 @@  static ssize_t shmem_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 			if (sgp == SGP_CACHE)
 				set_page_dirty(page);
 			unlock_page(page);
+
+			if (PageHWPoison(page)) {
+				put_page(page);
+				error = -EIO;
+				break;
+			}
 		}
 
 		/*
@@ -3092,7 +3110,8 @@  static const char *shmem_get_link(struct dentry *dentry,
 		page = find_get_page(inode->i_mapping, 0);
 		if (!page)
 			return ERR_PTR(-ECHILD);
-		if (!PageUptodate(page)) {
+		if (PageHWPoison(page) ||
+		    !PageUptodate(page)) {
 			put_page(page);
 			return ERR_PTR(-ECHILD);
 		}
@@ -3100,6 +3119,13 @@  static const char *shmem_get_link(struct dentry *dentry,
 		error = shmem_getpage(inode, 0, &page, SGP_READ);
 		if (error)
 			return ERR_PTR(error);
+		if (!page)
+		       return ERR_PTR(-ECHILD);
+		if (PageHWPoison(page)) {
+			unlock_page(page);
+			put_page(page);
+			return ERR_PTR(-ECHILD);
+		}
 		unlock_page(page);
 	}
 	set_delayed_call(done, shmem_put_link, page);
@@ -3750,6 +3776,13 @@  static void shmem_destroy_inodecache(void)
 	kmem_cache_destroy(shmem_inode_cachep);
 }
 
+/* Keep the page in page cache instead of truncating it */
+static int shmem_error_remove_page(struct address_space *mapping,
+				   struct page *page)
+{
+	return 0;
+}
+
 const struct address_space_operations shmem_aops = {
 	.writepage	= shmem_writepage,
 	.set_page_dirty	= __set_page_dirty_no_writeback,
@@ -3760,7 +3793,7 @@  const struct address_space_operations shmem_aops = {
 #ifdef CONFIG_MIGRATION
 	.migratepage	= migrate_page,
 #endif
-	.error_remove_page = generic_error_remove_page,
+	.error_remove_page = shmem_error_remove_page,
 };
 EXPORT_SYMBOL(shmem_aops);
 
@@ -4168,9 +4201,12 @@  struct page *shmem_read_mapping_page_gfp(struct address_space *mapping,
 	error = shmem_getpage_gfp(inode, index, &page, SGP_CACHE,
 				  gfp, NULL, NULL, NULL);
 	if (error)
-		page = ERR_PTR(error);
-	else
-		unlock_page(page);
+		return ERR_PTR(error);
+
+	unlock_page(page);
+	if (PageHWPoison(page))
+		return ERR_PTR(-EIO);
+
 	return page;
 #else
 	/*
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index ac6f036298cd..0780c2a57ff1 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -232,6 +232,11 @@  static int mcontinue_atomic_pte(struct mm_struct *dst_mm,
 		goto out;
 	}
 
+	if (PageHWPoison(page)) {
+		ret = -EIO;
+		goto out_release;
+	}
+
 	ret = mfill_atomic_install_pte(dst_mm, dst_pmd, dst_vma, dst_addr,
 				       page, false, wp_copy);
 	if (ret)