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.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL 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 6FC79C433E0 for ; Wed, 23 Dec 2020 03:36:12 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E4A98223E8 for ; Wed, 23 Dec 2020 03:36:11 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E4A98223E8 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 2EA1E6B008C; Tue, 22 Dec 2020 22:36:11 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 29BF38D0014; Tue, 22 Dec 2020 22:36:11 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 18A6B6B0098; Tue, 22 Dec 2020 22:36:11 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0076.hostedemail.com [216.40.44.76]) by kanga.kvack.org (Postfix) with ESMTP id 010906B008C for ; Tue, 22 Dec 2020 22:36:10 -0500 (EST) Received: from smtpin22.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id BC65F1EF3 for ; Wed, 23 Dec 2020 03:36:10 +0000 (UTC) X-FDA: 77623133700.22.note38_50103be27465 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin22.hostedemail.com (Postfix) with ESMTP id 9B02418038E60 for ; Wed, 23 Dec 2020 03:36:10 +0000 (UTC) X-HE-Tag: note38_50103be27465 X-Filterd-Recvd-Size: 7536 Received: from mail-il1-f169.google.com (mail-il1-f169.google.com [209.85.166.169]) by imf30.hostedemail.com (Postfix) with ESMTP for ; Wed, 23 Dec 2020 03:36:10 +0000 (UTC) Received: by mail-il1-f169.google.com with SMTP id g1so13865847ilk.7 for ; Tue, 22 Dec 2020 19:36:10 -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=ZIOrZp4VvbHbVHD6MydQhHnSb1pJwOMWKWbLXpdZOFI=; b=Vhrl2m3iMIa3mMtWmRqOmKoxmGP2khypurHxy69cJjigQyZicp85E1PyetI5yR1mGU QmUfzICRUzgBG35f210h8TMBpFciYgz+OzdbHRXJsDFlHg/i2bTTH7RalU4NzLIjd4Qf NnbDl1P02SGx/vxNyx72IrFpoKUlJLnXxXoIIuuDqUidX4Uu/zQSG7C2OrEVYjVXIqpn g2IWiiCHQQ9KJFzkdtCJOiZhVvSVzcgU+s/d+fLabWmPGTIzBB9VMJyHJyYGFRrb9l+o TIZ+0ggvpr4uSrtVQ6ikNCv1K6n+8koWhp7eU5/Jyd3iGMLG6igiwIlJgE6DVNhAR2QW y3SQ== 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=ZIOrZp4VvbHbVHD6MydQhHnSb1pJwOMWKWbLXpdZOFI=; b=O0yAc/YtMCrGU7r4yxsG99kA1fyzpoMmU/7vFprhbsfimE0u/AfpGPxkxH7+U43YTl ylJ90EByByIV6HG1cDxJAchKgR5IvdeQhGlGGcVjvoafyHnWkVzHpQJRp7HItZPvYcE4 UybAb53TORMnZWnosnVg0KQPd2LEM+K+H2HMHQIZj7FbyMs+ykNb8xT/iVmCyIKHcCtF UJmarZq9/4CRAh5qcMU7Oq/K30sY4Zv1p/nzqCSnLaao+MWePlW195DuJSSYJLYVPUVl P7wKDHQZ2rope7NCeBqnVNt0N73xKB8SnwLbShp46h5dZGogXzoSU+JFBFHAyzjSG0bJ 0dRw== X-Gm-Message-State: AOAM530PtAOB8h3Zpz+HdqMuy6rPCQNCdRylSeNlKpc6bD39AREIQwmo dMZzGMJmW+NxISGtvnrxNEcWGw== X-Google-Smtp-Source: ABdhPJwmFsNaiUtYb57RZoZls9iOlEmeZLPp8r+adegWDnYyoLjPiAj7KYKM5riNKEWykKA2igcNCQ== X-Received: by 2002:a92:1e10:: with SMTP id e16mr15481208ile.65.1608694569458; Tue, 22 Dec 2020 19:36:09 -0800 (PST) Received: from google.com ([2620:15c:183:200:7220:84ff:fe09:2d90]) by smtp.gmail.com with ESMTPSA id y12sm17546969ilk.32.2020.12.22.19.36.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 22 Dec 2020 19:36:08 -0800 (PST) Date: Tue, 22 Dec 2020 20:36:04 -0700 From: Yu Zhao To: Andrea Arcangeli Cc: Andy Lutomirski , Linus Torvalds , Peter Xu , Nadav Amit , linux-mm , lkml , Pavel Emelyanov , Mike Kravetz , Mike Rapoport , stable , Minchan Kim , Will Deacon , Peter Zijlstra Subject: Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect Message-ID: References: <1FCC8F93-FF29-44D3-A73A-DF943D056680@gmail.com> <20201221223041.GL6640@xz-x1> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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, Dec 22, 2020 at 09:56:11PM -0500, Andrea Arcangeli wrote: > On Tue, Dec 22, 2020 at 04:39:46PM -0700, Yu Zhao wrote: > > We are talking about non-COW anon pages here -- they can't be mapped > > more than once. So why not just identify them by checking > > page_mapcount == 1 and then unconditionally reuse them? (This is > > probably where I've missed things.) > > The problem in depending on page_mapcount to decide if it's COW or > non-COW (respectively wp_page_copy or wp_page_reuse) is that is GUP > may elevate the count of a COW anon page that become a non-COW anon > page. > > This is Jann's idea not mine. > > The problem is we have an unprivileged long term GUP like vmsplice > that facilitates elevating the page count indefinitely, until the > parent finally writes a secret to it. Theoretically a short term pin > would do it too so it's not just vmpslice, but the short term pin > would be incredibly more challenging to become a concern since it'd > kill a phone battery and flash before it can read any data. > > So what happens with your page_mapcount == 1 check is that it doesn't > mean non-COW (we thought it did until it didn't for the long term gup > pin in vmsplice). > > Jann's testcases does fork() and set page_mapcount 2 and page_count to > 2, vmsplice, take unprivileged infinitely long GUP pin to set > page_count to 3, queue the page in the pipe with page_count elevated, > munmap to drop page_count to 2 and page_mapcount to 1. > > page_mapcount is 1, so you'd think the page is non-COW and owned by > the parent, but the child can still read it so it's very much still > wp_page_copy material if the parent tries to modify it. Otherwise the > child can read the content. > > This was supposed to be solvable by just doing the COW in gup(write=0) > case if page_mapcount > 1 with commit 17839856fd58. I'm not exactly > sure why that didn't fly and it had to be reverted by Peter in > a308c71bf1e6e19cc2e4ced31853ee0fc7cb439a but at the time this was > happening I was side tracked by urgent issues and I didn't manage to > look back of how we ended up with the big hammer page_count == 1 check > instead to decide if to call wp_page_reuse or wp_page_shared. > > So anyway, the only thing that is clear to me is that keeping the > child from reading the page_mapcount == 1 pages of the parent, is the > only reason why wp_page_reuse(vmf) will only be called on > page_count(page) == 1 and not on page_mapcount(page) == 1. > > It's also the reason why your page_mapcount assumption will risk to > reintroduce the issue, and I only wish we could put back page_mapcount > == 1 back there. > > Still even if we put back page_mapcount there, it is not ok to leave > the page fault with stale TLB entries and to rely on the fact > wp_page_shared won't run. It'd also avoid the problem but I think if > you leave stale TLB entries in change_protection just like NUMA > balancing does, it also requires a catcher just like NUMA balancing > has, or it'd truly work by luck. > > So until we can put a page_mapcount == 1 check back there, the > page_count will be by definition unreliable because of the speculative > lookups randomly elevating all non zero page_counts at any time in the > background on all pages, so you will never be able to tell if a page > is true COW or if it's just a spurious COW because of a speculative > lookup. It is impossible to differentiate a speculative lookup from a > vmsplice ref in a child. Thanks for the details. In your patch, do we need to take wrprotect_rwsem in handle_userfault() as well? Otherwise, it seems userspace would have to synchronize between its wrprotect ioctl and fault handler? i.e., the fault hander needs to be aware that the content of write- protected pages can actually change before the iotcl returns.