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=-13.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=unavailable 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 1A409C56202 for ; Tue, 24 Nov 2020 01:15:06 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7DAAB20728 for ; Tue, 24 Nov 2020 01:15:05 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="LYKHkyJA"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="vquXySr+" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7DAAB20728 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=aTWAEMhGMfmmqaujD4ZHSEJuVi0heJYOTU8v7shE+Iw=; b=LYKHkyJAyh6vXW7kL9G6Hjx6p vwGUMUEWcVtwprlo5EA8Fl3Kw9e4gHBkTHCj53fwdxteBftONcHFX2VD4szscr6bIePA8ECv2PbzI T8vjhRgItgAeiO9oCcmxycXuEymStq7GXfCHL9ysWfTnA6OnK/jraNzNm/x6EAUKOIuNS0lsX5cPa ikJNSIMreJLTGBqxpDhksZGFcOWeWidsgFzxUXEFq0GPRqJY5vhIMMNb4hMHWvrF+erXdqWtr21Iv inXIj7PoUHBoNLrr6Rzih/I/vPKEqxfm976sNe4A3Xek2la2+yF/e7/Az/wPOKjwEtL9+zQNTlQP+ p+z7K88Hw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1khMtv-0007JR-Pu; Tue, 24 Nov 2020 01:13:47 +0000 Received: from mail-io1-xd44.google.com ([2607:f8b0:4864:20::d44]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1khMtt-0007I8-9y for linux-arm-kernel@lists.infradead.org; Tue, 24 Nov 2020 01:13:46 +0000 Received: by mail-io1-xd44.google.com with SMTP id o11so20183189ioo.11 for ; Mon, 23 Nov 2020 17:13:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=y1iZnuxmqMO3+jS4X8m+HK41h/MIptRIpHqE/xa6Iu4=; b=vquXySr+NJ+bDYOXcKipvjCDROTMY3rz/3P3rsSyvM9cqfkH65ighuwhtJYTuY3JGc dsxbUD93+EsA3dzieyAZ9Rl/5VZ71gZJ8K8YvmVL9S9/4p2fkUbgbvTRwL5MlkcLNRhT gv+4urOTtN1UQ5Yop83TCGmLvH1jWEose00HeBAxOE1TVQoNSM/WPI4yY33ajhJ+WBa2 y8qeMt6g9A6P9gk+Uq18ZqLuG/aW4Is2hurh5dht9KDWvzEFHP5ZRFFAEWylT/WFrB9n buRbrrjOmspATYfn7H5IJibkVWFHXODLUXxTXHlrOyShyUIvNwF3uXJuRi2KDmUqWtDy A9yQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=y1iZnuxmqMO3+jS4X8m+HK41h/MIptRIpHqE/xa6Iu4=; b=q9gavdJtGRoYHYXFak8aktACh46WSjY0A/rQfh8VYLUEqPKbCfCBEz38UPE7I6GMtc XZhY2fx6RuPu0ZF62kCBye8tuwCFLFfLG1GqsigDZIba1rYg+AqVU+W+exzhEuwmKe/X wzzBPGyXn66wz66QqOCYAj0ShdCvINv520zOHEbeTxgPw7jaWrt9xnNuQAM43ZNIFIU9 NKapdAVLQNYNCEWvmUA5xvUorcNdYQcCwDiggu4cw8Z9Nt14kQSKaJpLO4+tDLcb8YEl H4jhiZ09tsX/5O/4UX2Ungai8rW4hfTAA8OSy4vVaO8Fh8DVJswIwrkjUlSbnptt+5WI JW9A== X-Gm-Message-State: AOAM533g9Rm/YCFnVJbxCmCF0nqBKWXYetAAPbaO+MccsYXv6mvFGoSx z26h+5r23f6SUFEslQJVpej85A== X-Google-Smtp-Source: ABdhPJwhKELBuv/Bq74t/eA6OALgCjZWbDCFtNV4tku+5eU+QjwLkRPBxhUzkID53lZoekutus7G/A== X-Received: by 2002:a02:290e:: with SMTP id p14mr2259664jap.105.1606180418955; Mon, 23 Nov 2020 17:13:38 -0800 (PST) Received: from google.com ([2620:15c:183:200:7220:84ff:fe09:2d90]) by smtp.gmail.com with ESMTPSA id a3sm8260690ilp.5.2020.11.23.17.13.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 23 Nov 2020 17:13:38 -0800 (PST) Date: Mon, 23 Nov 2020 18:13:34 -0700 From: Yu Zhao To: Will Deacon Subject: Re: [PATCH 6/6] mm: proc: Avoid fullmm flush for young/dirty bit toggling Message-ID: <20201124011334.GA140483@google.com> References: <20201120143557.6715-1-will@kernel.org> <20201120143557.6715-7-will@kernel.org> <20201120204005.GC1303870@google.com> <20201123183554.GC11688@willie-the-truck> <20201123200403.GA3888699@google.com> <20201123211750.GA12069@willie-the-truck> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201123211750.GA12069@willie-the-truck> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201123_201345_379410_25262C66 X-CRM114-Status: GOOD ( 44.45 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kernel-team@android.com, Anshuman Khandual , Peter Zijlstra , Catalin Marinas , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Minchan Kim , Linus Torvalds , linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Nov 23, 2020 at 09:17:51PM +0000, Will Deacon wrote: > On Mon, Nov 23, 2020 at 01:04:03PM -0700, Yu Zhao wrote: > > On Mon, Nov 23, 2020 at 06:35:55PM +0000, Will Deacon wrote: > > > On Fri, Nov 20, 2020 at 01:40:05PM -0700, Yu Zhao wrote: > > > > On Fri, Nov 20, 2020 at 02:35:57PM +0000, Will Deacon wrote: > > > > > clear_refs_write() uses the 'fullmm' API for invalidating TLBs after > > > > > updating the page-tables for the current mm. However, since the mm is not > > > > > being freed, this can result in stale TLB entries on architectures which > > > > > elide 'fullmm' invalidation. > > > > > > > > > > Ensure that TLB invalidation is performed after updating soft-dirty > > > > > entries via clear_refs_write() by using the non-fullmm API to MMU gather. > > > > > > > > > > Signed-off-by: Will Deacon > > > > > --- > > > > > fs/proc/task_mmu.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > > > > > index a76d339b5754..316af047f1aa 100644 > > > > > --- a/fs/proc/task_mmu.c > > > > > +++ b/fs/proc/task_mmu.c > > > > > @@ -1238,7 +1238,7 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf, > > > > > count = -EINTR; > > > > > goto out_mm; > > > > > } > > > > > - tlb_gather_mmu_fullmm(&tlb, mm); > > > > > + tlb_gather_mmu(&tlb, mm, 0, TASK_SIZE); > > > > > > > > Let's assume my reply to patch 4 is wrong, and therefore we still need > > > > tlb_gather/finish_mmu() here. But then wouldn't this change deprive > > > > architectures other than ARM the opportunity to optimize based on the > > > > fact it's a full-mm flush? > > > > I double checked my conclusion on patch 4, and aside from a couple > > of typos, it still seems correct after the weekend. > > I still need to digest that, but I would prefer that we restore the > invalidation first, and then have a subsequent commit to relax it. I find > it hard to believe that the behaviour in mainline at the moment is deliberate. > > That is, I'm not against optimising this, but I'd rather get it "obviously > correct" first and the current code is definitely not that. I wouldn't mind having this patch and patch 4 if the invalidation they restore were in a correct state -- b3a81d0841a9 ("mm: fix KSM data corruption") isn't correct to start with. It is complicated, so please bear with me. Let's study this by looking at examples this time. > > > Only for the soft-dirty case, but I think TLB invalidation is required > > > there because we are write-protecting the entries and I don't see any > > > mechanism to handle lazy invalidation for that (compared with the aging > > > case, which is handled via pte_accessible()). > > > > The lazy invalidation for that is done when we write-protect a page, > > not an individual PTE. When we do so, our decision is based on both > > the dirty bit and the writable bit on each PTE mapping this page. So > > we only need to make sure we don't lose both on a PTE. And we don't > > here. > > Sorry, I don't follow what you're getting at here (page vs pte). Please can > you point me to the code you're referring to? The case I'm worried about is > code that holds sufficient locks (e.g. mmap_sem + ptl) finding an entry > where !pte_write() and assuming (despite pte_dirty()) that there can't be > any concurrent modifications to the mapped page. Granted, I haven't found > anything doing that, but I could not convince myself that it would be a bug > to write such code, either. Example 1: memory corruption is still possible with patch 4 & 6 CPU0 CPU1 CPU2 CPU3 ---- ---- ---- ---- userspace page writeback [cache writable PTE in TLB] inc_tlb_flush_pending() clean_record_pte() pte_mkclean() tlb_gather_mmu() [set mm_tlb_flush_pending()] clear_refs_write() pte_wrprotect() page_mkclean_one() !pte_dirty() && !pte_write() [true, no flush] write page to disk Write to page [using stale PTE] drop clean page [data integrity compromised] flush_tlb_range() tlb_finish_mmu() [flush (with patch 4)] Example 2: why no flush when write-protecting is not a problem (after we fix the problem correctly by adding mm_tlb_flush_pending()). Case a: CPU0 CPU1 CPU2 CPU3 ---- ---- ---- ---- userspace page writeback [cache writable PTE in TLB] inc_tlb_flush_pending() clean_record_pte() pte_mkclean() clear_refs_write() pte_wrprotect() page_mkclean_one() !pte_dirty() && !pte_write() && !mm_tlb_flush_pending() [false: flush] write page to disk Write to page [page fault] drop clean page [data integrity guaranteed] flush_tlb_range() Case b: CPU0 CPU1 CPU2 ---- ---- ---- userspace page writeback [cache writable PTE in TLB] clear_refs_write() pte_wrprotect() [pte_dirty() is false] page_mkclean_one() !pte_dirty() && !pte_write() && !mm_tlb_flush_pending() [true: no flush] write page to disk Write to page [h/w tries to set the dirty bit but sees write- protected PTE, page fault] drop clean page [data integrity guaranteed] Case c: CPU0 CPU1 CPU2 ---- ---- ---- userspace page writeback [cache writable PTE in TLB] clear_refs_write() pte_wrprotect() [pte_dirty() is true] page_mkclean_one() !pte_dirty() && !pte_write() && !mm_tlb_flush_pending() [false: flush] write page to disk Write to page [page fault] drop clean page [data integrity guaranteed] > > > Furthermore, If we decide that we can relax the TLB invalidation > > > requirements here, then I'd much rather than was done deliberately, rather > > > than as an accidental side-effect of another commit (since I think the > > > current behaviour was a consequence of 7a30df49f63a). > > > > Nope. tlb_gather/finish_mmu() should be added by b3a81d0841a9 ^^^^^^ shouldn't Another typo, I apologize. > > ("mm: fix KSM data corruption") in the first place. > > Sure, but if you check out b3a81d0841a9 then you have a fullmm TLB > invalidation in tlb_finish_mmu(). 7a30df49f63a is what removed that, no? > > Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel