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.7 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,URIBL_BLOCKED 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 E1DAFC433E0 for ; Wed, 23 Dec 2020 09:45:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A2759224BE for ; Wed, 23 Dec 2020 09:45:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728194AbgLWJpo (ORCPT ); Wed, 23 Dec 2020 04:45:44 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47886 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727876AbgLWJpn (ORCPT ); Wed, 23 Dec 2020 04:45:43 -0500 Received: from mail-lf1-x131.google.com (mail-lf1-x131.google.com [IPv6:2a00:1450:4864:20::131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CA7D5C0613D6 for ; Wed, 23 Dec 2020 01:45:02 -0800 (PST) Received: by mail-lf1-x131.google.com with SMTP id y19so38545522lfa.13 for ; Wed, 23 Dec 2020 01:45:02 -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=nGD/IzQ8B854voURxPB+V56vRqmJO6UpGkJrR67WHaw=; b=Q/sgS5Hi/obloj/5gsQkvmd2wLqZ6prYhK0W1YOEK2FQvZDEIB/tM9W58WLSQDPHDd rgZygccbXO5HlUib555+1dyxTlbXIqg45ICso56K8SBlVGavSYBi8xZPNBtkSGOke1cB xgiKQ4BGaiLF563JI063gBUzVI0x6h/e/YM6U= 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=nGD/IzQ8B854voURxPB+V56vRqmJO6UpGkJrR67WHaw=; b=EheHe+F5p6mS/BdHTsMl7QxJv+7gKigN7WaMUcczmpTnLBPKI+GQ5UekK/Lyvse75W hdJEc72jXCRuGIKOHLgtFkkbwEYC5Lod/yy2ixxOymLNHECl0rJlH8WBAUlW8X3zOSdU Od8pY5uHO6BnKNNAUyXmxnTXDp+aeP+/NN9ZyZntbK6DMeUeqTTbwYZfaPv8Mp9YV4SG bN6NewBLssa3Om9GBtclSJ7tjX109et0R4MjZweOLD85m0E8ZLJWhLFjXTcDwUlZ7bfz /VM4DPBeQioROsl1sTrywmh23Za+C3rZCNekVnIPvmoBr+Rr700hDj5sE+7IaVOYt/IW LtmQ== X-Gm-Message-State: AOAM532sTIR91Ze+aylvGU8UuDhjjAVaypiDpjoSWH9D7ZKcrI2aOhDj NK4rrRUVyaIEiz4krs4iNWXvUVRcPVsLOA== X-Google-Smtp-Source: ABdhPJzUpe0xErTf0mDXQxeO5pnsH6nyyVNzRQLIA7qSKjpSTO1qVKs1agGxzjjdFH9JoHY3u0o7cw== X-Received: by 2002:a2e:a584:: with SMTP id m4mr11049486ljp.247.1608716700797; Wed, 23 Dec 2020 01:45:00 -0800 (PST) Received: from mail-lf1-f54.google.com (mail-lf1-f54.google.com. [209.85.167.54]) by smtp.gmail.com with ESMTPSA id u14sm3110074lfk.108.2020.12.23.01.44.59 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 23 Dec 2020 01:44:59 -0800 (PST) Received: by mail-lf1-f54.google.com with SMTP id h22so29162862lfu.2 for ; Wed, 23 Dec 2020 01:44:59 -0800 (PST) X-Received: by 2002:a19:7d85:: with SMTP id y127mr10914984lfc.253.1608716699063; Wed, 23 Dec 2020 01:44:59 -0800 (PST) MIME-Version: 1.0 References: <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: Wed, 23 Dec 2020 01:44:42 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect To: Yu Zhao Cc: Andrea Arcangeli , Andy Lutomirski , Peter Xu , Nadav Amit , linux-mm , lkml , Pavel Emelyanov , Mike Kravetz , Mike Rapoport , stable , Minchan Kim , Will Deacon , Peter Zijlstra Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Dec 22, 2020 at 4:01 PM Linus Torvalds wrote: > > The more I look at the mprotect code, the less I like it. We seem to > be much better about the TLB flushes in other places (looking at > mremap, for example). The mprotect code seems to be very laissez-faire > about the TLB flushing. No, this doesn't help. > Does adding a TLB flush to before that > > pte_unmap_unlock(pte - 1, ptl); > > fix things for you? It really doesn't fix it. Exactly because - as pointed out earlier - the actual page *copy* happens outside the pte lock. So what can happen is: - CPU 1 holds the page table lock, while doing the write protect. It has cleared the writable bit, but hasn't flushed the TLB's yet - CPU 2 did *not* have the TLB entry, sees the new read-only state, takes a COW page fault, and reads the PTE from memory (into vmf->orig_pte) - CPU 2 correctly decides it needs to be a COW, and copies the page contents - CPU 3 *does* have a stale TLB (because TLB invalidation hasn't happened yet), and writes to that page in users apce - CPU 1 now does the TLB invalidate, and releases the page table lock - CPU 2 gets the page table lock, sees that its PTE matches vmf->orig_pte, and switches it to be that writable copy of the page. where the copy happened before CPU 3 had stopped writing to the page. So the pte lock doesn't actually matter, unless we actually do the page copy inside of it (on CPU2), in addition to doing the TLB flush inside of it (on CPU1). mprotect() is actually safe for two independent reasons: (a) it does the mmap_sem for writing (so mprotect can't race with the COW logic at all), and (b) it changes the vma permissions so turning something read-only actually disables COW anyway, since it won't be a COW, it will be a SIGSEGV. So mprotect() is irrelevant, other than the fact that it shares some code with that "turn it read-only in the page tables". fork() is a much closer operation, in that it actually triggers that COW behavior, but fork() takes the mmap_sem for writing, so it avoids this too. So it's really just userfaultfd and that kind of ilk that is relevant here, I think. But that "you need to flush the TLB before releasing the page table lock" was not true (well, it's true in other circumstances - just not *here*), and is not part of the solution. Or rather, if it's part of the solution here, it would have to be matched with that "page copy needs to be done under the page table lock too". Linus