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=-16.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham 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 DB167C433DB for ; Sat, 27 Mar 2021 21:57:13 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 5706561992 for ; Sat, 27 Mar 2021 21:57:13 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5706561992 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 7E1956B0078; Sat, 27 Mar 2021 17:57:12 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 791746B007B; Sat, 27 Mar 2021 17:57:12 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 60A9B6B007D; Sat, 27 Mar 2021 17:57:12 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0211.hostedemail.com [216.40.44.211]) by kanga.kvack.org (Postfix) with ESMTP id 48C746B0078 for ; Sat, 27 Mar 2021 17:57:12 -0400 (EDT) Received: from smtpin05.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id F24A0B7A7 for ; Sat, 27 Mar 2021 21:57:11 +0000 (UTC) X-FDA: 77967015462.05.0987C7E Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by imf22.hostedemail.com (Postfix) with ESMTP id 96FA3C0007C6 for ; Sat, 27 Mar 2021 21:57:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1616882229; 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=Bj/1bcBSUwlXPODMgK+7TR54EtYdkqxCKK5SCH5Eb+k=; b=XLvTBm0Nf2uQsVEG/xRWsz9KrubmRdjjXaGHwFrYRa8CiFsm5G9oo0vhRAJjdvWKvZv0z4 0pIOUlz3q3XTk9ZYSvutWr6WYrIYy72kTZ8O6r0qGZhAkrLNF8ux4WJKUtprPHd02IpR8k vvqZfK8Zi3e2RlMgFcmkjS9gZawRUx0= Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-293-asNdm4RzM0SHsBv0ayIXzw-1; Sat, 27 Mar 2021 17:57:08 -0400 X-MC-Unique: asNdm4RzM0SHsBv0ayIXzw-1 Received: by mail-qk1-f197.google.com with SMTP id g18so9070456qki.15 for ; Sat, 27 Mar 2021 14:57:08 -0700 (PDT) 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=Bj/1bcBSUwlXPODMgK+7TR54EtYdkqxCKK5SCH5Eb+k=; b=aNzrdTmv2k/qFpumvU3M9JuEnjUFjAximwSwLZCz09Cbg4SagT3U/fMmWkdGFu0dxV h1vdnpTJS3kSeXYIkaDkq+S/pvRlUfTbXzKeKXudxP0Bk2lk+ZDBzpUOhPWdQQ2ZOdPa P0tUaAcD+ndXDqYshMf6WINkldYG/hZ1DpbX+fKvb+KegyRgILJVYO6ocp0txQTwAHCO RXZTpJxXCU+EUAyvSayAlPUcC/mXcIBvMBfnLA48DXUtt38AnuP03tSTgRUxnsVKWXey Erd6uOcC66bGNztzucCAdxto7W0C9TUFjDAN2ezg29cGwQt+UoDBdMDbHT6DPHcFTHn+ w9tg== X-Gm-Message-State: AOAM531qPNKL6KqASwnhsApL1WQvR6ANNXWAH5bU3RrdR8Qq62l2TdUn NnkUc9D9oLHaKpYPMB5Yh3Bvi85upL47gDds4tQm8vUa3E5r7XU1SZ+Zs2dfKONTi97cXQdQGm+ JtP3S1+yi6eU= X-Received: by 2002:a05:620a:31a:: with SMTP id s26mr19301672qkm.355.1616882227417; Sat, 27 Mar 2021 14:57:07 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzCJKaSGSlzQNoXwiiYL1bXbPYCri/xP97a1UnyExgW9n9/TCZ5ehH0OYORnNJ3nuDDuOgj8A== X-Received: by 2002:a05:620a:31a:: with SMTP id s26mr19301653qkm.355.1616882227094; Sat, 27 Mar 2021 14:57:07 -0700 (PDT) Received: from xz-x1 (bras-base-toroon474qw-grc-82-174-91-135-175.dsl.bell.ca. [174.91.135.175]) by smtp.gmail.com with ESMTPSA id t24sm8185897qto.23.2021.03.27.14.57.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 27 Mar 2021 14:57:06 -0700 (PDT) Date: Sat, 27 Mar 2021 17:57:03 -0400 From: Peter Xu To: Axel Rasmussen Cc: Alexander Viro , Andrea Arcangeli , Andrew Morton , Hugh Dickins , Jerome Glisse , Joe Perches , Lokesh Gidra , Mike Rapoport , Shaohua Li , Shuah Khan , Stephen Rothwell , Wang Qing , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org, Brian Geffon , Cannon Matthews , "Dr . David Alan Gilbert" , David Rientjes , Michel Lespinasse , Mina Almasry , Oliver Upton Subject: Re: [PATCH] userfaultfd/shmem: fix MCOPY_ATOMIC_CONTNUE error handling + accounting Message-ID: <20210327215703.GA429942@xz-x1> References: <20210325231027.3402321-1-axelrasmussen@google.com> MIME-Version: 1.0 In-Reply-To: <20210325231027.3402321-1-axelrasmussen@google.com> Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=peterx@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-Stat-Signature: nt8cbx7fg3i67hg1r49hqsmj1x6wtzic X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 96FA3C0007C6 Received-SPF: none (redhat.com>: No applicable sender policy available) receiver=imf22; identity=mailfrom; envelope-from=""; helo=us-smtp-delivery-124.mimecast.com; client-ip=216.205.24.124 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1616882228-238264 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: Axel, On Thu, Mar 25, 2021 at 04:10:27PM -0700, Axel Rasmussen wrote: > Previously, in the error path, we unconditionally removed the page from > the page cache. But in the continue case, we didn't add it - it was > already there because the page is used by a second (non-UFFD-registered) > mapping. So, in that case, it's incorrect to remove it as the other > mapping may still use it normally. > > For this error handling failure, trivially exercise it in the > userfaultfd selftest, to detect this kind of bug in the future. > > Also, we previously were unconditionally calling shmem_inode_acct_block. > In the continue case, however, this is incorrect, because we would have > already accounted for the RAM usage when the page was originally > allocated (since at this point it's already in the page cache). So, > doing it in the continue case causes us to double-count. > > Fixes: 00da60b9d0a0 ("userfaultfd: support minor fault handling for shmem") > Signed-off-by: Axel Rasmussen > --- > mm/shmem.c | 15 ++++++++++----- > tools/testing/selftests/vm/userfaultfd.c | 12 ++++++++++++ > 2 files changed, 22 insertions(+), 5 deletions(-) > > diff --git a/mm/shmem.c b/mm/shmem.c > index d2e0e81b7d2e..5ac8ea737004 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -2379,9 +2379,11 @@ int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd, > int ret; > pgoff_t offset, max_off; > > - ret = -ENOMEM; > - if (!shmem_inode_acct_block(inode, 1)) IMHO a better change here is to only touch this line into: if (!is_continue && !shmem_inode_acct_block(inode, 1)) Then you don't need to touch any other line, also you can drop line [1] below too as a side benefit. > - goto out; > + if (!is_continue) { > + ret = -ENOMEM; > + if (!shmem_inode_acct_block(inode, 1)) > + goto out; > + } > > if (is_continue) { > ret = -EFAULT; > @@ -2389,6 +2391,7 @@ int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd, > if (!page) > goto out_unacct_blocks; > } else if (!*pagep) { > + ret = -ENOMEM; [1] > page = shmem_alloc_page(gfp, info, pgoff); > if (!page) > goto out_unacct_blocks; > @@ -2486,12 +2489,14 @@ int shmem_mcopy_atomic_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd, > out_release_unlock: > pte_unmap_unlock(dst_pte, ptl); > ClearPageDirty(page); Should this be conditional too? IIUC this will clear dirty for the page cache even if it was dirty. I'm not sure whether it'll cause data loss. > - delete_from_page_cache(page); > + if (!is_continue) > + delete_from_page_cache(page); > out_release: > unlock_page(page); > put_page(page); > out_unacct_blocks: > - shmem_inode_unacct_blocks(inode, 1); > + if (!is_continue) > + shmem_inode_unacct_blocks(inode, 1); > goto out; > } Besides the error handling, I looked at the function again and I have another two thoughts: 1. IMHO in shmem_mcopy_atomic_pte() we should also conditionally call pte_mkwrite() just like the hugetlb code too, so as to keep W bit clear when !VM_SHARED. 2. I see even more "if (!is_continue)" here.. I'm thinking whether we can simply jump to pte installation "if (is_continue)" instead, because uffdio-continue shoiuld really be a lightweight operation. E.g., most of the things at the middle of the function is not relevant to uffd-continue. To be explicit: ret = -EFAULT; offset = linear_page_index(dst_vma, dst_addr); max_off = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); if (unlikely(offset >= max_off)) goto out_release; These chunk can be put into !is_continue too, then if you see you're bypassing mostly everything. Then error handling of uffdio-continue would be simple too, since it could only fail if either page cache not exist, or pte changed. Nothing else could happen. For above point (1), I even start to doubt whether we should conditionally grant the write bit for normal uffdio_copy case only if both WRITE|SHARED set for the vma flags. E.g., shmem_mcopy_atomic_pte() of a normal uffdio-copy will fill in the page cache into pte, however what if this mapping is privately mapped? IMHO we can't apply write bit otherwise the process will be writting to the page cache directly. However I think that question will be irrelevant to this patch. Thanks, -- Peter Xu