diff mbox series

[2/2] hugetlb: revert use of page_cache_next_miss()

Message ID 20230621212403.174710-2-mike.kravetz@oracle.com
State New
Headers
Series [1/2] Revert "page cache: fix page_cache_next/prev_miss off by one" |

Commit Message

Mike Kravetz June 21, 2023, 9:24 p.m. UTC 
  Ackerley Tng reported an issue with hugetlbfs fallocate as noted in the
Closes tag.  The issue showed up after the conversion of hugetlb page
cache lookup code to use page_cache_next_miss.  User visible effects are:
- hugetlbfs fallocate incorrectly returns -EEXIST if pages are presnet
  in the file.
- hugetlb pages will not be included in core dumps if they need to be
  brought in via GUP.
- userfaultfd UFFDIO_COPY will not notice pages already present in the
  cache.  It may try to allocate a new page and potentially return
  ENOMEM as opposed to EEXIST.

Revert the use page_cache_next_miss() in hugetlb code.

IMPORTANT NOTE FOR STABLE BACKPORTS:
This patch will apply cleanly to v6.3.  However, due to the change of
filemap_get_folio() return values, it will not function correctly.  This
patch must be modified for stable backports.

Fixes: d0ce0e47b323 ("mm/hugetlb: convert hugetlb fault paths to use alloc_hugetlb_folio()")
Reported-by: Ackerley Tng <ackerleytng@google.com>
Closes: https://lore.kernel.org/linux-mm/cover.1683069252.git.ackerleytng@google.com
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/hugetlbfs/inode.c |  8 +++-----
 mm/hugetlb.c         | 11 +++++------
 2 files changed, 8 insertions(+), 11 deletions(-)

Comments

Sidhartha Kumar June 21, 2023, 10:19 p.m. UTC | #1 
On 6/21/23 2:24 PM, Mike Kravetz wrote:
> Ackerley Tng reported an issue with hugetlbfs fallocate as noted in the
> Closes tag.  The issue showed up after the conversion of hugetlb page
> cache lookup code to use page_cache_next_miss.  User visible effects are:
> - hugetlbfs fallocate incorrectly returns -EEXIST if pages are presnet
>    in the file.
> - hugetlb pages will not be included in core dumps if they need to be
>    brought in via GUP.
> - userfaultfd UFFDIO_COPY will not notice pages already present in the
>    cache.  It may try to allocate a new page and potentially return
>    ENOMEM as opposed to EEXIST.
> 
> Revert the use page_cache_next_miss() in hugetlb code.
> 
> IMPORTANT NOTE FOR STABLE BACKPORTS:
> This patch will apply cleanly to v6.3.  However, due to the change of
> filemap_get_folio() return values, it will not function correctly.  This
> patch must be modified for stable backports.

This patch I sent previously can be used for the 6.3 backport:

https://lore.kernel.org/lkml/b5bd2b39-7e1e-148f-7462-9565773f6d41@oracle.com/T/#me37b56ca89368dc8dda2a33d39f681337788d13c

> 
> Fixes: d0ce0e47b323 ("mm/hugetlb: convert hugetlb fault paths to use alloc_hugetlb_folio()")
> Reported-by: Ackerley Tng <ackerleytng@google.com>
> Closes: https://lore.kernel.org/linux-mm/cover.1683069252.git.ackerleytng@google.com
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>   fs/hugetlbfs/inode.c |  8 +++-----
>   mm/hugetlb.c         | 11 +++++------
>   2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 90361a922cec..7b17ccfa039d 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -821,7 +821,6 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>   		 */
>   		struct folio *folio;
>   		unsigned long addr;
> -		bool present;
>   
>   		cond_resched();
>   
> @@ -842,10 +841,9 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>   		mutex_lock(&hugetlb_fault_mutex_table[hash]);
>   
>   		/* See if already present in mapping to avoid alloc/free */
> -		rcu_read_lock();
> -		present = page_cache_next_miss(mapping, index, 1) != index;
> -		rcu_read_unlock();
> -		if (present) {
> +		folio = filemap_get_folio(mapping, index);
> +		if (!IS_ERR(folio)) {
> +			folio_put(folio);
>   			mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>   			continue;
>   		}
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index d76574425da3..cb9077b96b43 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5728,13 +5728,12 @@ static bool hugetlbfs_pagecache_present(struct hstate *h,
>   {
>   	struct address_space *mapping = vma->vm_file->f_mapping;
>   	pgoff_t idx = vma_hugecache_offset(h, vma, address);
> -	bool present;
> -
> -	rcu_read_lock();
> -	present = page_cache_next_miss(mapping, idx, 1) != idx;
> -	rcu_read_unlock();
> +	struct folio *folio;
>   
> -	return present;
> +	folio = filemap_get_folio(mapping, idx);
> +	if (!IS_ERR(folio))
> +		folio_put(folio);
> +	return folio != NULL;
>   }
>   
>   int hugetlb_add_to_page_cache(struct folio *folio, struct address_space *mapping,

Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
Andrew Morton June 21, 2023, 10:39 p.m. UTC | #2 
On Wed, 21 Jun 2023 15:19:58 -0700 Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:

> > IMPORTANT NOTE FOR STABLE BACKPORTS:
> > This patch will apply cleanly to v6.3.  However, due to the change of
> > filemap_get_folio() return values, it will not function correctly.  This
> > patch must be modified for stable backports.
> 
> This patch I sent previously can be used for the 6.3 backport:
> 
> https://lore.kernel.org/lkml/b5bd2b39-7e1e-148f-7462-9565773f6d41@oracle.com/T/#me37b56ca89368dc8dda2a33d39f681337788d13c

Are we suggesting that this be backported?  If so, I'll add the cc:stable.

Because -stable maintainers have been asked not to backport MM patches to
which we didn't add the cc:stable.
Mike Kravetz June 21, 2023, 10:46 p.m. UTC | #3 
On 06/21/23 15:39, Andrew Morton wrote:
> On Wed, 21 Jun 2023 15:19:58 -0700 Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:
> 
> > > IMPORTANT NOTE FOR STABLE BACKPORTS:
> > > This patch will apply cleanly to v6.3.  However, due to the change of
> > > filemap_get_folio() return values, it will not function correctly.  This
> > > patch must be modified for stable backports.
> > 
> > This patch I sent previously can be used for the 6.3 backport:
> > 
> > https://lore.kernel.org/lkml/b5bd2b39-7e1e-148f-7462-9565773f6d41@oracle.com/T/#me37b56ca89368dc8dda2a33d39f681337788d13c
> 
> Are we suggesting that this be backported?  If so, I'll add the cc:stable.
> 
> Because -stable maintainers have been asked not to backport MM patches to
> which we didn't add the cc:stable.

Yes, we need to get a fix into 6.3 as well.

The 'issue' with a backport is noted in the IMPORTANT NOTE above.

My concern is that adding cc:stable will have it automatically picked up
and this would make things worse than they are in 6.3.

My 'plan' was to not add the cc:stable, but manually create a patch for
6.3 once this goes upstream.  Honestly, I am not sure what is the best
way to deal with this.  I could also try to catch the email about the
automatic backport and say 'no, use this new patch instead'.
Andrew Morton June 21, 2023, 10:52 p.m. UTC | #4 
On Wed, 21 Jun 2023 15:46:57 -0700 Mike Kravetz <mike.kravetz@oracle.com> wrote:

> On 06/21/23 15:39, Andrew Morton wrote:
> > On Wed, 21 Jun 2023 15:19:58 -0700 Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:
> > 
> > > > IMPORTANT NOTE FOR STABLE BACKPORTS:
> > > > This patch will apply cleanly to v6.3.  However, due to the change of
> > > > filemap_get_folio() return values, it will not function correctly.  This
> > > > patch must be modified for stable backports.
> > > 
> > > This patch I sent previously can be used for the 6.3 backport:
> > > 
> > > https://lore.kernel.org/lkml/b5bd2b39-7e1e-148f-7462-9565773f6d41@oracle.com/T/#me37b56ca89368dc8dda2a33d39f681337788d13c
> > 
> > Are we suggesting that this be backported?  If so, I'll add the cc:stable.
> > 
> > Because -stable maintainers have been asked not to backport MM patches to
> > which we didn't add the cc:stable.
> 
> Yes, we need to get a fix into 6.3 as well.
> 
> The 'issue' with a backport is noted in the IMPORTANT NOTE above.
> 
> My concern is that adding cc:stable will have it automatically picked up
> and this would make things worse than they are in 6.3.
> 
> My 'plan' was to not add the cc:stable, but manually create a patch for
> 6.3 once this goes upstream.  Honestly, I am not sure what is the best
> way to deal with this.  I could also try to catch the email about the
> automatic backport and say 'no, use this new patch instead'.

OK, how about I leave it without cc:stable, so you can send the 6.3
version at a time of your choosing?
Mike Kravetz June 21, 2023, 11:02 p.m. UTC | #5 
On 06/21/23 15:52, Andrew Morton wrote:
> On Wed, 21 Jun 2023 15:46:57 -0700 Mike Kravetz <mike.kravetz@oracle.com> wrote:
> 
> > On 06/21/23 15:39, Andrew Morton wrote:
> > > On Wed, 21 Jun 2023 15:19:58 -0700 Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:
> > > 
> > > > > IMPORTANT NOTE FOR STABLE BACKPORTS:
> > > > > This patch will apply cleanly to v6.3.  However, due to the change of
> > > > > filemap_get_folio() return values, it will not function correctly.  This
> > > > > patch must be modified for stable backports.
> > > > 
> > > > This patch I sent previously can be used for the 6.3 backport:
> > > > 
> > > > https://lore.kernel.org/lkml/b5bd2b39-7e1e-148f-7462-9565773f6d41@oracle.com/T/#me37b56ca89368dc8dda2a33d39f681337788d13c
> > > 
> > > Are we suggesting that this be backported?  If so, I'll add the cc:stable.
> > > 
> > > Because -stable maintainers have been asked not to backport MM patches to
> > > which we didn't add the cc:stable.
> > 
> > Yes, we need to get a fix into 6.3 as well.
> > 
> > The 'issue' with a backport is noted in the IMPORTANT NOTE above.
> > 
> > My concern is that adding cc:stable will have it automatically picked up
> > and this would make things worse than they are in 6.3.
> > 
> > My 'plan' was to not add the cc:stable, but manually create a patch for
> > 6.3 once this goes upstream.  Honestly, I am not sure what is the best
> > way to deal with this.  I could also try to catch the email about the
> > automatic backport and say 'no, use this new patch instead'.
> 
> OK, how about I leave it without cc:stable, so you can send the 6.3
> version at a time of your choosing?

Perfect
diff mbox series

Patch

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 90361a922cec..7b17ccfa039d 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -821,7 +821,6 @@  static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
 		 */
 		struct folio *folio;
 		unsigned long addr;
-		bool present;
 
 		cond_resched();
 
@@ -842,10 +841,9 @@  static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
 		mutex_lock(&hugetlb_fault_mutex_table[hash]);
 
 		/* See if already present in mapping to avoid alloc/free */
-		rcu_read_lock();
-		present = page_cache_next_miss(mapping, index, 1) != index;
-		rcu_read_unlock();
-		if (present) {
+		folio = filemap_get_folio(mapping, index);
+		if (!IS_ERR(folio)) {
+			folio_put(folio);
 			mutex_unlock(&hugetlb_fault_mutex_table[hash]);
 			continue;
 		}
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index d76574425da3..cb9077b96b43 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5728,13 +5728,12 @@  static bool hugetlbfs_pagecache_present(struct hstate *h,
 {
 	struct address_space *mapping = vma->vm_file->f_mapping;
 	pgoff_t idx = vma_hugecache_offset(h, vma, address);
-	bool present;
-
-	rcu_read_lock();
-	present = page_cache_next_miss(mapping, idx, 1) != idx;
-	rcu_read_unlock();
+	struct folio *folio;
 
-	return present;
+	folio = filemap_get_folio(mapping, idx);
+	if (!IS_ERR(folio))
+		folio_put(folio);
+	return folio != NULL;
 }
 
 int hugetlb_add_to_page_cache(struct folio *folio, struct address_space *mapping,