LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Greg KH <firstname.lastname@example.org>
To: Suren Baghdasaryan <email@example.com>
Cc: firstname.lastname@example.org, email@example.com,
firstname.lastname@example.org, email@example.com, firstname.lastname@example.org,
email@example.com, firstname.lastname@example.org, email@example.com,
firstname.lastname@example.org, email@example.com, firstname.lastname@example.org,
email@example.com, Christoph Hellwig <firstname.lastname@example.org>,
Oleg Nesterov <email@example.com>,
Kirill Shutemov <firstname.lastname@example.org>, Jan Kara <email@example.com>,
Matthew Wilcox <firstname.lastname@example.org>
Subject: Re: [PATCH 1/1] gup: document and work around "COW can break either way" issue
Date: Fri, 23 Apr 2021 17:05:21 +0200 [thread overview]
Message-ID: <YILiMSSHUvPZxI4l@kroah.com> (raw)
On Wed, Apr 21, 2021 at 03:56:13PM -0700, Suren Baghdasaryan wrote:
> From: Linus Torvalds <email@example.com>
> 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
> 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
> 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]
> Replaced (gup_flags | FOLL_WRITE) with write=1 in gup_pgd_range.
> Removed FOLL_PIN usage in should_force_cow_break since it's missing in
> the earlier kernels.
> Reported-by: Jann Horn <firstname.lastname@example.org>
> Tested-by: Christoph Hellwig <email@example.com>
> Acked-by: Oleg Nesterov <firstname.lastname@example.org>
> Acked-by: Kirill Shutemov <email@example.com>
> Acked-by: Jan Kara <firstname.lastname@example.org>
> Cc: Andrea Arcangeli <email@example.com>
> Cc: Matthew Wilcox <firstname.lastname@example.org>
> Signed-off-by: Linus Torvalds <email@example.com>
> [surenb: backport to 4.19 kernel]
> Cc: firstname.lastname@example.org # 4.19.x
> Signed-off-by: Suren Baghdasaryan <email@example.com>
> mm/gup.c | 44 ++++++++++++++++++++++++++++++++++++++------
> mm/huge_memory.c | 7 +++----
> 2 files changed, 41 insertions(+), 10 deletions(-)
Thanks for these backports, I've now queued them up.
next prev parent reply other threads:[~2021-04-23 15:05 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-21 22:56 Suren Baghdasaryan
2021-04-23 15:05 ` Greg KH [this message]
2021-04-21 22:57 Suren Baghdasaryan
2021-10-12 1:52 Suren Baghdasaryan
2021-10-12 1:55 ` Suren Baghdasaryan
2021-10-12 5:45 ` Greg Kroah-Hartman
2021-10-12 8:06 ` Jan Kara
2021-10-12 8:14 ` Vlastimil Babka
2021-10-12 8:42 ` Greg KH
2021-10-12 18:57 ` Thadeu Lima de Souza Cascardo
2021-10-13 8:56 ` Greg KH
2021-10-12 10:06 ` Greg KH
2021-10-12 16:16 ` Suren Baghdasaryan
2021-10-13 21:58 ` John Hubbard
2021-10-12 1:53 Suren Baghdasaryan
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--subject='Re: [PATCH 1/1] gup: document and work around "COW can break either way" issue' \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).