Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230720013249.199981-1-jannh@google.com> <CAJuCfpEs=jwaJEJQV9=Qo4h9JobrR1S=vETy1h8Q7TfS3eF_Qw@mail.gmail.com>
In-Reply-To: <CAJuCfpEs=jwaJEJQV9=Qo4h9JobrR1S=vETy1h8Q7TfS3eF_Qw@mail.gmail.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Thu, 20 Jul 2023 09:53:18 -0700
Message-ID: <CAJuCfpH-iwwzudP-RVd981fAdZnBOWxE=Z-kH3xQbwEN2=N0rw@mail.gmail.com>
Subject: Re: [PATCH] mm: Don't drop VMA locks in mm_drop_all_locks()
To:     Jann Horn <jannh@google.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Jul 20, 2023 at 9:51=E2=80=AFAM Suren Baghdasaryan <surenb@google.c=
om> wrote:
>
> On Wed, Jul 19, 2023 at 6:33=E2=80=AFPM Jann Horn <jannh@google.com> 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 <surenb@google.com>
> > Signed-off-by: Jann Horn <jannh@google.com>
>
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>
>
> > ---
> >  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
> >