| Message ID | cb971ac8dd315df97058ea69442ecc007b9a364a.1683381545.git.lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/gup: add missing gup_must_unshare() check to gup_huge_pgd() | expand |
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.
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>
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")
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;
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(+)