Re: [syzbot] [mm?] kernel BUG in vma_replace_policy

[Suren Baghdasaryan]

On Thu, Sep 14, 2023 at 9:24 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Thu, Sep 14, 2023 at 08:53:59PM +0000, Suren Baghdasaryan wrote:
> > On Thu, Sep 14, 2023 at 8:00 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Sep 14, 2023 at 7:09 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> > > >
> > > > On Thu, Sep 14, 2023 at 06:20:56PM +0000, Suren Baghdasaryan wrote:
> > > > > I think I found the problem and the explanation is much simpler. While
> > > > > walking the page range, queue_folios_pte_range() encounters an
> > > > > unmovable page and queue_folios_pte_range() returns 1. That causes a
> > > > > break from the loop inside walk_page_range() and no more VMAs get
> > > > > locked. After that the loop calling mbind_range() walks over all VMAs,
> > > > > even the ones which were skipped by queue_folios_pte_range() and that
> > > > > causes this BUG assertion.
> > > > >
> > > > > Thinking what's the right way to handle this situation (what's the
> > > > > expected behavior here)...
> > > > > I think the safest way would be to modify walk_page_range() and make
> > > > > it continue calling process_vma_walk_lock() for all VMAs in the range
> > > > > even when __walk_page_range() returns a positive err. Any objection or
> > > > > alternative suggestions?
> > > >
> > > > So we only return 1 here if MPOL_MF_MOVE* & MPOL_MF_STRICT were
> > > > specified.  That means we're going to return an error, no matter what,
> > > > and there's no point in calling mbind_range().  Right?
> > > >
> > > > +++ b/mm/mempolicy.c
> > > > @@ -1334,6 +1334,8 @@ static long do_mbind(unsigned long start, unsigned long len,
> > > >         ret = queue_pages_range(mm, start, end, nmask,
> > > >                           flags | MPOL_MF_INVERT, &pagelist, true);
> > > >
> > > > +       if (ret == 1)
> > > > +               ret = -EIO;
> > > >         if (ret < 0) {
> > > >                 err = ret;
> > > >                 goto up_out;
> > > >
> > > > (I don't really understand this code, so it can't be this simple, can
> > > > it?  Why don't we just return -EIO from queue_folios_pte_range() if
> > > > this is the right answer?)
> > >
> > > Yeah, I'm trying to understand the expected behavior of this function
> > > to make sure we are not missing anything. I tried a simple fix that I
> > > suggested in my previous email and it works but I want to understand a
> > > bit more about this function's logic before posting the fix.
> >
> > So, current functionality is that after queue_pages_range() encounters
> > an unmovable page, terminates the loop and returns 1, mbind_range()
> > will still be called for the whole range
> > (https://elixir.bootlin.com/linux/latest/source/mm/mempolicy.c#L1345),
> > all pages in the pagelist will be migrated
> > (https://elixir.bootlin.com/linux/latest/source/mm/mempolicy.c#L1355)
> > and only after that the -EIO code will be returned
> > (https://elixir.bootlin.com/linux/latest/source/mm/mempolicy.c#L1362).
> > So, if we follow Matthew's suggestion we will be altering the current
> > behavior which I assume is not what we want to do.
>
> Right, I'm intentionally changing the behaviour.  My thinking is
> that mbind(MPOL_MF_MOVE | MPOL_MF_STRICT) is going to fail.  Should
> such a failure actually move the movable pages before reporting that
> it failed?  I don't know.
>
> > The simple fix I was thinking about that would not alter this behavior
> > is smth like this:
>
> I don't like it, but can we run it past syzbot to be sure it solves the
> issue and we're not chasing a ghost here?

Yes, I just finished running the reproducer on both upstream and
linux-next builds listed in
https://syzkaller.appspot.com/bug?extid=b591856e0f0139f83023 and the
problem does not happen anymore.
I'm fine with your suggestion too, just wanted to point out it would
introduce change in the behavior. Let me know how you want to proceed.