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 98D0BC4828D for ; Wed, 7 Feb 2024 20:25:09 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 07C4C6B0074; Wed, 7 Feb 2024 15:25:09 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 02BE06B0078; Wed, 7 Feb 2024 15:25:08 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E35826B007D; Wed, 7 Feb 2024 15:25:08 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id D029D6B0074 for ; Wed, 7 Feb 2024 15:25:08 -0500 (EST) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 5B4C5A2090 for ; Wed, 7 Feb 2024 20:25:08 +0000 (UTC) X-FDA: 81766137096.21.4F41197 Received: from mail-wr1-f53.google.com (mail-wr1-f53.google.com [209.85.221.53]) by imf24.hostedemail.com (Postfix) with ESMTP id 7FCD518000D for ; Wed, 7 Feb 2024 20:25:06 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=i80JtfMp; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf24.hostedemail.com: domain of lokeshgidra@google.com designates 209.85.221.53 as permitted sender) smtp.mailfrom=lokeshgidra@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707337506; 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=RAp9QYmAagl4q8JstQaVuR+UzMqpGvUOafiaUGshjJs=; b=hXDNZx0s9yNcVyDMZyGB0DwB6n80PTscCjDkiKnsK4oHv1LXB4S8rE0gW5cTJyG5saFIg/ pLltoj/BP54m5Dbcq/tHv9nU+pz00SW3IoSHYL3DzG/BgmOVa+fvIDRAJnXcJ21fNotUU8 KppcgJy5apUeXukck1sReDMI6IuxeOI= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=i80JtfMp; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf24.hostedemail.com: domain of lokeshgidra@google.com designates 209.85.221.53 as permitted sender) smtp.mailfrom=lokeshgidra@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707337506; a=rsa-sha256; cv=none; b=z+H/EzEKhUSZZxFhguwoBg4QsdyVcy0yUjVTKkt2u3GnWvGndDeaZOTNO3gT2X5Pxi5TlD IMSss8KmWDdTxWm0YqeusJ55gBgwktVSUyDP2VnA0aT1o1x8XCuYb5ann0NQM4ivCX0n7Z qtrjUsae9lIQClSgyi6t+lICoqP/d1I= Received: by mail-wr1-f53.google.com with SMTP id ffacd0b85a97d-33b436dbdcfso832231f8f.0 for ; Wed, 07 Feb 2024 12:25:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1707337505; x=1707942305; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=RAp9QYmAagl4q8JstQaVuR+UzMqpGvUOafiaUGshjJs=; b=i80JtfMpK7+B7My4oO1R8akxED2egDZYC2FMYIfQlgrT7nvrvYHyEQa5AXiQsWlFTT MlKa6x+Jvb9Xla4sze3NreQfIz+B3jLFyuYaA/ri9/lDQ59tZq8MeVdwfTi4jQb3bfGd H5XnX7GbxIarzezbAsmlFZ1gnxkifM5+4/JCNdM3yFOrJC2qOF9BNydDVbKIiQphFCWu CXYkU3QVrCLLj+uAe/VpmnFpHnmeIEAv/dlReTrO3EhqOq5HDpIO4iMJxYnGpuqrQl6m neUVRv9TvIftn9JJFGv1C209uajc1UN2KB6sCWcLnIxZA0/7w2XFAQsC52HENH8CwxXu SOLQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707337505; x=1707942305; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=RAp9QYmAagl4q8JstQaVuR+UzMqpGvUOafiaUGshjJs=; b=djbMSqm6VaTaF1GLoLDI6W0V2CWS3DsGAbjInOVqH4fy6JWY4GTQZVDBWxDdkqhabF eNfJElURnGui73otSg10etAM8UPROOlgfnza7nqnjdW4+GF4dyXKeqnWKpv4bS/gcGSe jD5MyNBIZHDQ9KQ2sD95+uf15gBbm2Qs0dyeQ8zBJlxMjHEaVWk30NVkfF9sV0agk8gl YPX3TbKSa3C7Jk7W2r6XBs9/055ELg3sugN/nr5w6DRD+iIIfvRFnxldijF48QqmMDID SOAPctoylEs4wyclirv1cyVI4rB3BBTJw2b1FSX3cO6lMJmvtp10WxBLwmvYyxuW9Wx5 EJuQ== X-Forwarded-Encrypted: i=1; AJvYcCV6GOXT0dfy56F8NWNCXKckRX3SSCjx3YjOw5dlSrNRQ5UJwPMvVpx+DioKq98hryVR9HofCW9mk+uoUKWKKUm4x3o= X-Gm-Message-State: AOJu0YwnUE4OsFonmuwFGOTUouief/amUNu6OOh3veYSowDGqeg3xVZV NqWBhZxyOPWM8lUBKwuk+YSw33KhUy6PTCYHHbaAY1Fu0+ANE0Bd4o8IIOCbsxUDFZ2xA3bxU/J kJHRZPJrgAH5JVf+3jhfuGiL2KCuVqYvxeGC+D1grg4jwlIkDxw7bvvw= X-Google-Smtp-Source: AGHT+IGQqWin6xslOwihUtxiZ2GFaThvfVui+rAPsrOr+dtRa0BkT869a3jIv2EUBTyktcfXEleTmaU5Z1oYmEKy778= X-Received: by 2002:adf:ff91:0:b0:33b:2177:2228 with SMTP id j17-20020adfff91000000b0033b21772228mr4665957wrr.59.1707337504834; Wed, 07 Feb 2024 12:25:04 -0800 (PST) MIME-Version: 1.0 References: <20240129193512.123145-1-lokeshgidra@google.com> <20240129193512.123145-3-lokeshgidra@google.com> <20240129210014.troxejbr3mzorcvx@revolver> <20240130034627.4aupq27mksswisqg@revolver> <20240130172831.hv5z7a7bhh4enoye@revolver> In-Reply-To: From: Lokesh Gidra Date: Wed, 7 Feb 2024 12:24:52 -0800 Message-ID: Subject: Re: [PATCH v2 2/3] userfaultfd: protect mmap_changing with rw_sem in userfaulfd_ctx To: Mike Rapoport 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 7FCD518000D X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: of1dyc669qsaqctbn4n4soigqrbdgrdt X-HE-Tag: 1707337506-581876 X-HE-Meta: U2FsdGVkX1/dD2RNnu2E0MCKpMZOJqsAef+F3ixjEwEOH8hsInxTVl+5HvNZG4ZsomW4DsPjAtuOPhfs1cyPhVuFF7BnduCCZpED29WbHvfDljSOCQrnPa9pF/M784i6dR40S3UykVqBuea/OHpNTv2faBv68MnpT3Tij06X25sBd6s2WRIKrcyuIKNsyDxw5R3/ZrleU7vu7gHJ2jqYWYZ6ol77CI8uRIf4lwj3gkY+ycr9a6eq5NCC0DXtO8KjBrxWbWzxSkfjRdARuMVA10tIuz2wDCzltA9aAxXzUC70JuA0hmDpjaaz3Sfq6I0jLrGtNeyxw/G/U93jtKwfMH47ojluvAkOJxiYngP43akdj8RaGrr5T/0FTJQhFfrAJ2u170U7Q+8HHppTpKp4LlyS8UVqS30YZddHH5ROAnBgWLEdvRLOwOlAm9KeQUJhIR8RzWbOMh1ZUq/X7P81DteTI7gQppfDoUPgvOaS2QBMAoOksxusKpo6sPlgKW3PkaPwRF4h/9AvKZjeR3VfFls5MkhDV05kpNO0ZZf0fC2gwbzOIJ1dX9Yxlvgz1LW5WOWRVFADyx5sKS41p8eguhyojsHrj9QmKcIH6JtI+Te5JAKZXtCB5xuWocEnUlVh5XtYFFrMn5sO8ES2OV2NtYB9XIChtqYPmeuuKsqRBY+iPp3ZQi4V75PLH9iu9R31Uj0hSrlD6V3jf4rWF0CpjZq6j0t7p/OQm5bG5M4G6sMaw40/MGpQFOtzp9FAICxsSGG6JHDmeBXyuL0XhTKDKx0BiR7Pjn+VL77iuyuCZg/QgIJGBGt+zgknRt4SAAYDqA5NYdwPXgXzCTU6C8XZ7ycYDACuor8syAIe4dPMsKIJfD3OmnlxHpSaObkTJbJJLGRsFT5rNp3aan98LDWOcE9sX3JcOA3fN9/NNpMRDvFAKp9AcDOUNmFoDJ/wN0ghu0lVMPjefTT4dymFBok C8p2LGfB thA51P2wASQmORhW+108op/BWWBZkSQW8M8g/Cngj8AI8O9YMf5PpQhpdKdgIFC3EhX0QxVXxjDJb0VnOPJYczzQU4l4+4AJ38c8ENC5E7AI84CAOEcErFWXH/iRz6Dk1IVXoBQBHly0qeJSddcArw/jeMBLT2/PZPtd/EDdDNQgjh+MgLOsVuEwcfqrGV3ikCmx3KvrI55DxLTZCGIegU/fx2kRH8+r8wiTHMLLF6e/xBAEDu7QS3YBQYd2NxqV6Z3/3t0YyGObGZEl42WzAbdxkw+LFNdYYF+ygqLVO/3FPdlYDisUAPkFFnM9J+akKpZRMfyq7DPhU8xixqyjmdbFPzg== 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 Wed, Feb 7, 2024 at 7:27=E2=80=AFAM Mike Rapoport wrot= e: > > On Mon, Feb 05, 2024 at 12:53:33PM -0800, Lokesh Gidra wrote: > > On Sun, Feb 4, 2024 at 2:27=E2=80=AFAM Mike Rapoport = wrote: > > > > > > > 3) Based on [1] I see how mmap_changing helps in eliminating duplic= ate > > > > 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 d= ata 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_a= rea_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 th= e readers > > > > > > > to leave? Can't you just increment the atomic? It's fine ha= ppening 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 rem= ember > > > > > > 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 moni= tor 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 en= ough. > > > > > > > > > > > > > > > > Right now this function won't stop to wait for readers to exit th= e > > > > > critical section, but with this change there will be a pause (sin= ce 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 re= moved, > > > > > where as before it was a fire & forget, this is now a wait to tel= l. > > > > > > > > > I think a lot will be clearer once we get a response to my question= s > > > > 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? No, I think write lock is good as it enables parallel background copy. Also because it brings consistency in blocking userfaultfd operations. I meant encapsulating remove operations within userfaultfd_remove_prep() and userfaultfd_remove_complete(). I couldn't figure out any need for that. > > -- > Sincerely yours, > Mike.