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=-2.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, 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 4E03EC4361B for ; Sun, 20 Dec 2020 08:06:45 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B210823A5E for ; Sun, 20 Dec 2020 08:06:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B210823A5E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id C3E706B005C; Sun, 20 Dec 2020 03:06:43 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id BC77A6B005D; Sun, 20 Dec 2020 03:06:43 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A8EB66B0068; Sun, 20 Dec 2020 03:06:43 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0169.hostedemail.com [216.40.44.169]) by kanga.kvack.org (Postfix) with ESMTP id 88C966B005C for ; Sun, 20 Dec 2020 03:06:43 -0500 (EST) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 55C90249E for ; Sun, 20 Dec 2020 08:06:43 +0000 (UTC) X-FDA: 77612929086.29.wrist72_2c0eb252744d Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin29.hostedemail.com (Postfix) with ESMTP id 3722B18086CB5 for ; Sun, 20 Dec 2020 08:06:43 +0000 (UTC) X-HE-Tag: wrist72_2c0eb252744d X-Filterd-Recvd-Size: 7216 Received: from mail-pj1-f52.google.com (mail-pj1-f52.google.com [209.85.216.52]) by imf11.hostedemail.com (Postfix) with ESMTP for ; Sun, 20 Dec 2020 08:06:42 +0000 (UTC) Received: by mail-pj1-f52.google.com with SMTP id b5so4665038pjk.2 for ; Sun, 20 Dec 2020 00:06:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=3Wcun+zv/hhQE2K3TcWCKWB24fTPBiCA4UQ28awSatM=; b=I7yhK5wXqmGnt4AwF/BbAz/ynHIuyVIarnQXVsSVeupjujJdEMpZ6QnSvmvqaZE5fg AGk5XJbnKwCrWDICN0byKxPNEYzqXPCQkZHrjG0XPebHvPK9D8kHudqiwmBAgZs7KBt+ i+Ff8k3SQWJdFj6Byu434Xs9126dZnExTR5qvwjNCyi6SS+R4TD4gIxNJnuGDQ+2/+7m icjJp+JmD29qB2fZfnD6eEuhffOk5Dsio5fJmoGOdazVwFJEiL333USGDYPqlBeioLcs Qh8tsnahBUi2M/nMggb4SLqn63m5HU9CeOOSgZtbhWIaZRj5czh/HZFbsfnL8D+dA5IH m3ig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=3Wcun+zv/hhQE2K3TcWCKWB24fTPBiCA4UQ28awSatM=; b=lVhM9kFitR5NXQdUgbk50PW7y2IrdsQLwtWQtkZmWFP6VPrHSQAbeVwteXkV3VDcHW z9DeXOuWk3ZsN9jtNpIlnmcyhm5SDMu88bDc+rIfl4sjrL+g/n9vkc7+7PzMnnRvKrH5 ioHpz+g2bgsW9U4Mt4k4YqPoAFjg5WPDaYJrnmw+i6BMGCgfbFmC9lBQh6oOVRejxxpG f9MHaN9Kxodr386cn+WhjDs5kfun/FaVEYzwXp/lIrmJhOezQhI32oUDCjR9ejiTL/vb qdJil9uaFbcjdDgFGhL6ZP3DA4AvCH1ysKlyWliMaFsYBx9Oy9d+oF3Jz0NomqRj8Snk HEvA== X-Gm-Message-State: AOAM533+if+oz7A8aHZVlMQdEyrKXil2OOUofVne2Wh+liBsPLVDuhUT xyKkJV0fl6qJ5kvSOfILjfs= X-Google-Smtp-Source: ABdhPJwPlS6ZPqw+MisTBKarDSPQd+plS4TfpstHjZVwno3BIEi3z/ZaweENtfBX2IsioSakFZ/DaQ== X-Received: by 2002:a17:90a:398d:: with SMTP id z13mr12209076pjb.1.1608451601658; Sun, 20 Dec 2020 00:06:41 -0800 (PST) Received: from [10.0.1.10] (c-24-4-128-201.hsd1.ca.comcast.net. [24.4.128.201]) by smtp.gmail.com with ESMTPSA id s29sm13830137pgn.65.2020.12.20.00.06.39 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 20 Dec 2020 00:06:40 -0800 (PST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Subject: Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect From: Nadav Amit In-Reply-To: Date: Sun, 20 Dec 2020 00:06:38 -0800 Cc: Andrea Arcangeli , linux-mm , Peter Xu , lkml , Pavel Emelyanov , Mike Kravetz , Mike Rapoport , stable@vger.kernel.org, minchan@kernel.org, Andy Lutomirski , Will Deacon , Peter Zijlstra Content-Transfer-Encoding: quoted-printable Message-Id: References: <20201219043006.2206347-1-namit@vmware.com> To: Yu Zhao X-Mailer: Apple Mail (2.3608.120.23.2.4) 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 Dec 19, 2020, at 10:05 PM, Yu Zhao wrote: >=20 > On Sat, Dec 19, 2020 at 01:34:29PM -0800, Nadav Amit wrote: >> [ cc=E2=80=99ing some more people who have experience with similar = problems ] >>=20 >>> On Dec 19, 2020, at 11:15 AM, Andrea Arcangeli = wrote: >>>=20 >>> Hello, >>>=20 >>> On Fri, Dec 18, 2020 at 08:30:06PM -0800, Nadav Amit wrote: >>>> Analyzing this problem indicates that there is a real bug since >>>> mmap_lock is only taken for read in mwriteprotect_range(). This = might >>>=20 >>> Never having to take the mmap_sem for writing, and in turn never >>> blocking, in order to modify the pagetables is quite an important >>> feature in uffd that justifies uffd instead of mprotect. It's not = the >>> most important reason to use uffd, but it'd be nice if that = guarantee >>> would remain also for the UFFDIO_WRITEPROTECT API, not only for the >>> other pgtable manipulations. >>>=20 >>>> Consider the following scenario with 3 CPUs (cpu2 is not shown): >>>>=20 >>>> cpu0 cpu1 >>>> ---- ---- >>>> userfaultfd_writeprotect() >>>> [ write-protecting ] >>>> mwriteprotect_range() >>>> mmap_read_lock() >>>> change_protection() >>>> change_protection_range() >>>> ... >>>> change_pte_range() >>>> [ defer TLB flushes] >>>> userfaultfd_writeprotect() >>>> mmap_read_lock() >>>> change_protection() >>>> [ write-unprotect ] >>>> ... >>>> [ unprotect PTE logically ] >>>> ... >>>> [ page-fault] >>>> ... >>>> wp_page_copy() >>>> [ set new writable page in PTE] >=20 > I don't see any problem in this example -- wp_page_copy() calls > ptep_clear_flush_notify(), which should take care of the stale entry > left by cpu0. >=20 > That being said, I suspect the memory corruption you observed is > related this example, with cpu1 running something else that flushes > conditionally depending on pte_write(). >=20 > Do you know which type of pages were corrupted? file, anon, etc. First, Yu, you are correct. My analysis is incorrect, but let me have another try (below). To answer your (and Andrea=E2=80=99s) question - = this happens with upstream without any changes, excluding a small fix to the = selftest, since it failed (got stuck) due to missing wake events. [1] We are talking about anon memory. So to correct myself, I think that what I really encountered was = actually during MM_CP_UFFD_WP_RESOLVE (i.e., when the protection is removed). The problem was that in this case the =E2=80=9Cwrite=E2=80=9D-bit was = removed during unprotect. Sorry for the strange formatting to fit within 80 columns: [ Start: PTE is writable ] cpu0 cpu1 cpu2 ---- ---- ---- [ Writable PTE=20= cached in TLB = ] userfaultfd_writeprotect() =09 [ write-*unprotect* ] mwriteprotect_range() mmap_read_lock() change_protection() change_protection_range() ... change_pte_range() [ *clear* =E2=80=9Cwrite=E2=80=9D-bit ] [ defer TLB flushes] [ page-fault ] =E2=80=A6 wp_page_copy() cow_user_page() [ copy page ] [ write to old page ] =E2=80=A6 set_pte_at_notify() [ End: cpu2 write not copied form old to new page. ] So this was actually resolved by the second part of the patch - changing preserve_write in change_pte_range(). I removed the acquisition of = mmap_lock for write, left the change in change_pte_range() and the test passes. Let me give some more thought on whether a mmap_lock is needed=20 for write. I need to rehash this TLB flushing algorithm. Thanks, Nadav [1] https://lore.kernel.org/patchwork/patch/1346386=