Re: [PATCH] mm/hugetlb: revert use of page_cache_next_miss()

[Mike Kravetz]

On 05/05/23 14:58, Mike Kravetz wrote:
> On 05/05/23 11:53, Sidhartha Kumar wrote:
> > As reported by Ackerley[1], the use of page_cache_next_miss() in
> > hugetlbfs_fallocate() introduces a bug where a second fallocate() call to
> > same offset fails with -EEXIST. Revert this change and go back to the
> > previous method of using get from the page cache and then dropping the
> > reference on success.
> > 
> > hugetlbfs_pagecache_present() was also refactored to use
> > page_cache_next_miss(), revert the usage there as well.
> > 
> > User visible impacts include hugetlb fallocate incorrectly returning
> > EEXIST if pages are already present in the file. In addition, hugetlb
> > pages will not be included in core dumps if they need to be brought in via
> > GUP. userfaultfd UFFDIO_COPY also uses this code and 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.
> > 
> > Fixes: d0ce0e47b323 ("mm/hugetlb: convert hugetlb fault paths to use alloc_hugetlb_folio()")
> 
> Small nit and a question for people more familiar with stable backports.
> 
> d0ce0e47b323 added the usage of page_cache_next_miss to hugetlb fallocate.
> 91a2fb956ad99 added the usage to hugetlbfs_pagecache_present.  Both are
> in v6.3 and d0ce0e47b323 (referenced here) comes later.  So, I 'think' it
> is OK to fix both instances with a single patch and reference the commit
> where both are present.  Or, should there be two patches which is more
> technically correct?
> 
> > Cc: <stable@xxxxxxxxxxxxxxx> #v6.3+
> > Reported-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
> > Signed-off-by: Sidhartha Kumar <sidhartha.kumar@xxxxxxxxxx>
> > 
> > [1] https://lore.kernel.org/linux-mm/cover.1683069252.git.ackerleytng@xxxxxxxxxx/
> > ---
> > This patch is meant to fix stable v6.3.1 as safe as possible by doing a
> > simple revert.
> > 
> > Patch page cache: fix page_cache_next/prev_miss off by one by Mike is a
> > potential fix that will allow the use of page_cache_next_miss() and is
> > awaiting review.
> > 
> > Patch Fix fallocate error in hugetlbfs when fallocating again by Ackerley
> > is another fix but introduces a new function and is also awaiting review.
> > 
> >  fs/hugetlbfs/inode.c |  8 +++-----
> >  mm/hugetlb.c         | 11 +++++------
> >  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> IMO, this is safest and simplest way of fixing v6.3.  My proposed changes to
> page_cache_next/prev_miss have the potential to impact readahead, so really
> need review/testing by someone more familiar with that.  If a fix is
> urgently needed, I would suggest using this for backport and then either
> use my patch or expand Ackerley's proposal to move forward.
> 
> As a backport to stable,
> Reviewed-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
> -- 
> Mike Kravetz

Any objection to using this patch to fix v6.3 while we decide what is the best
way to move forward?

-- 
Mike Kravetz