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 CB499C4828D for ; Wed, 7 Feb 2024 15:27:48 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5BDC06B0080; Wed, 7 Feb 2024 10:27:48 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 56DE16B0083; Wed, 7 Feb 2024 10:27:48 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 437FF6B0085; Wed, 7 Feb 2024 10:27:48 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 33AAD6B0080 for ; Wed, 7 Feb 2024 10:27:48 -0500 (EST) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 0EC6480DBA for ; Wed, 7 Feb 2024 15:27:48 +0000 (UTC) X-FDA: 81765387816.01.855AD9A Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf26.hostedemail.com (Postfix) with ESMTP id 4F28F140018 for ; Wed, 7 Feb 2024 15:27:46 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=PdJyBcfh; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf26.hostedemail.com: domain of rppt@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=rppt@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707319666; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=KX2UT2kbb1zJuhP+lk26UOQQj3IhG0JcfoCp36/CCY8=; b=fk3lGaTVu8mHyz2LRO5zIsx34FaQgNC0UtsLtEdg12Wp0T9+QXMGXwUWnBaZ3KA9tFgkCD pzHNddvOaX47IMoj3HvfaSdQCJiRpSrK91/UKhHVk7uLiHe+xquZoJGUJ0BnpvdsD74Tiq fV+yLE4Ge2c1eV0dA8DMTl0Fi4SWkCg= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=PdJyBcfh; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf26.hostedemail.com: domain of rppt@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=rppt@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707319666; a=rsa-sha256; cv=none; b=HMAOjdQ0dCUbtA27A169hI8ewPTLssTDlAP8YIZLJ5g3aahGptgkTnoafGv9295lWU1xWY E4qSGC/SDr9JCi78SZRGhPiMR/9ccLUNy3grubUqjD8/3nY5QfO16tLZk2YreBPPFcsGpb L/bMNhPzdcXcBDReZUpJHGym8VrLfuY= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 39464618E8; Wed, 7 Feb 2024 15:27:45 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1FDCBC433C7; Wed, 7 Feb 2024 15:27:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1707319664; bh=Wpz6YO32Qj0aa5MgpAxe/gu7xxnOiMKkMHU89Lo/BS8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=PdJyBcfhBDugPqfPAHL60XArKC2R50gNcdevNydbNpntpB4hLQISmk+WvQz7ExJDC BnjEi16qiL+uUkmniRIt1RsDgBSF5k0jpliADyYSSFkFX0W/Pl2dewzvm+c8fpAvbu JQx7pQ+aXo7bsJIwGvplzWHtlbtl5mdKbGTUngISuIeYG3C/1J9eHtQqvg24oUDoJa MUuXi5UfS/5Jn1xIHp9P1A16Va9pv3WErzdoSRR6QErvLz99M99eJedf6zKHULEtww 3FFY+q2LKz3vMWSnNkBQIws27BPo8ZjZejR4lUtsf7CwtDNhTLL1DvA6c/0DyKJi3X D8zf0Ses6KFCQ== Date: Wed, 7 Feb 2024 17:27:23 +0200 From: Mike Rapoport To: Lokesh Gidra Cc: "Liam R. Howlett" , akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, selinux@vger.kernel.org, surenb@google.com, kernel-team@android.com, aarcange@redhat.com, peterx@redhat.com, david@redhat.com, axelrasmussen@google.com, bgeffon@google.com, willy@infradead.org, jannh@google.com, kaleshsingh@google.com, ngeoffray@google.com, timmurray@google.com Subject: Re: [PATCH v2 2/3] userfaultfd: protect mmap_changing with rw_sem in userfaulfd_ctx Message-ID: References: <20240129193512.123145-1-lokeshgidra@google.com> <20240129193512.123145-3-lokeshgidra@google.com> <20240129210014.troxejbr3mzorcvx@revolver> <20240130034627.4aupq27mksswisqg@revolver> <20240130172831.hv5z7a7bhh4enoye@revolver> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 4F28F140018 X-Stat-Signature: ry7eawywjf4hg3tq15jg3ik4dc4rg8cq X-Rspam-User: X-HE-Tag: 1707319666-703043 X-HE-Meta: U2FsdGVkX1/yvOf4dBJDl9leUX9tv4FiMPdEELb2iZJMEy06cqc/H8Jc6Fg0pzpKzMR+NIVQS9lng/IpIblSVkCOd07Dm2IjhormFNkgvgDuKrOCgWzWOfENNL3a+3sh9Ni7oVi4yfKKRaheG11+owQVJ45Tk6H3MSuRKbPDy9SB8EZSrHdS9fff0j8IKN+LC2Ddgx1JO7qvpsDaJM2f6/BycFr3nfKyRdv7b6fB4uZzFBYcel/c61ax0e40KOr6GjId6iGlyrPfXepaZDFE2GWBwiWN0FuDAJtr23hAJJ5pJbXGPbGHU9cU+O1cZO0ryhTk+jvvMkHAuj+c8kBg7pfCmO+5fLTe4lTsyJERG/mUDfopUSSodGsrXXuPxV6OnsFG/QXDNRuAd4LWhIV0k4k5satcB93r0/tMyuBKOL09jJrfGtjPXd6mCuCYW/Nuq/RrjUyrcpMyaCElVFHsT4suRdtypoLoQ206xKnlHly0n3ELNy6azALQ2iA2ReWOvYSWVUS2CC192SDJ09koKcEGTVjsChtLZAD0bwFELw0br+RQK3FGZHK2EH87Lwn1spGTlGSFHTq6l3woKW1PKHTL6Fi+WP5P3Q1gce/UnNl4fHd1U0ieSAU62eUilAAI9JekpwqF+t5deEV07MjR3p+bTymYeVRSqdizSV6CQO9uWRcg/kQNmOeA2o4+oE30Tk1iSR+ZpbPyRitF99e1kwokg+XvXwsS9iVIwYP99s0JuEAMxKXqKUWJZt1uN3dqVx+ffocH52fnzjbtgEFUm5CMpUz5RhDYT9ax+fRY+XFLnCdSrm9Iq3dPlNWhGypACKci8mgHAnJVYhRfuSADt25CUahwAA/r2akSPRlvB2LiPEmCh0Fub+xFhPPBX5UaRSk2Rqk+y2YuNfKemt3AYcseV1+H2epB7dm0pwg2NeQLd5+Iw17+fz44puMEEjjCyHE+xCVCoL9bRFBU3sO isQPrRxR xlZFsOpWNQ6VmxbM3teNvccbhlAXWQr4Ae1SI6bBQyYR7BAB6nUoYqkLwOkVbkaeBJ49FNU6L3yBN54TlMf4SRg2W5HdzQA0z56rbDegnMfHnTCXpuN7+neMZK3K/WmjhVHS1ezhpEruXDNkMRPvEZ3Gy+2pi5ARf/Sre7rakYy4UnxVVoX1k3znC+pis970DRdhZm6DxrTS039d6T4OVINzBuuyEp5R0jHfKuFksXtbTkH+0OYWyBskzJNw41f9r5syYIQhBWPtFbm3w0e/0MyMNQZZYmsnfI0xPbcXYr5a1hpCQo8r9FHI37SdhYt/nQC/AlhY9IXTqjH8= 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: List-Subscribe: List-Unsubscribe: On Mon, Feb 05, 2024 at 12:53:33PM -0800, Lokesh Gidra wrote: > On Sun, Feb 4, 2024 at 2:27 AM Mike Rapoport wrote: > > > > > 3) Based on [1] I see how mmap_changing helps in eliminating duplicate > > > work (background copy) by uffd monitor, but didn't get if there is a > > > correctness aspect too that I'm missing? I concur with Amit's point in > > > [1] that getting -EEXIST when setting up the pte will avoid memory > > > corruption, no? > > > > In the fork case without mmap_changing the child process may be get data or > > zeroes depending on the race for mmap_lock between the fork and > > uffdio_copy and -EEXIST is not enough for monitor to detect what was the > > ordering between fork and uffdio_copy. > > This is extremely helpful. IIUC, there is a window after mmap_lock > (write-mode) is released and before the uffd monitor thread is > notified of fork. In that window, the monitor doesn't know that fork > has already happened. So, without mmap_changing it would have done > background copy only in the parent, thereby causing data inconsistency > between parent and child processes. Yes. > It seems to me that the correctness argument for mmap_changing is > there in case of FORK event and REMAP when mremap is called with > MREMAP_DONTUNMAP. In all other cases its only benefit is by avoiding > unnecessary background copies, right? Yes, I think you are right, but it's possible I've forgot some nasty race that will need mmap_changing for other events. > > > > > > > > > @@ -783,7 +788,9 @@ bool userfaultfd_remove(struct vm_area_struct *vma, > > > > > > > > > return true; > > > > > > > > > > > > > > > > > > userfaultfd_ctx_get(ctx); > > > > > > > > > + down_write(&ctx->map_changing_lock); > > > > > > > > > atomic_inc(&ctx->mmap_changing); > > > > > > > > > + up_write(&ctx->map_changing_lock); > > > > > > > > > mmap_read_unlock(mm); > > > > > > > > > > > > > > > > > > msg_init(&ewq.msg); > > > > > > > > > > > > If this happens in read mode, then why are you waiting for the readers > > > > > > to leave? Can't you just increment the atomic? It's fine happening in > > > > > > read mode today, so it should be fine with this new rwsem. > > > > > > > > > > It's been a while and the details are blurred now, but if I remember > > > > > correctly, having this in read mode forced non-cooperative uffd monitor to > > > > > be single threaded. If a monitor runs, say uffdio_copy, and in parallel a > > > > > thread in the monitored process does MADV_DONTNEED, the latter will wait > > > > > for userfaultfd_remove notification to be processed in the monitor and drop > > > > > the VMA contents only afterwards. If a non-cooperative monitor would > > > > > process notification in parallel with uffdio ops, MADV_DONTNEED could > > > > > continue and race with uffdio_copy, so read mode wouldn't be enough. > > > > > > > > > > > > > Right now this function won't stop to wait for readers to exit the > > > > critical section, but with this change there will be a pause (since the > > > > down_write() will need to wait for the readers with the read lock). So > > > > this is adding a delay in this call path that isn't necessary (?) nor > > > > existed before. If you have non-cooperative uffd monitors, then you > > > > will have to wait for them to finish to mark the uffd as being removed, > > > > where as before it was a fire & forget, this is now a wait to tell. > > > > > > > I think a lot will be clearer once we get a response to my questions > > > above. IMHO not only this write-lock is needed here, we need to fix > > > userfaultfd_remove() by splitting it into userfaultfd_remove_prep() > > > and userfaultfd_remove_complete() (like all other non-cooperative > > > operations) as well. This patch enables us to do that as we remove > > > mmap_changing's dependency on mmap_lock for synchronization. > > > > The write-lock is not a requirement here for correctness and I don't see > > why we would need userfaultfd_remove_prep(). > > > > As I've said earlier, having a write-lock here will let CRIU to run > > background copy in parallel with processing of uffd events, but I don't > > feel strongly about doing it. > > > Got it. Anyways, such a change needn't be part of this patch, so I'm > going to keep it unchanged. You mean with a read lock? -- Sincerely yours, Mike.