linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: Hugh Dickins <hughd@google.com>
Cc: Naoya Horiguchi <nao.horiguchi@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Michal Hocko <mhocko@kernel.org>,
	Oscar Salvador <osalvador@suse.de>,
	Tony Luck <tony.luck@intel.com>,
	"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>,
	Naoya Horiguchi <naoya.horiguchi@nec.com>,
	Jue Wang <juew@google.com>, Greg Thelen <gthelen@google.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH v1] mm, hwpoison: enable error handling on shmem thp
Date: Thu, 11 Mar 2021 14:45:57 +0000	[thread overview]
Message-ID: <20210311144557.GV3479805@casper.infradead.org> (raw)
In-Reply-To: <alpine.LSU.2.11.2103102214170.7159@eggly.anvils>

On Wed, Mar 10, 2021 at 11:22:18PM -0800, Hugh Dickins wrote:
> But something we found on the way,
> we do have a patch for: add_to_kill() uses page_address_in_vma(), but
> that has not been used on file THP tails before - fix appended at the
> end below, so as not to waste your time on that bit.

> [PATCH] mm: fix page_address_in_vma() on file THP tails
> From: Jue Wang <juew@google.com>
> 
> Anon THP tails were already supported, but memory-failure now needs to use
> page_address_in_vma() on file THP tails, which its page->mapping check did
> not permit: fix it.
> 
> Signed-off-by: Jue Wang <juew@google.com>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
> 
>  mm/rmap.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- 5.12-rc2/mm/rmap.c	2021-02-28 16:58:57.950450151 -0800
> +++ linux/mm/rmap.c	2021-03-10 20:29:21.591475177 -0800
> @@ -717,11 +717,11 @@ unsigned long page_address_in_vma(struct
>  		if (!vma->anon_vma || !page__anon_vma ||
>  		    vma->anon_vma->root != page__anon_vma->root)
>  			return -EFAULT;
> -	} else if (page->mapping) {
> -		if (!vma->vm_file || vma->vm_file->f_mapping != page->mapping)
> -			return -EFAULT;
> -	} else
> +	} else if (!vma->vm_file) {
> +		return -EFAULT;
> +	} else if (vma->vm_file->f_mapping != compound_head(page)->mapping) {
>  		return -EFAULT;
> +	}

This is a common bug I'm seeing; people just assume they can dereference
page->mapping.  Below is how I would convert this patch into folios, but
it doesn't remove the problem that people just use page->mapping.

Do we want to set ourselves the goal of making this bug impossible?  That
is, eventually, do something like this ...

struct folio {
	union {
		struct page page;
		struct {
			unsigned long flags;
			struct list_head lru;
			struct address_space *mapping;
			pgoff_t index;
			unsigned long private;
		};
	};
};

... and remove the names 'mapping' & 'index' from struct page.
If so, now would be a good time to prepare for that, so we write
folio->mapping instead of folio->page.mapping.

diff --git a/mm/rmap.c b/mm/rmap.c
index 3142ea1dd071..fcad8c6a417a 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -707,9 +707,11 @@ static bool should_defer_flush(struct mm_struct *mm, enum ttu_flags flags)
  */
 unsigned long page_address_in_vma(struct page *page, struct vm_area_struct *vma)
 {
+	struct folio *folio = page_folio(page);
 	unsigned long address;
-	if (PageAnon(page)) {
-		struct anon_vma *page__anon_vma = page_anon_vma(page);
+
+	if (FolioAnon(folio)) {
+		struct anon_vma *page__anon_vma = folio_anon_vma(folio);
 		/*
 		 * Note: swapoff's unuse_vma() is more efficient with this
 		 * check, and needs it to match anon_vma when KSM is active.
@@ -717,9 +719,10 @@ unsigned long page_address_in_vma(struct page *page, struct vm_area_struct *vma)
 		if (!vma->anon_vma || !page__anon_vma ||
 		    vma->anon_vma->root != page__anon_vma->root)
 			return -EFAULT;
-	} else if (page->mapping) {
-		if (!vma->vm_file || vma->vm_file->f_mapping != page->mapping)
-			return -EFAULT;
+	} else if (!vma->vm_file) {
+		return -EFAULT;
+	} else if (vma->vm_file->f_mapping != folio->page.mapping) {
+		return -EFAULT;
 	} else
 		return -EFAULT;
 	address = __vma_address(page, vma);
diff --git a/mm/util.c b/mm/util.c
index 9ab72cfa4aa1..91ac4fc90a07 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -635,11 +635,11 @@ void kvfree_sensitive(const void *addr, size_t len)
 }
 EXPORT_SYMBOL(kvfree_sensitive);
 
-static inline void *__page_rmapping(struct page *page)
+static inline void *folio_rmapping(struct folio *folio)
 {
 	unsigned long mapping;
 
-	mapping = (unsigned long)page->mapping;
+	mapping = (unsigned long)folio->page.mapping;
 	mapping &= ~PAGE_MAPPING_FLAGS;
 
 	return (void *)mapping;
@@ -648,8 +648,7 @@ static inline void *__page_rmapping(struct page *page)
 /* Neutral page->mapping pointer to address_space or anon_vma or other */
 void *page_rmapping(struct page *page)
 {
-	page = compound_head(page);
-	return __page_rmapping(page);
+	return folio_rmapping(page_folio(page));
 }
 
 /*
@@ -675,15 +674,12 @@ bool page_mapped(struct page *page)
 }
 EXPORT_SYMBOL(page_mapped);
 
-struct anon_vma *page_anon_vma(struct page *page)
+struct anon_vma *folio_anon_vma(struct folio *folio)
 {
-	unsigned long mapping;
-
-	page = compound_head(page);
-	mapping = (unsigned long)page->mapping;
+	unsigned long mapping = (unsigned long)folio->page.mapping;
 	if ((mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
 		return NULL;
-	return __page_rmapping(page);
+	return folio_rmapping(folio);
 }
 
 struct address_space *folio_mapping(struct folio *folio)

  reply	other threads:[~2021-03-11 14:47 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-09  6:21 Naoya Horiguchi
2021-02-09 19:46 ` Andrew Morton
2021-02-09 23:51   ` Naoya Horiguchi
2021-02-11  8:06 ` Oscar Salvador
2021-03-11  7:22 ` Hugh Dickins
2021-03-11 14:45   ` Matthew Wilcox [this message]
     [not found]   ` <20210311151446.GA28650@hori.linux.bs1.fc.nec.co.jp>
2021-03-11 19:32     ` Jue Wang
2021-03-11 22:00       ` Hugh Dickins
2021-03-18 18:33         ` Matthew Wilcox

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210311144557.GV3479805@casper.infradead.org \
    --to=willy@infradead.org \
    --cc=akpm@linux-foundation.org \
    --cc=aneesh.kumar@linux.vnet.ibm.com \
    --cc=gthelen@google.com \
    --cc=hughd@google.com \
    --cc=juew@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@kernel.org \
    --cc=nao.horiguchi@gmail.com \
    --cc=naoya.horiguchi@nec.com \
    --cc=osalvador@suse.de \
    --cc=tony.luck@intel.com \
    --subject='Re: [PATCH v1] mm, hwpoison: enable error handling on shmem thp' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).