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.1 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 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 77B03C433DB for ; Wed, 6 Jan 2021 00:30:03 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id D62EB22D71 for ; Wed, 6 Jan 2021 00:30:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D62EB22D71 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id D9B1C8D00C9; Tue, 5 Jan 2021 19:30:01 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D24368D006E; Tue, 5 Jan 2021 19:30:01 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BC48E8D00C9; Tue, 5 Jan 2021 19:30:01 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0021.hostedemail.com [216.40.44.21]) by kanga.kvack.org (Postfix) with ESMTP id A6B548D006E for ; Tue, 5 Jan 2021 19:30:01 -0500 (EST) Received: from smtpin17.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 6381A181AEF1A for ; Wed, 6 Jan 2021 00:30:01 +0000 (UTC) X-FDA: 77673467802.17.fight87_250efee274dd Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin17.hostedemail.com (Postfix) with ESMTP id 4D7ED180D0185 for ; Wed, 6 Jan 2021 00:30:01 +0000 (UTC) X-HE-Tag: fight87_250efee274dd X-Filterd-Recvd-Size: 6649 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by imf05.hostedemail.com (Postfix) with ESMTP for ; Wed, 6 Jan 2021 00:30:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1609892999; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=lCbjpgDmnTMB324ACAGwgrr0FLk5GHKNaJCMbIN6Iis=; b=LejaXoB32x9TpNbIzEoWQ5iLAj8y/D3wQKvHOeqgikyTogHIRFl/ibs00/vgJOC4VPrhb2 Hhk9Xq55DOSjf4mEM+ryN0SpUyqvjtmDfTojNgFgOC5el+0ZojRKCE1wfcMcNZo6sH9QEi Xy5SenmnLy8CMwVnHwFA26mNr+IQElI= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-39-ickF_wgCNsCRvbalVRc8Jg-1; Tue, 05 Jan 2021 19:29:58 -0500 X-MC-Unique: ickF_wgCNsCRvbalVRc8Jg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 66871107ACE3; Wed, 6 Jan 2021 00:29:56 +0000 (UTC) Received: from mail (ovpn-112-76.rdu2.redhat.com [10.10.112.76]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D5FCA50DD5; Wed, 6 Jan 2021 00:29:52 +0000 (UTC) Date: Tue, 5 Jan 2021 19:29:52 -0500 From: Andrea Arcangeli To: Will Deacon Cc: Nadav Amit , linux-mm , lkml , Yu Zhao , Andy Lutomirski , Peter Xu , Pavel Emelyanov , Mike Kravetz , Mike Rapoport , Minchan Kim , Peter Zijlstra Subject: Re: [RFC PATCH v2 2/2] fs/task_mmu: acquire mmap_lock for write on soft-dirty cleanup Message-ID: References: <20201225092529.3228466-1-namit@vmware.com> <20201225092529.3228466-3-namit@vmware.com> <15758743-B8E3-48C4-A13B-DFFEBF8AF435@vmware.com> <20210105221628.GA12854@willie-the-truck> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210105221628.GA12854@willie-the-truck> User-Agent: Mutt/2.0.4 (2020-12-30) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Content-Transfer-Encoding: quoted-printable 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 Tue, Jan 05, 2021 at 10:16:29PM +0000, Will Deacon wrote: > On Tue, Jan 05, 2021 at 09:22:51PM +0000, Nadav Amit wrote: > > > On Jan 5, 2021, at 12:39 PM, Andrea Arcangeli = wrote: > > >=20 > > > On Tue, Jan 05, 2021 at 07:26:43PM +0000, Nadav Amit wrote: > > >>> On Jan 5, 2021, at 10:20 AM, Andrea Arcangeli wrote: > > >>>=20 > > >>> On Fri, Dec 25, 2020 at 01:25:29AM -0800, Nadav Amit wrote: > > >>>> Fixes: 0f8975ec4db2 ("mm: soft-dirty bits for user memory change= s tracking") > > >>>=20 > > >>> Targeting a backport down to 2013 when nothing could wrong in pra= ctice > > >>> with page_mapcount sounds backwards and unnecessarily risky. > > >>>=20 > > >>> In theory it was already broken and in theory > > >>> 09854ba94c6aad7886996bfbee2530b3d8a7f4f4 is absolutely perfect an= d the > > >>> previous code of 2013 is completely wrong, but in practice the co= de > > >>> from 2013 worked perfectly until Aug 21 2020. > > >>=20 > > >> Well=E2=80=A6 If you consider the bug that Will recently fixed [1]= , then soft-dirty > > >> was broken (for a different, yet related reason) since 0758cd83049= 4 > > >> ("asm-generic/tlb: avoid potential double flush=E2=80=9D). > > >>=20 > > >> This is not to say that I argue that the patch should be backporte= d to 2013, > > >> just to say that memory corruption bugs can be unnoticed. > > >>=20 > > >> [1] https://patchwork.kernel.org/project/linux-mm/patch/2020121012= 1110.10094-2-will@kernel.org/ > > >=20 > > > Is this a fix or a cleanup? > > >=20 > > > The above is precisely what I said earlier that tlb_gather had no > > > reason to stay in clear_refs and it had to use inc_tlb_flush_pendin= g > > > as mprotect, but it's not a fix? Is it? I suggested it as a pure > > > cleanup. So again no backport required. The commit says fix this bu= t > > > it means "clean this up". > >=20 > > It is actually a fix. I think the commit log is not entirely correct = and > > should include: > >=20 > > Fixes: 0758cd830494 ("asm-generic/tlb: avoid potential double flush= =E2=80=9D). Agreed. > >=20 > > Since 0758cd830494, calling tlb_finish_mmu() without any previous cal= l to > > pte_free_tlb() and friends does not flush the TLB. The soft-dirty bug > > producer that I sent fails without this patch of Will. >=20 > Yes, it's a fix, but I didn't rush it for 5.10 because I don't think ru= shing > this sort of thing does anybody any favours. I agree that the commit lo= g > should be updated; I mentioned this report in the cover letter: >=20 > https://lore.kernel.org/linux-mm/CA+32v5zzFYJQ7eHfJP-2OHeR+6p5PZsX=3DRD= JNU6vGF3hLO+j-g@mail.gmail.com/ >=20 > demonstrating that somebody has independently stumbled over the missing= TLB > invalidation in userspace, but it's not as bad as the other issues we'v= e been > discussing in this thread. Thanks for the explanation Nadav and Will. The fact the code was a 100% match to the cleanup I independently suggested a few weeks ago to reduce the confusion in clear_refs, made me overlook the difference 0758cd830494 made. I didn't realize the flush got optimized away if no gathering happened. Backporting this sort of thing with Fixes I guess tends to give the same kind of favors as rushing it for 5.10, but then in general the Fixes is accurate here. Overall it looks obviously safe cleanup and it is also a fix starting in v5.6, so I don't think this can cause more issues than what it sure fixes at least. The cleanup was needed anyway, even before it become a fix, since if it was mandatory to use tlb_gather when you purely need inc_tlb_flush_pending, then mprotect couldn't get away with it. I guess the the optimization in 0758cd830494 just made it more explicit that no code should use tlb_gather if it doesn't need to gather any page. Maybe adding some commentary in the comment on top of tlb_gather_mmu about the new behavior wouldn't hurt.