From patchwork Tue Oct 12 01:53:01 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Suren Baghdasaryan X-Patchwork-Id: 12551283 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6CDC6C433F5 for ; Tue, 12 Oct 2021 01:53:10 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id F296360ED4 for ; Tue, 12 Oct 2021 01:53:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org F296360ED4 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 8E7F06B006C; Mon, 11 Oct 2021 21:53:09 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8707F900002; Mon, 11 Oct 2021 21:53:09 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 711736B0073; Mon, 11 Oct 2021 21:53:09 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0069.hostedemail.com [216.40.44.69]) by kanga.kvack.org (Postfix) with ESMTP id 5BADD6B006C for ; Mon, 11 Oct 2021 21:53:09 -0400 (EDT) Received: from smtpin25.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 1E21218234920 for ; Tue, 12 Oct 2021 01:53:09 +0000 (UTC) X-FDA: 78686112498.25.E6E3A0A Received: from mail-yb1-f202.google.com (mail-yb1-f202.google.com [209.85.219.202]) by imf10.hostedemail.com (Postfix) with ESMTP id B779B6003AF8 for ; Tue, 12 Oct 2021 01:53:08 +0000 (UTC) Received: by mail-yb1-f202.google.com with SMTP id z130-20020a256588000000b005b6b4594129so25623740ybb.15 for ; Mon, 11 Oct 2021 18:53:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:message-id:mime-version:subject:from:to:cc; bh=WQParBdE8Wx5REtDL/4l+GJT+pnnoGYyakCAqube4nM=; b=finmDCVXGzhWCj2R0jkv8MvFwzJuQGr6Z8lYEN74KtmrinKULG/zCW5fyW6piM73WG v0pgTluwaGms01BEKpR+TlaFqQVfWdd/LJnSpNzSC78vxuwbmtbfzF2EXTUPRt5LNLpE 2vU2CMkroqWOa43jwIN22s4pymlrd1mhXr/svCnl9+HP6ePQp6tECfAWHDiVpUxB1qH/ c7232pZb18IaCLvHxy0t2wvYrP8GYAcQZsxH3uVdwXJx2Ss6lJQXhK9Gs/Bs8708VSMx tmcM2BsBOW0cF5fytse0LZM1YqnZTBENw4ThqBpNwUKRJQ5uAOfzafUQiEx6I6xFqBgT LxbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:message-id:mime-version:subject:from:to:cc; bh=WQParBdE8Wx5REtDL/4l+GJT+pnnoGYyakCAqube4nM=; b=4ztsK9JhS9gmKbX9JhzVnxzJ6Lf4ireis0ljPieod4BemBYHNM+dCjyYRFzHysdF2v CS05z9C2KmVynSijQC0zCqTAWkwy5xyByzI2X/vYRvH+RhGogEsGoSfVVp2LvO54Iv8d fgl4+wfhODE5polJWzQ9z07P871KGVYiyIOWVek92k1Gvkx0txdT0suKD9JhC/i6QY4M 9Jad52MiJxbUzm+PAxLrQ/ZIb6br0WXgmHFWc2sdWvwnzVxiV0xxoc/V7WvhJi7eNZSg Dqzr9Z6AmrdxQHsrYi2ZcbGdi//f1GEy4wJsS6difr30fgQ9XiRiohpmczGZSMmCPJyw SQVw== X-Gm-Message-State: AOAM530Ds/qQ6Sjvcq0bgO4/zJF5I/qi0aj5d5XFkKWRB5PQV2xk7AIu ySDRt92bKLOJ9Mxw4N6cY1ofEnGFEuU= X-Google-Smtp-Source: ABdhPJzbfUeDwn/mJh4xoIboL1lPLy+rNzq0F7LBkDLYVdTl9L54iXkAUT8hPwYoB4/pTWln//UZQ35OY7Y= X-Received: from surenb-desktop.mtv.corp.google.com ([2620:15c:211:200:8f50:78d2:a82f:433]) (user=surenb job=sendgmr) by 2002:a25:5244:: with SMTP id g65mr26554207ybb.464.1634003588066; Mon, 11 Oct 2021 18:53:08 -0700 (PDT) Date: Mon, 11 Oct 2021 18:53:01 -0700 Message-Id: <20211012015301.694046-1-surenb@google.com> Mime-Version: 1.0 X-Mailer: git-send-email 2.33.0.882.g93a45727a2-goog Subject: [PATCH 1/1] gup: document and work around "COW can break either way" issue From: Suren Baghdasaryan To: stable@vger.kernel.org Cc: gregkh@linuxfoundation.org, jannh@google.com, torvalds@linux-foundation.org, vbabka@suse.cz, peterx@redhat.com, aarcange@redhat.com, david@redhat.com, jgg@ziepe.ca, ktkhai@virtuozzo.com, shli@fb.com, namit@vmware.com, hch@lst.de, oleg@redhat.com, kirill@shutemov.name, jack@suse.cz, willy@infradead.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@android.com, surenb@google.com X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: B779B6003AF8 X-Stat-Signature: noefr55d3nx9hgoniitp8wukg8st8jb9 Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=finmDCVX; spf=pass (imf10.hostedemail.com: domain of 3hOpkYQYKCP0xzwjsglttlqj.htrqnsz2-rrp0fhp.twl@flex--surenb.bounces.google.com designates 209.85.219.202 as permitted sender) smtp.mailfrom=3hOpkYQYKCP0xzwjsglttlqj.htrqnsz2-rrp0fhp.twl@flex--surenb.bounces.google.com; dmarc=pass (policy=reject) header.from=google.com X-HE-Tag: 1634003588-591582 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: From: Linus Torvalds From: Linus Torvalds commit 17839856fd588f4ab6b789f482ed3ffd7c403e1f upstream. Doing a "get_user_pages()" on a copy-on-write page for reading can be ambiguous: the page can be COW'ed at any time afterwards, and the direction of a COW event isn't defined. Yes, whoever writes to it will generally do the COW, but if the thread that did the get_user_pages() unmapped the page before the write (and that could happen due to memory pressure in addition to any outright action), the writer could also just take over the old page instead. End result: the get_user_pages() call might result in a page pointer that is no longer associated with the original VM, and is associated with - and controlled by - another VM having taken it over instead. So when doing a get_user_pages() on a COW mapping, the only really safe thing to do would be to break the COW when getting the page, even when only getting it for reading. At the same time, some users simply don't even care. For example, the perf code wants to look up the page not because it cares about the page, but because the code simply wants to look up the physical address of the access for informational purposes, and doesn't really care about races when a page might be unmapped and remapped elsewhere. This adds logic to force a COW event by setting FOLL_WRITE on any copy-on-write mapping when FOLL_GET (or FOLL_PIN) is used to get a page pointer as a result. The current semantics end up being: - __get_user_pages_fast(): no change. If you don't ask for a write, you won't break COW. You'd better know what you're doing. - get_user_pages_fast(): the fast-case "look it up in the page tables without anything getting mmap_sem" now refuses to follow a read-only page, since it might need COW breaking. Which happens in the slow path - the fast path doesn't know if the memory might be COW or not. - get_user_pages() (including the slow-path fallback for gup_fast()): for a COW mapping, turn on FOLL_WRITE for FOLL_GET/FOLL_PIN, with very similar semantics to FOLL_FORCE. If it turns out that we want finer granularity (ie "only break COW when it might actually matter" - things like the zero page are special and don't need to be broken) we might need to push these semantics deeper into the lookup fault path. So if people care enough, it's possible that we might end up adding a new internal FOLL_BREAK_COW flag to go with the internal FOLL_COW flag we already have for tracking "I had a COW". Alternatively, if it turns out that different callers might want to explicitly control the forced COW break behavior, we might even want to make such a flag visible to the users of get_user_pages() instead of using the above default semantics. But for now, this is mostly commentary on the issue (this commit message being a lot bigger than the patch, and that patch in turn is almost all comments), with that minimal "enable COW breaking early" logic using the existing FOLL_WRITE behavior. [ It might be worth noting that we've always had this ambiguity, and it could arguably be seen as a user-space issue. You only get private COW mappings that could break either way in situations where user space is doing cooperative things (ie fork() before an execve() etc), but it _is_ surprising and very subtle, and fork() is supposed to give you independent address spaces. So let's treat this as a kernel issue and make the semantics of get_user_pages() easier to understand. Note that obviously a true shared mapping will still get a page that can change under us, so this does _not_ mean that get_user_pages() somehow returns any "stable" page ] [surenb: backport notes Since gup_pgd_range does not exist, made appropriate changes on the the gup_huge_pgd, gup_huge_pd and gup_pud_range calls instead. Replaced (gup_flags | FOLL_WRITE) with write=1 in gup_huge_pgd, gup_huge_pd and gup_pud_range. Removed FOLL_PIN usage in should_force_cow_break since it's missing in the earlier kernels.] Reported-by: Jann Horn Tested-by: Christoph Hellwig Acked-by: Oleg Nesterov Acked-by: Kirill Shutemov Acked-by: Jan Kara Cc: Andrea Arcangeli Cc: Matthew Wilcox Signed-off-by: Linus Torvalds [surenb: backport to 4.9 kernel] Cc: stable@vger.kernel.org # 4.9.x Signed-off-by: Suren Baghdasaryan --- mm/gup.c | 48 ++++++++++++++++++++++++++++++++++++++++-------- mm/huge_memory.c | 7 +++---- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/mm/gup.c b/mm/gup.c index 6bb7a8eb7f82..301dd96ef176 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -61,13 +61,22 @@ static int follow_pfn_pte(struct vm_area_struct *vma, unsigned long address, } /* - * FOLL_FORCE can write to even unwritable pte's, but only - * after we've gone through a COW cycle and they are dirty. + * FOLL_FORCE or a forced COW break can write even to unwritable pte's, + * but only after we've gone through a COW cycle and they are dirty. */ static inline bool can_follow_write_pte(pte_t pte, unsigned int flags) { - return pte_write(pte) || - ((flags & FOLL_FORCE) && (flags & FOLL_COW) && pte_dirty(pte)); + return pte_write(pte) || ((flags & FOLL_COW) && pte_dirty(pte)); +} + +/* + * A (separate) COW fault might break the page the other way and + * get_user_pages() would return the page from what is now the wrong + * VM. So we need to force a COW break at GUP time even for reads. + */ +static inline bool should_force_cow_break(struct vm_area_struct *vma, unsigned int flags) +{ + return is_cow_mapping(vma->vm_flags) && (flags & FOLL_GET); } static struct page *follow_page_pte(struct vm_area_struct *vma, @@ -577,12 +586,18 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm, if (!vma || check_vma_flags(vma, gup_flags)) return i ? : -EFAULT; if (is_vm_hugetlb_page(vma)) { + if (should_force_cow_break(vma, foll_flags)) + foll_flags |= FOLL_WRITE; i = follow_hugetlb_page(mm, vma, pages, vmas, &start, &nr_pages, i, - gup_flags); + foll_flags); continue; } } + + if (should_force_cow_break(vma, foll_flags)) + foll_flags |= FOLL_WRITE; + retry: /* * If we have a pending SIGKILL, don't keep faulting pages and @@ -1503,6 +1518,10 @@ static int gup_pud_range(pgd_t pgd, unsigned long addr, unsigned long end, /* * Like get_user_pages_fast() except it's IRQ-safe in that it won't fall back to * the regular GUP. It will only return non-negative values. + * + * Careful, careful! COW breaking can go either way, so a non-write + * access can get ambiguous page results. If you call this function without + * 'write' set, you'd better be sure that you're ok with that ambiguity. */ int __get_user_pages_fast(unsigned long start, int nr_pages, int write, struct page **pages) @@ -1532,6 +1551,12 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write, * * We do not adopt an rcu_read_lock(.) here as we also want to * block IPIs that come from THPs splitting. + * + * NOTE! We allow read-only gup_fast() here, but you'd better be + * careful about possible COW pages. You'll get _a_ COW page, but + * not necessarily the one you intended to get depending on what + * COW event happens after this. COW may break the page copy in a + * random direction. */ local_irq_save(flags); @@ -1542,15 +1567,22 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write, next = pgd_addr_end(addr, end); if (pgd_none(pgd)) break; + /* + * The FAST_GUP case requires FOLL_WRITE even for pure reads, + * because get_user_pages() may need to cause an early COW in + * order to avoid confusing the normal COW routines. So only + * targets that are already writable are safe to do by just + * looking at the page tables. + */ if (unlikely(pgd_huge(pgd))) { - if (!gup_huge_pgd(pgd, pgdp, addr, next, write, + if (!gup_huge_pgd(pgd, pgdp, addr, next, 1, pages, &nr)) break; } else if (unlikely(is_hugepd(__hugepd(pgd_val(pgd))))) { if (!gup_huge_pd(__hugepd(pgd_val(pgd)), addr, - PGDIR_SHIFT, next, write, pages, &nr)) + PGDIR_SHIFT, next, 1, pages, &nr)) break; - } else if (!gup_pud_range(pgd, addr, next, write, pages, &nr)) + } else if (!gup_pud_range(pgd, addr, next, 1, pages, &nr)) break; } while (pgdp++, addr = next, addr != end); local_irq_restore(flags); diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 14cd0ef33b62..c5424483c066 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -1135,13 +1135,12 @@ int do_huge_pmd_wp_page(struct fault_env *fe, pmd_t orig_pmd) } /* - * FOLL_FORCE can write to even unwritable pmd's, but only - * after we've gone through a COW cycle and they are dirty. + * FOLL_FORCE or a forced COW break can write even to unwritable pmd's, + * but only after we've gone through a COW cycle and they are dirty. */ static inline bool can_follow_write_pmd(pmd_t pmd, unsigned int flags) { - return pmd_write(pmd) || - ((flags & FOLL_FORCE) && (flags & FOLL_COW) && pmd_dirty(pmd)); + return pmd_write(pmd) || ((flags & FOLL_COW) && pmd_dirty(pmd)); } struct page *follow_trans_huge_pmd(struct vm_area_struct *vma,