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: <000000000000607ff905ffc8e477@google.com> <0000000000000aeb7f06015e5cbd@google.com>
 <20230727164757.e2di75xjybxncohn@revolver> <CAJuCfpEyE18kbH84FfmfzUnar2dxgzpi=FOYPbU8MOpz-SbVjg@mail.gmail.com>
 <CAG48ez1yg7m=aNsjNiGt_s0_tEBEmEXXx0-vijuN9MBmoxL7PQ@mail.gmail.com> <ZMK0ony0LG2SL2Ha@casper.infradead.org>
In-Reply-To: <ZMK0ony0LG2SL2Ha@casper.infradead.org>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Thu, 27 Jul 2023 11:31:01 -0700
Message-ID: <CAJuCfpEWfFY_JXoXDnK0X+rY6Qf==RBpLzSSJhEtgU50HHp4-A@mail.gmail.com>
Subject: Re: [syzbot] [mm?] WARNING: suspicious RCU usage in mas_walk (2)
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Jann Horn <jannh@google.com>,
        "Liam R. Howlett" <Liam.Howlett@oracle.com>,
        akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, syzkaller-bugs@googlegroups.com,
        syzbot <syzbot+8645fe63c4d22c8d27b8@syzkaller.appspotmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Jul 27, 2023 at 11:17=E2=80=AFAM Matthew Wilcox <willy@infradead.or=
g> wrote:
>
> On Thu, Jul 27, 2023 at 07:59:33PM +0200, Jann Horn wrote:
> > On Thu, Jul 27, 2023 at 7:22=E2=80=AFPM Suren Baghdasaryan <surenb@goog=
le.com> wrote:
> > > Hmm. lock_vma_under_rcu() specifically checks for vma->anon_vma=3D=3D=
NULL
> > > condition (see [1]) to avoid going into find_mergeable_anon_vma() (a
> > > check inside anon_vma_prepare() should prevent that). So, it should
> > > fall back to mmap_lock'ing.
> >
> > This syzkaller report applies to a tree with Willy's in-progress patch
> > series, where lock_vma_under_rcu() only checks for vma->anon_vma if
> > vma_is_anonymous() is true - it permits private non-anonymous VMAs
> > (which require an anon_vma for handling write faults)  even if they
> > don't have an anon_vma.
> >
> > The commit bisected by syzkaller
> > (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/co=
mmit/?id=3Da52f58b34afe095ebc5823684eb264404dad6f7b)
> > removes the vma_is_anonymous() check in handle_pte_fault(), so it lets
> > us reach do_wp_page() with a non-anonymous private VMA without
> > anon_vma, even though that requires allocation of an anon_vma.
> >
> > So I think this is pretty clearly an issue with Willy's in-progress
> > patch series that syzkaller blamed correctly.

A comment for __anon_vma_prepare() says "This must be called with the
mmap_lock held for reading."
I think adding an explicit mmap_assert_locked() in this function would
help catch such issues.

>
> Agreed.  What do we think the right solution is?
>
> Option 1:
>
> +++ b/mm/memory.c
> @@ -3197,6 +3197,12 @@ static vm_fault_t wp_page_copy(struct vm_fault *vm=
f)
>         struct mmu_notifier_range range;
>         int ret;
>
> +       if (!vma->anon_vma) {
> +               // check if there are other things to undo here
> +               vma_end_read(vmf->vma);
> +               return VM_FAULT_RETRY;
> +       }
> +

This one bails out later but if the path is not taken too often I
think it's cleaner.


>         delayacct_wpcopy_start();
>
> Option 2:
>
> @@ -5581,7 +5587,8 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm=
_struct *mm,
>                 goto inval;
>
>         /* find_mergeable_anon_vma uses adjacent vmas which are not locke=
d */
> -       if (vma_is_anonymous(vma) && !vma->anon_vma)
> +       if ((vma_is_anonymous(vma) ||
> +            vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) && !vma->anon_vma=
)
>                 goto inval;
>
> The problem with option 2 is that we don't know whether this is a write
> fault or not, so we'll handle read faults on private file
> mappings under the mmap_lock UNTIL somebody writes to the mapping, which
> might be never.  That seems like a bad idea.
>
> We could pass FAULT_FLAG_WRITE into lock_vma_under_rcu(), but that also
> seems like a bad idea.  I dunno.  Three bad ideas.  Anyone think of a
> good one?