Re: [PATCH v1] mm/gup: fix handling of errors from arch_make_folio_accessible() in follow_page_pte()

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Mon,  8 Sep 2025 11:45:17 +0200
David Hildenbrand <david@redhat.com> wrote:

> In case we call arch_make_folio_accessible() and it fails, we would
> incorrectly return a value that is "!= 0" to the caller, indicating that
> we pinned all requested pages and that the caller can keep going.
> 
> follow_page_pte() is not supposed to return error values, but instead
> "0" on failure and "1" on success -- we'll clean that up separately.
> 
> In case we return "!= 0", the caller will just keep going pinning
> more pages. If we happen to pin a page afterwards, we're in trouble,
> because we essentially skipped some pages in the requested range.
> 
> Staring at the arch_make_folio_accessible() implementation on s390x, I
> assume it should actually never really fail unless something unexpected
> happens (BUG?). So let's not CC stable and just fix common code to do
> the right thing.
> 
> Clean up the code a bit now that there is no reason to store the
> return value of arch_make_folio_accessible().
> 
> Fixes: f28d43636d6f ("mm/gup/writeback: add callbacks for inaccessible pages")

Ooops!

thanks for finding and fixing this

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/gup.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index d27b6b9818a18..c969259d095c9 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2920,12 +2920,9 @@ static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
>  		 * see Documentation/core-api/pin_user_pages.rst for
>  		 * details.
>  		 */
> -		if (flags & FOLL_PIN) {
> -			ret = arch_make_folio_accessible(folio);
> -			if (ret) {
> -				gup_put_folio(folio, 1, flags);
> -				goto pte_unmap;
> -			}
> +		if ((flags & FOLL_PIN) && arch_make_folio_accessible(folio)) {
> +			gup_put_folio(folio, 1, flags);
> +			goto pte_unmap;
>  		}
>  		folio_set_referenced(folio);
>  		pages[*nr] = page;