From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EA94BC433E0 for ; Tue, 22 Dec 2020 04:16:34 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 3556A22B2B for ; Tue, 22 Dec 2020 04:16:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3556A22B2B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linux-foundation.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 487CD8D0001; Mon, 21 Dec 2020 23:16:33 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 438BE6B0099; Mon, 21 Dec 2020 23:16:33 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 326B48D0001; Mon, 21 Dec 2020 23:16:33 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0182.hostedemail.com [216.40.44.182]) by kanga.kvack.org (Postfix) with ESMTP id 190D56B0098 for ; Mon, 21 Dec 2020 23:16:33 -0500 (EST) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id D610F3641 for ; Tue, 22 Dec 2020 04:16:32 +0000 (UTC) X-FDA: 77619606624.29.honey88_340588f2745d Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin29.hostedemail.com (Postfix) with ESMTP id BD0D6180868EE for ; Tue, 22 Dec 2020 04:16:32 +0000 (UTC) X-HE-Tag: honey88_340588f2745d X-Filterd-Recvd-Size: 8964 Received: from mail-lf1-f41.google.com (mail-lf1-f41.google.com [209.85.167.41]) by imf16.hostedemail.com (Postfix) with ESMTP for ; Tue, 22 Dec 2020 04:16:32 +0000 (UTC) Received: by mail-lf1-f41.google.com with SMTP id b26so19345140lff.9 for ; Mon, 21 Dec 2020 20:16:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=C/IyHJco45Qv6BHyymqm5fsLF1iVOYE2Rc667l3brbw=; b=IR0T55DmJ9mkOAt5PW8Gqr31ryut37JKHFwkKEKNUnz84Mk9NwhsAQe1HxZsHQ7zmB MdR95VrHiV3LKUGcBK3v6VgHVBsa/aLcrgAlzNjpqqiV6O7hKjnM4BwDsBhDCmFtduFt aGec+wYY5jH8H2b3n90B1OMSiOJSvUAF62wS0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=C/IyHJco45Qv6BHyymqm5fsLF1iVOYE2Rc667l3brbw=; b=Bo+RcgksfXTlF72Y/GwAEREqVSg6bwWM7y+OLqZV6f/+IWFnoHbZLr+02ctsghxRhW 9gZ56xATbXvlbEnT3Xk3GKjZemNbqApHADpxTFZy2qQxA4X5A8GFOh0QzFiD1OIFZ1PS z9n+qIym7NaGeCAyS62dMh7MirQTkPSQiTrlQtK7IpIOQ1McvJ4vaXVsxCdoQUrjrBZW Z+vf9rUV4drPtxtYp4n2v4LTpAeMpjMMYblYEd1D+Po0aZDj4BmBw/0i5W8GXBpWXCjo sKvS2ZVTTguc8yAtA+HCProeZG8FVJdFuEAj49c7OWD0L2QbUSKCjHKWL/CfY+e/z5sS 542w== X-Gm-Message-State: AOAM5307BksGv5twJEGwxMMhinJQqMrzEzRw7Mbz+J1BEdYz4GpuMjcs IWwznkaas/KEqRGFToIqbIwkWbxo/em1ZA== X-Google-Smtp-Source: ABdhPJz363+kerLNNIUK078q5/90FL0YRvSmWwDciRJKqCaP08l1bNN3dxdPZ0IvxrmaJSTFNcpleg== X-Received: by 2002:ac2:5cac:: with SMTP id e12mr7626305lfq.216.1608610589498; Mon, 21 Dec 2020 20:16:29 -0800 (PST) Received: from mail-lf1-f42.google.com (mail-lf1-f42.google.com. [209.85.167.42]) by smtp.gmail.com with ESMTPSA id t3sm2381705lfe.263.2020.12.21.20.16.27 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 21 Dec 2020 20:16:28 -0800 (PST) Received: by mail-lf1-f42.google.com with SMTP id o13so28875174lfr.3 for ; Mon, 21 Dec 2020 20:16:27 -0800 (PST) X-Received: by 2002:a2e:b4af:: with SMTP id q15mr8557233ljm.507.1608610587583; Mon, 21 Dec 2020 20:16:27 -0800 (PST) MIME-Version: 1.0 References: <20201221172711.GE6640@xz-x1> <76B4F49B-ED61-47EA-9BE4-7F17A26B610D@gmail.com> <9E301C7C-882A-4E0F-8D6D-1170E792065A@gmail.com> <1FCC8F93-FF29-44D3-A73A-DF943D056680@gmail.com> <20201221223041.GL6640@xz-x1> In-Reply-To: From: Linus Torvalds Date: Mon, 21 Dec 2020 20:16:11 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect To: Andy Lutomirski Cc: Peter Xu , Nadav Amit , Yu Zhao , Andrea Arcangeli , linux-mm , lkml , Pavel Emelyanov , Mike Kravetz , Mike Rapoport , stable , Minchan Kim , Will Deacon , Peter Zijlstra Content-Type: text/plain; charset="UTF-8" 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: On Mon, Dec 21, 2020 at 7:19 PM Andy Lutomirski wrote: > > Ugh, this is unpleasantly complicated. I probably should have phrased it differently, because the case you quote is actually a good one: > I will admit that any API that > takes an address and more-or-less-blindly marks it RO makes me quite > nervous even assuming all the relevant locks are held. At least > userfaultfd refuses to operate on VM_SHARED VMAs, but we have another > instance of this (with mmap_sem held for write) in x86: > mark_screen_rdonly(). Dare I ask how broken this is? We could likely > get away with deleting it entirely. So I think the basic rule is that "if you hold mmap_sem for writing, you're always safe". And that really should be considered the "default" locking. ANY time you make a modification to the VM layer, you should basically always treat it as a write operation, and get the mmap_sem for writing. Yeah, yeah, that's a bit simplified, and it ignores various special cases (and the hardware page table walkers that obviously take no locks at all), but if you hold the mmap_sem for writing you won't really race with anything else - not page faults, and not other "modify this VM". So mark_screen_rdonly() looks trivially fine to me. I don't think it really necessarily matters any more, and it's a legacy thing for a legacy hardware issue, but I don't think there's any reason at all to remove it either. To a first approximation, everybody that changes the VM should take the mmap_sem for writing, and the readers should just be just about page fault handling (and I count GUP as "page fault handling" too - it's kind of the same "look up page" rather than "modify vm" kind of operation). And there are just a _lot_ more page faults than there are things that modify the page tables and the vma's. So having that mental model of "lookup of pages in a VM take mmap_semn for reading, any modification of the VM uses it for writing" makes sense both from a performance angle and a logical standpoint. It's the correct model. And it's worth noting that COW is still "lookup of pages", even though it might modify the page tables in the process. The same way lookup can modify the page tables to mark things accessed or dirty. So COW is still a lookup operation, in ways that "change the writabiility of this range" very much is not. COW is "lookup for write", and the magic we do to copy to make that write valid is still all about the lookup of the page. Which brings up another mental mistake I saw earlier in this thread: you should not think "mmap_sem is for vma, and the page table lock is for the page table changes". mmap_sem is the primary lock for any modifications to the VM layout, whether it be in the vma's or in the page tables. Now, the page table lock does exist _in_addition_to_ the mmap_sem, but it is partly because (a) we have things that historically walked the page tables _without_ walking the vma's (notably the virtual memory scanning) (b) we do allow concurrent page faults, so we then need a lower-level lock to serialize the parallelism we _do_ have. And yes, we have ended up allowing some of those "modify the VM state" to then take the mmap_sem for reading, just because their modifications are slight enough that we can then say "ok, this is a write modification, but the writes it does are protected by the page table lock, we'll just get the mmap_sem for reading". It's an optimization, but it should be considered exactly that: not fundamental, but just a clever trick and an optimization. It's why I went "userfaultfd is buggy" immediately when I noticed. It basically became clear that "oh, that's an optimization, but it's an *invalid* one", in that it didn't actually work and give the same end result. So when I said: > Anything that changes RW->RO - like fork(), for example - needs to > take the mmap_lock. I didn't mean that we should consider that RW->RO change to be this complex semantic marker that we should honor and say "ok, because it's a RW->RO change, we need to hold the mmap_sem". I phrased it really badly, in other words. What I *should* have said is that *because* userfaultfd is changing the VM layout, it should always act as if it had to take the mmap_sem for writing, and that the RW->RO change is an example of when downgrading that write-lock to a read lock is simply not valid - because it basically breaks the rules about what a lookup (ie a read) can do. A lookup can never turn a writable page non-writable. A lookup - through COW - _can_ turn a read-only page writable. So that's why "RO->RW" can be ok under the read lock, but RW->RO is not. Does that clarify the situation when I phrase it that way instead? Linus