LKML Archive on lore.kernel.org help / color / mirror / Atom feed
From: Greg KH <firstname.lastname@example.org> To: Vlastimil Babka <email@example.com> Cc: Jan Kara <firstname.lastname@example.org>, Suren Baghdasaryan <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, 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 Subject: Re: [PATCH 1/1] gup: document and work around "COW can break either way" issue Date: Tue, 12 Oct 2021 10:42:40 +0200 [thread overview] Message-ID: <YWVKgIUY6hWagEPo@kroah.com> (raw) In-Reply-To: <email@example.com> On Tue, Oct 12, 2021 at 10:14:27AM +0200, Vlastimil Babka wrote: > On 10/12/21 10:06, Jan Kara wrote: > > On Mon 11-10-21 18:52:44, Suren Baghdasaryan wrote: > >> From: Linus Torvalds <firstname.lastname@example.org> > >> > >> 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 > >> 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.] > > > > I'd be really careful with backporting this to stable. There was a lot of > > userspace breakage caused by this change if I remember right which needed > > to be fixed up later. There is a nice summary at > > https://lwn.net/Articles/849638/ and https://lwn.net/Articles/849876/ and > > some problems are still being found... > > Yeah that was my initial reaction. But looks like back in April we agreed > that backporting only this commit could be feasible - the relevant subthread > starts around here . The known breakage for just this commit was uffd > functionality introduced only in 5.7, and strace on dax on pmem (that was > never properly root caused). 5.4 stable already has the backport since year > ago, Suren posted 4.14 and 4.19 in April after . Looks like nobody > reported issues? Continuing with 4.4 and 4.9 makes this consistent at least, > although the risk of breaking something is always there and the CVE probably > not worth it, but whatever... I have had people "complain" that the issue was not fixed on these older kernels, now if that is just because those groups have a "it has a CVE so it must be fixed!" policy or not, it is hard to tell. But this seems to be exploitable, and we have a reproducer somewhere around here, so it would be nice to get resolved for the reason of it being a bug that we should fix if possible. So I would err on the side of "lets merge this" as fixing a known issue is ALWAYS better than the fear of "maybe something might break". We can always revert if the latter happens in testing. thanks, greg k-h
next prev parent reply other threads:[~2021-10-12 8:42 UTC|newest] Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-12 1:52 [PATCH 1/1] gup: document and work around "COW can break either way" issue 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 [this message] 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 -- strict thread matches above, loose matches on Subject: below -- 2021-10-12 1:53 Suren Baghdasaryan 2021-04-21 22:57 Suren Baghdasaryan 2021-04-21 22:56 Suren Baghdasaryan 2021-04-23 15:05 ` Greg KH
Reply instructions: 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: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YWVKgIUY6hWagEPo@kroah.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 \ --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 \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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).