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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id CACB5C4332F for ; Fri, 16 Dec 2022 14:54:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CE1F98E0003; Fri, 16 Dec 2022 09:54:38 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C910C8E0002; Fri, 16 Dec 2022 09:54:38 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B313E8E0003; Fri, 16 Dec 2022 09:54:38 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id A43778E0002 for ; Fri, 16 Dec 2022 09:54:38 -0500 (EST) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 6E8034104B for ; Fri, 16 Dec 2022 14:54:38 +0000 (UTC) X-FDA: 80248465836.15.044E54F Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf01.hostedemail.com (Postfix) with ESMTP id 8A1714001A for ; Fri, 16 Dec 2022 14:54:35 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=S6d4ADa7; spf=pass (imf01.hostedemail.com: domain of peterx@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=peterx@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1671202475; a=rsa-sha256; cv=none; b=nOZyVUOzLU+I78mVsOMQGnzcsuQrSJiReHJVAJjPMQ1O79wS6isN416Su1NK1Uyr+wZNZP N9xpea2ykswSkKH62Hbz8YjkMhfsDt9cpU7YBwG1Cxldt0+M6vG6owVVSWaenBrOLABXUN IrIDFxguCWpmU5WH/4SVUPa8/pqzzNA= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=S6d4ADa7; spf=pass (imf01.hostedemail.com: domain of peterx@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=peterx@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1671202475; h=from:from:sender: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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=eyyx/aF2V9lTUZ0rSVhQrdjXcZ3OfNikbAhK+3xqsq4=; b=lpfDwP9G/7pNe4VO9j5QtVc6k8nHAaa/+VmE7H3YdHguyb54/BCaQpOgeX6ykQ600ZBVOF 6PnICyv+fCzpklmXu6qnW8zlcshBk6FJVGCNg1nq9T5LUmI/DZF9bFhFftXM86eRx4S9U9 G7qSDvo8OLy2UDXvYLaoaZUD3e8zP9s= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1671202474; 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: in-reply-to:in-reply-to:references:references; bh=eyyx/aF2V9lTUZ0rSVhQrdjXcZ3OfNikbAhK+3xqsq4=; b=S6d4ADa7zXY8lzIQ+gqwqob4fDyFBPBrWQZsrdiZAniv4zBDwdwAPvtxrbyqKlDJmH035o UN0RRGzsMZ1bBnP3+7Pg8ojYPMbA7z/VF22a4FzN4ToF8HW+GrYqrZLyR7UB87FL6nVJ9s hjEwU3CksANj26aPX0Wtl0rT1ghmAEg= Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-617-2YOg68KpOiGssrgDlocPoQ-1; Fri, 16 Dec 2022 09:54:33 -0500 X-MC-Unique: 2YOg68KpOiGssrgDlocPoQ-1 Received: by mail-qk1-f198.google.com with SMTP id bi42-20020a05620a31aa00b006faaa1664b9so1956875qkb.8 for ; Fri, 16 Dec 2022 06:54:33 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=eyyx/aF2V9lTUZ0rSVhQrdjXcZ3OfNikbAhK+3xqsq4=; b=EntX6X/NT2h+QORRdU+7Q36vHE+brCUgaEmQlWbzgR3Ua1xswACk+gpFkCufRdCn7s ij7FjKpBJ0U8J9cP4/eLrM32FRSVhYoZa4STeWXDMLFksc8e/31SfmGIHxLH30AHWA+0 yv6RkR3lWyOm2Vp2bznkSniZQ9+umuM9IroNizl3eoRI24+tJ6pl57UioFuAS1SWL+Ut RqJiuXOn/j36l0WGhxv0awQLoX8+ThLC3364m/ZCMZHVlii0R5760oK5vd9L4yhU9/yZ Bv0g5Igy0tBgYOWfmmHk0SoKwomgO01VXTK+AfD1VAHFyPc8ZCXoPEsM5nTA6qpE6mhj 5epA== X-Gm-Message-State: ANoB5pmF924ygrMfv0dAEdZQvbSrDzxaXUwAj8Ev3zPPOuxuWXyjg6VK pxKiJm4HqkANKcuc/9An1UtvMUnFJTWdPnScSmHIOufWwv1DoehAo5jWHkAHISqMPwwT6OoysGB Vg1W5KyX63+0= X-Received: by 2002:ad4:43e9:0:b0:4bb:6eb9:a220 with SMTP id f9-20020ad443e9000000b004bb6eb9a220mr38021837qvu.8.1671202473243; Fri, 16 Dec 2022 06:54:33 -0800 (PST) X-Google-Smtp-Source: AA0mqf5AHUM11A/bB73hu6cvluJb5tmi8bh492KBCIr5bVruuG1aH/QXeVJO6LChik/tvjIwG/6Bjw== X-Received: by 2002:ad4:43e9:0:b0:4bb:6eb9:a220 with SMTP id f9-20020ad443e9000000b004bb6eb9a220mr38021814qvu.8.1671202472943; Fri, 16 Dec 2022 06:54:32 -0800 (PST) Received: from x1n (bras-base-aurron9127w-grc-45-70-31-26-132.dsl.bell.ca. [70.31.26.132]) by smtp.gmail.com with ESMTPSA id u7-20020a05620a430700b006fa31bf2f3dsm1620323qko.47.2022.12.16.06.54.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 16 Dec 2022 06:54:32 -0800 (PST) Date: Fri, 16 Dec 2022 09:54:31 -0500 From: Peter Xu To: David Hildenbrand Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Andrea Arcangeli , Pengfei Xu , Nadav Amit , Andrew Morton , Miaohe Lin , Huang Ying , stable@vger.kernel.org Subject: Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event Message-ID: References: <20221214200453.1772655-1-peterx@redhat.com> <20221214200453.1772655-2-peterx@redhat.com> <618b69be-0e99-e35f-04b3-9c63d78ece50@redhat.com> MIME-Version: 1.0 In-Reply-To: <618b69be-0e99-e35f-04b3-9c63d78ece50@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-Rspam-User: X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 8A1714001A X-Stat-Signature: 71446p6g68emogxqyfmeecxaqx5gz8ki X-HE-Tag: 1671202475-515286 X-HE-Meta: U2FsdGVkX1/Be8JQ9WhmjTj0GY039dT0FSVRu/8LdqCOo5Gr0BhA9lVe5hgdkDLeRQe7uAf8JIXiNhmXadKL5Up4+wMLI2VM+cB37OSvpy8++mgr1D6BnW2gtjyp6sxi3u2jBHy71Kwrao0j3b0JfEQoECIE/OyKsAMzz4nC/YqKT3lgXjSJ+e1rMiLO5MoXpRfureCC4h3dSIYy0ovQISkkYWlwk6+fUC326hcGvjs/GNI2d5bo9p8oQNh6Q6YhwEUvSih9R7nT8f59WoHZ/UHTItU6lps7ymhztLD8ra8lYGDyjjq+1l7hAuTH0OJSSyHV/WdppANzGz6gVnd2ZostxL6IsCGdxcgjIfxuyRPZvfc5wDepvE3sVqPAS1l0SM+rmIH6n1ThEowXfECkc1VdpCr1qY2CNhuj5A/5aA86PlAfEx5DpcGo0AujPPQXfl7bEc9fc5+QFiFLeVnUtHR1qCZTErCNUMjV+l32mt5hkmAo4MJ7ViBqzGTqKz4MPbzM+9H7A19WZZLZfyYxYyHFDKtgTyZz++zv4M4LA/BT3oBXTSXm0qWS1qZzYOTutpA/iXoamzBnZfH7kzyKYDbbYm3WbLpQDJvhrn+KTKJuOrE8DZq6rii7bhRDXppzi+seFuHAEA2z02aFMct3RYvzqdrrGlLsmWU32VYRWoofUVJmRIbdpUy3+Eqfk1WAFAGzVGSY16SICPcPzgwbGaNM6Al0ud7+GA1UvwgeL8uYJTIpfYI61h3xkJ5sUHMs2cIj5z2Wxe+EpXb5uDxQTrdU54Ze/UYM6Y0u27erBTSPP0GYaU6+cvK53EjAfKLSjRDCAbdcMAd6YvBLgdcPIbdIwjNtGPKgOqoLfImgV9n7hP2t1FQXX0UIcwm67T6GEWH8wfyfECmBfM6dQDej0lxKgj/Ud2vp+nf4B89anqCrE0YqF691jMSM9NymxhIa+BXun2Vnr8lPVGT7WTS 5coRWMxN ySAmn99RULUDfAnvdmGomift3oN/yrQFZIXsi9KUYxPx51yQhIMIdUfLWZv5dnW+fVmB2KP0oWJ0JQWOTzqxLWo/3fVhHwnSenYN+nxwkdfh3O319GbtUcIHe69YF7v+3bF5gEhTvcNncQVWtuvD9fiiqXaN2avRKvfBBi/gwmMX3tt/k27tYGSNAdA== 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 Fri, Dec 16, 2022 at 10:04:27AM +0100, David Hildenbrand wrote: > On 14.12.22 21:04, Peter Xu wrote: > > When fork(), dst_vma is not guaranteed to have VM_UFFD_WP even if src may > > have it and has pte marker installed. The warning is improper along with > > the comment. The right thing is to inherit the pte marker when needed, or > > keep the dst pte empty. > > > > A vague guess is this happened by an accident when there's the prior patch > > to introduce src/dst vma into this helper during the uffd-wp feature got > > developed and I probably messed up in the rebase, since if we replace > > dst_vma with src_vma the warning & comment it all makes sense too. > > > > Hugetlb did exactly the right here (copy_hugetlb_page_range()). Fix the > > general path. > > > > Reproducer: > > > > https://github.com/xupengfe/syzkaller_logs/blob/main/221208_115556_copy_page_range/repro.c > > > > Cc: # 5.19+ > > Fixes: c56d1b62cce8 ("mm/shmem: handle uffd-wp during fork()") > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216808 > > Reported-by: Pengfei Xu > > Signed-off-by: Peter Xu > > --- > > mm/memory.c | 8 ++------ > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/mm/memory.c b/mm/memory.c > > index aad226daf41b..032ef700c3e8 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -828,12 +828,8 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, > > return -EBUSY; > > return -ENOENT; > > } else if (is_pte_marker_entry(entry)) { > > - /* > > - * We're copying the pgtable should only because dst_vma has > > - * uffd-wp enabled, do sanity check. > > - */ > > - WARN_ON_ONCE(!userfaultfd_wp(dst_vma)); > > - set_pte_at(dst_mm, addr, dst_pte, pte); > > + if (userfaultfd_wp(dst_vma)) > > + set_pte_at(dst_mm, addr, dst_pte, pte); > > return 0; > > } > > if (!userfaultfd_wp(dst_vma)) > > Staring at the code first made me go "what about other PTE markers". I then > looked into the discussion in patch #2. The fix as is is suboptimal, because > it > > 1) Removes the warning which is good, but > 2) Silently drops swapin errors now > > So it silently breaks something else temporarily ... > > > I remember, that theoretically we could have multiple markers stored in a > single PTE marker. > > Wouldn't it be cleaner to be able to "clean" specific markers from a PTE > marker without having to special case on each and everyone? I mean, only > uffd-wp is really special such that it might disappear for the target. Quotting the commit message in patch 2: Currently there is a priority difference between the uffd-wp bit and the swapin error entry, in which the swapin error always has higher priority (e.g. we don't need to wr-protect a swapin error pte marker). If there will be a 3rd bit introduced, we'll probably need to consider a more involved approach so we may need to start operate on the bits. Let's leave that for later. I actually started the fix with something like that, but I noticed it's not needed to add more code if there's no 3rd bit introduced so I dropped that. I decided to go the simpler change approach and leave that for later. > > Something like (pseudocode): > > if (!userfaultfd_wp(dst_vma)) > pte_marker_clear_uff_wp(entry); > if (!pte_marker_empty(entry)) { > pte = make_pte_marker(pte_marker_get(entry)); > set_pte_at(dst_mm, addr, dst_pte, pte); > } > > Then this fix would be correct and backport-able even without #2. And it > would work for new types of markers :) When that comes, we may need one set_pte_marker_at() taking care of empty pte markers, otherwise there can be a lot of such check. > > > I'd prefer a fix that doesn't break something else temporarily, even if the > stable backport might require 5 additional minutes to do. So squashing #2 > into #1 would also work. The thing is whether do we care about someone: (1) explicitly checkout at the commit of patch 1, then (2) runs the kernel, hit a swapnin error, (3) fork(), and (4) access the swapin error page in the child. To me I don't care even starting from (1).. because it really shouldn't happen at all in any serious environment. The other reason is these are indeed two issues to solve. Even if by accident we kept the swapin error in old code we'll probably dump an warning which is not wanted either. It's not something someone will really get benefit from.. So like many other places, I don't have a strong opinion, but personally I prefer the current approach. Andrew, let me know if you want me to repost with a squashed version. Thanks, -- Peter Xu