[PATCH] mm/gup: add missing gup_must_unshare() check to gup_huge_pgd()

[PATCH] mm/gup: add missing gup_must_unshare() check to gup_huge_pgd()

[Lorenzo Stoakes]

All other instances of gup_huge_pXd() perform the unshare check, so update
the PGD-specific function to do so as well.

While checking pgd_write() might seem unusual, this function already
performs such a check via pgd_access_permitted() so this is in line with
the existing implementation.

Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
---
 mm/gup.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/gup.c b/mm/gup.c
index ef43ffb3d1fe..78a5198e3212 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2898,6 +2898,11 @@ static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr,
 		return 0;
 	}
 
+	if (!pgd_write(orig) && gup_must_unshare(NULL, flags, &folio->page)) {
+		gup_put_folio(folio, refs, flags);
+		return 0;
+	}
+
 	*nr += refs;
 	folio_set_referenced(folio);
 	return 1;
-- 
2.40.1

Re: [PATCH] mm/gup: add missing gup_must_unshare() check to gup_huge_pgd()

[Andrew Morton]

On Sat,  6 May 2023 15:05:25 +0100 Lorenzo Stoakes <lstoakes@gmail.com> wrote:

> All other instances of gup_huge_pXd() perform the unshare check, so update
> the PGD-specific function to do so as well.
> 
> While checking pgd_write() might seem unusual, this function already
> performs such a check via pgd_access_permitted() so this is in line with
> the existing implementation.

Rationale seems strange.  "Other sites do it so this should also".  Why
is this a desirable change?  Maybe the "other instances" shouldn't be
performing this check either?

IOW, what are the runtime effects of this change?

Thanks.

Re: [PATCH] mm/gup: add missing gup_must_unshare() check to gup_huge_pgd()

[David Hildenbrand]

On 08.05.23 02:30, Andrew Morton wrote:
> On Sat,  6 May 2023 15:05:25 +0100 Lorenzo Stoakes <lstoakes@gmail.com> wrote:
> 
>> All other instances of gup_huge_pXd() perform the unshare check, so update
>> the PGD-specific function to do so as well.
>>
>> While checking pgd_write() might seem unusual, this function already
>> performs such a check via pgd_access_permitted() so this is in line with
>> the existing implementation.
> 
> Rationale seems strange.  "Other sites do it so this should also".  Why
> is this a desirable change?  Maybe the "other instances" shouldn't be
> performing this check either?

This change makes unshare handling across all GUP-fast variants consistent,
which is desirable as GUP-fast is complicated enough already even when
consistent :)

This function was the only one I seemed to have missed (or left out and forgot
why -- maybe because it's really dead code for now). The COW selftest would
identify the problem, so far there was no report. Either the selftest wasn't
run on corresponding architectures with that hugetlb size, or that code is still
dead code and unused by architectures.

I suspect the latter.


It might be worth to add a reference to the original commit(s) that added unsharing,
because they also explain why we care about these checks:

commit a7f226604170acd6b142b76472c1a49c12ebb83d
Author: David Hildenbrand <david@redhat.com>
Date:   Mon May 9 18:20:45 2022 -0700

     mm/gup: trigger FAULT_FLAG_UNSHARE when R/O-pinning a possibly shared anonymous page


commit 84209e87c6963f928194a890399e24e8ad299db1
Author: David Hildenbrand <david@redhat.com>
Date:   Wed Nov 16 11:26:48 2022 +0100

     mm/gup: reliable R/O long-term pinning in COW mappings


Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm/gup: add missing gup_must_unshare() check to gup_huge_pgd()

[Andrew Morton]

On Mon, 8 May 2023 02:45:12 +0200 David Hildenbrand <david@redhat.com> wrote:

> On 08.05.23 02:30, Andrew Morton wrote:
> > On Sat,  6 May 2023 15:05:25 +0100 Lorenzo Stoakes <lstoakes@gmail.com> wrote:
> > 
> >> All other instances of gup_huge_pXd() perform the unshare check, so update
> >> the PGD-specific function to do so as well.
> >>
> >> While checking pgd_write() might seem unusual, this function already
> >> performs such a check via pgd_access_permitted() so this is in line with
> >> the existing implementation.
> > 
> > Rationale seems strange.  "Other sites do it so this should also".  Why
> > is this a desirable change?  Maybe the "other instances" shouldn't be
> > performing this check either?
> 
> This change makes unshare handling across all GUP-fast variants consistent,
> which is desirable as GUP-fast is complicated enough already even when
> consistent :)

Thanks, I added the below to the changelog:

David said:

: This change makes unshare handling across all GUP-fast variants
: consistent, which is desirable as GUP-fast is complicated enough
: already even when consistent.
: 
: This function was the only one I seemed to have missed (or left out and
: forgot why -- maybe because it's really dead code for now).  The COW
: selftest would identify the problem, so far there was no report. 
: Either the selftest wasn't run on corresponding architectures with that
: hugetlb size, or that code is still dead code and unused by
: architectures.
: 
: the original commit(s) that added unsharing explain why we care about
: these checks:
: 
: a7f226604170acd6 ("mm/gup: trigger FAULT_FLAG_UNSHARE when R/O-pinning a possibly shared anonymous page")
: 84209e87c6963f92 ("mm/gup: reliable R/O long-term pinning in COW mappings")