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

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

[David Hildenbrand]

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")
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;
-- 
2.50.1

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

[Claudio Imbrenda]

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;

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

[David Hildenbrand]

On 09.09.25 18:07, Claudio Imbrenda wrote:
> 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

Thanks! Is my assumption correct that this is not stable material?

-- 
Cheers

David / dhildenb

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

[Claudio Imbrenda]

On Tue, 9 Sep 2025 18:15:17 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 09.09.25 18:07, Claudio Imbrenda wrote:
> > 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  
> 
> Thanks! Is my assumption correct that this is not stable material?
> 

your assessment looks correct, an error return value can only be caused
by a bug