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 8E8BBEB64DA for ; Thu, 20 Jul 2023 16:53:33 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1AE32280144; Thu, 20 Jul 2023 12:53:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 15E4028004C; Thu, 20 Jul 2023 12:53:33 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F4167280144; Thu, 20 Jul 2023 12:53:32 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id E207328004C for ; Thu, 20 Jul 2023 12:53:32 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id AD5CB4020F for ; Thu, 20 Jul 2023 16:53:32 +0000 (UTC) X-FDA: 81032586264.21.FFAA7FA Received: from mail-yb1-f179.google.com (mail-yb1-f179.google.com [209.85.219.179]) by imf09.hostedemail.com (Postfix) with ESMTP id DE1DE140014 for ; Thu, 20 Jul 2023 16:53:30 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=Nux4DICX; spf=pass (imf09.hostedemail.com: domain of surenb@google.com designates 209.85.219.179 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1689872010; a=rsa-sha256; cv=none; b=771rZcT4Zy/EuLG96EEPOXkdKGS9yBMIlJ5soAumqbUkDoxdCHPSDN8WyWByUmZJSlAtIA c4iFdjd7VIADH/ZDP1TZeXtIdOoI0QJrNDHmWnqnpu5uL/JPrHP0+WH4ny8/Ipp0EA4Sqq 8wBj/IaI3Nl7GP8Kt6V05Va+wked1v4= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=Nux4DICX; spf=pass (imf09.hostedemail.com: domain of surenb@google.com designates 209.85.219.179 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1689872010; 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=afUwPeZDgES71SVrZPVCgwP/hpGD5z58VLoiPj5sM7Y=; b=lvuRmAj/KSMyXoznjKRE8R54NzCONYHDus7r+E8rf6eHg6Qu545BshnNOeKZqMARCauwZn t8d/IdJN3gK6PfwSFZpB4zzACAmPflfXQv6YqwM8Y5/85QP7rxoNMh2GTOauY46XTeVnFU B1SgC/a4W9cyT3fDPPlWEUyVn2HApmY= Received: by mail-yb1-f179.google.com with SMTP id 3f1490d57ef6-cfd4ea89978so884179276.2 for ; Thu, 20 Jul 2023 09:53:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1689872010; x=1690476810; 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=afUwPeZDgES71SVrZPVCgwP/hpGD5z58VLoiPj5sM7Y=; b=Nux4DICX4FPvUdzUa82d9VQtoJbLwxOQFwg1iE4FTTT0cQulb4kEHiLA5fjeKiatt6 08pW7Iq4QoQ8I7yIoIZGYU6mL5XLGm7NUJDRj1WvUSn+5zrkAMH40OrC0ytszPbL80/X LwxffghmZUn48Ej1/ciLLRLt5pV+lqLqQRvjJEXdmgS2WDZ2ByJc5ELG+ynOM4dwvNwt II6rkqBK8kkMViTU7eOXUiaxIrWiBpvRRHXDYJgcosAjeQLh9hx4YPRZM1WXobB3ZGLV mokBwmLO+jAWbJweoI38HWnmsUBK27R7Rmfn6e6DJ4U0DEg2y+kTsfPP4zS3VfcOQgeF P7VA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689872010; x=1690476810; 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=afUwPeZDgES71SVrZPVCgwP/hpGD5z58VLoiPj5sM7Y=; b=PCKPFbVkNOs1sUN6t3m7OQRph4vp7TQlbdiSfkd2mWHRjJfS4a5O3yGAf+RG4HQozD Q8sMGd9TSbxbj5fUrheef+kesK/AtvNuWZBgVm32KBRkP3rA37evAT477WZk/klL8MjQ JjI6T6EiK1ZwBUjff1uBHxUx4LAJ1dgZBKLxnBq531C4LZ0Glnk9YF4P0a1H2Vgecgwj OrHxQ5Kf6+iRaC5GLWfsz0MWMu3bwB8wzLpa0iUDlLWKiSREZa07a+96WA+ZdZMAElST SwST2RooZPq0V9O3rYIjnStB6Uqjzkvwzmu8zLvxPmFLT6VVLYad1r+Q06LofqmRcaMZ ECng== X-Gm-Message-State: ABy/qLb9jeGEfaNa2c8mgLRjGZz/vNGmLnyJNprPcOTAKAgovgJXSfPl K70NWzr3z1JAdWKIKkih3QbX5Y5eHNpVKe5Ix1b8Ww== X-Google-Smtp-Source: APBJJlGEoD1ID63gguX/BQy+QoSeuwk25jEhSpxWpmXM9YSjJkwOC9KMJA1v50LvSLks7bqCB1Xc5pzVcZD7oawxhM0= X-Received: by 2002:a25:aba1:0:b0:c6f:db67:cbf7 with SMTP id v30-20020a25aba1000000b00c6fdb67cbf7mr5114750ybi.16.1689872009815; Thu, 20 Jul 2023 09:53:29 -0700 (PDT) MIME-Version: 1.0 References: <20230720013249.199981-1-jannh@google.com> In-Reply-To: From: Suren Baghdasaryan Date: Thu, 20 Jul 2023 09:53:18 -0700 Message-ID: Subject: Re: [PATCH] mm: Don't drop VMA locks in mm_drop_all_locks() To: Jann Horn Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: DE1DE140014 X-Stat-Signature: cei6t5zc5u5wwnw7zzgksa447huruys4 X-Rspam-User: X-HE-Tag: 1689872010-956162 X-HE-Meta: U2FsdGVkX1/3m67U0fpXfV9WvYy8vO+VNAnrLrQLQzW3ExDpImw9i8sfS++kBokGpCFWdjNMTwpQd83mLisWqtkBdyz/6m3+TOFVviSUHc2Gcjc2DLKz1rrsBm0qT3ZaTzu4fTSWT2WmtptDYs92yRPkYoI6h7p+PIdCChqSZXWv/nhnVczZfW3JIKC7e3aviOzHraSVISmmWhaj6K4/eo991CuZCsrSnhMd+MMAm+7OjKVV9vB/z7XBVfdy7LDYAqCcSx6tWEMboGR0E8tIFjCHJ9jT8tl3Co3UvA6yE4e66kkDTZ1MDl1L1zmXScUtS2HV150fXzLeUMGZgFLy+AjtCJmS0JBlS6+0blr4YRB0sVoKt778GPdtL0iAzDWT4BNf9nCwJmMhY1HkDJGUZSZmjfhLvzj7RMk1l5rT3ZJ1s7oh/BrUDagP6hljZztPvZATTcq1eF0mXOqQdZLAd8FKdHLZMw6tpoEl1Sh4fPD3Xqpt6scA0CBa6wQL0Nhtijz9qyBtQKNu0JWq49su43qguaMVS+yK6ob+Sjd3Vtr8gtNaLx3EUn4VsEkdPxsTBf49oMbNT7BnsmoKIW6Otcki60RhvWH1yk2EIgGbgKjCXdFQj18XFGTl/NzdV+/eEPevSSGuhmjk9GDk+s7SO5GNt6bhhhpZSgbx30AcsJ6jPLR4rejjheoPVHtBb1jTu+pqhG623zGd7HEJcr7lNILSb2fWZyGOcOXcgAp0RLh1wcxB228Zfy1b6NyIbKheQrZJUcdv/h63mKQMvMLhd0wQNbF8jmJ6oyPVOEF/sHAtMHTy57SmrdrG6hVdggdTHNcD/TU9Fk4YLeE9ScBrTd1vla5pTjbtRXeQHuiRvNOSSnXUOQCSqmZGGevxrT4NbI8lnlVjpu/zIx33ylGpqG9UX4i2yjY8W8chm3CvRzisyfMU7f85kLHX3MTv0pxK+mV6tmOU36MeS3w67q9 aMMcAp7r HuQB8dXQuzpFHAjSVvUmdcdvsB7RhehfP+/dpyWQ1wJcTg8ySHGd+ggEjKAJurJU2TJF49FniNwKLTno6TEJ4LbX6HEtjj4lW0DJgrXAaqcnv2zgvjAw+IEWRsz/j0tGBAlTpIETH4psp0UZSTt1ImBj4Pfs0EtXxFO0orrB8duer2r+4dcfHl11TVq1VzPOiwtszX6zCbCTRTM0BZYT9bRmItaJGJm3+mT2dNcUEtyoYzvACsHC85nI5GdtH+KZdtwiymjdFdob4NpGepIcv1AhJz9wNo283okgy98D0bT2Vrnw= X-Bogosity: Ham, tests=bogofilter, spamicity=0.030596, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Jul 20, 2023 at 9:51=E2=80=AFAM Suren Baghdasaryan wrote: > > On Wed, Jul 19, 2023 at 6:33=E2=80=AFPM Jann Horn wrot= e: > > > > Despite its name, mm_drop_all_locks() does not drop _all_ locks; the mm= ap > > lock is held write-locked by the caller, and the caller is responsible = for > > dropping the mmap lock at a later point (which will also release the VM= A > > locks). > > Calling vma_end_write_all() here is dangerous because the caller might = have > > write-locked a VMA with the expectation that it will stay write-locked > > until the mmap_lock is released, as usual. > > > > This _almost_ becomes a problem in the following scenario: > > > > An anonymous VMA A and an SGX VMA B are mapped adjacent to each other. > > Userspace calls munmap() on a range starting at the start address of A = and > > ending in the middle of B. > > > > Hypothetical call graph with additional notes in brackets: > > > > do_vmi_align_munmap > > [begin first for_each_vma_range loop] > > vma_start_write [on VMA A] > > vma_mark_detached [on VMA A] > > __split_vma [on VMA B] > > sgx_vma_open [=3D=3D new->vm_ops->open] > > sgx_encl_mm_add > > __mmu_notifier_register [luckily THIS CAN'T ACTUALLY HAPPEN] > > mm_take_all_locks > > mm_drop_all_locks > > vma_end_write_all [drops VMA lock taken on VMA A before] > > vma_start_write [on VMA B] > > vma_mark_detached [on VMA B] > > [end first for_each_vma_range loop] > > vma_iter_clear_gfp [removes VMAs from maple tree] > > mmap_write_downgrade > > unmap_region > > mmap_read_unlock > > > > In this hypothetical scenario, while do_vmi_align_munmap() thinks it st= ill > > holds a VMA write lock on VMA A, the VMA write lock has actually been > > invalidated inside __split_vma(). > > > > The call from sgx_encl_mm_add() to __mmu_notifier_register() can't > > actually happen here, as far as I understand, because we are duplicatin= g an > > existing SGX VMA, but sgx_encl_mm_add() only calls > > __mmu_notifier_register() for the first SGX VMA created in a given proc= ess. > > So this could only happen in fork(), not on munmap(). > > But in my view it is just pure luck that this can't happen. > > > > Also, we wouldn't actually have any bad consequences from this in > > do_vmi_align_munmap(), because by the time the bug drops the lock on VM= A A, > > we've already marked VMA A as detached, which makes it completely > > ineligible for any VMA-locked page faults. > > But again, that's just pure luck. > > > > So remove the vma_end_write_all(), so that VMA write locks are only eve= r > > released on mmap_write_unlock() or mmap_write_downgrade(). > > Your logic makes sense to be. mm_drop_all_locks() unlocking all VMAs, > even the ones which were locked before mm_take_all_locks() seems > dangerous. > One concern I have is that mm_take_all_locks() and mm_drop_all_locks() > become asymmetric with this change: mm_take_all_locks() locks all VMAs > but mm_drop_all_locks() does not release them. I think there should be > an additional comment explaining this asymmetry. > Another side-effect which would be nice to document in a comment is > that when mm_take_all_locks() fails after it locked the VMAs, those > VMAs will stay locked until mmap_write_unlock/mmap_write_downgrade. > This happens because of failure mm_take_all_locks() jumps to perform s/of/on in the above statement > mm_drop_all_locks() and this will not unlock already locked VMAs. > Other than that LGTM. Thanks! > > > > > Fixes: eeff9a5d47f8 ("mm/mmap: prevent pagefault handler from racing wi= th mmu_notifier registration") > > Cc: Suren Baghdasaryan > > Signed-off-by: Jann Horn > > Reviewed-by: Suren Baghdasaryan > > > --- > > mm/mmap.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index 3eda23c9ebe7..1ff354b1e23c 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -3758,7 +3758,6 @@ void mm_drop_all_locks(struct mm_struct *mm) > > if (vma->vm_file && vma->vm_file->f_mapping) > > vm_unlock_mapping(vma->vm_file->f_mapping); > > } > > - vma_end_write_all(mm); > > > > mutex_unlock(&mm_all_locks_mutex); > > } > > > > base-commit: bfa3037d828050896ae52f6467b6ca2489ae6fb1 > > -- > > 2.41.0.255.g8b1d071c50-goog > >