Message-ID: <CAHbLzkoQV_1LzxtdEazfPmSEh+CgRLpED6nxdTZ1D=2D1HwV4g@mail.gmail.com>
Date:   Mon, 18 Sep 2023 15:34:07 -0700
From:   Yang Shi <shy828301@...il.com>
To:     Hugh Dickins <hughd@...gle.com>
Cc:     Suren Baghdasaryan <surenb@...gle.com>,
        Matthew Wilcox <willy@...radead.org>,
        Michal Hocko <mhocko@...e.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        syzbot <syzbot+b591856e0f0139f83023@...kaller.appspotmail.com>,
        akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [mm?] kernel BUG in vma_replace_policy

On Fri, Sep 15, 2023 at 8:57 PM Hugh Dickins <hughd@...gle.com> wrote:
>
> On Fri, 15 Sep 2023, Yang Shi wrote:
> >
> > Hi Suren and Hugh,
> >
> > Thanks for figuring this out. The mbind behavior is a little bit messy
> > and hard to follow. I tried my best to recall all the changes.
>
> Messy and confusing yes; and for every particular behavior, I suspect
> that by now there exists some release which has done it that way.
>
> >
> > IIUC, mbind did break the vma iteration early in the first place, then
> > commit 6f4576e3687b ("mempolicy: apply page table walker on
> > queue_pages_range()") changed the behavior (didn't break vma iteration
> > early for some cases anymore), but it messed up the return value and
> > caused some test cases failure, also violated the manual. The return
> > value issue was fixed by commit a7f40cfe3b7a ("mm: mempolicy: make
> > mbind() return -EIO when MPOL_MF_STRICT is specified"), this commit
> > also restored the oldest behavior (break loop early). But it also
> > breaks the loop early when MPOL_MF_MOVE|MOVEALL is set, kernel should
> > actually continue the loop to try to migrate all existing pages per
> > the manual.
>
> Oh, I missed that aspect in my description: yes, I think that's the
> worst of it: MPOL_MF_STRICT alone could break out early because it had
> nothing more to learn by going further, but it was simply a mistake for
> the MOVEs to break out early (and arguable what MOVE|STRICT should do).
>
> I thought you and I were going to have a debate about this, but we
> appear to be in agreement.  And I'm not sure whether I agree with
> myself about whether do_mbind() should apply the mbind_range()s
> when STRICT queue_pages_range() found an unmovable - there are
> consistency and regression arguments both ways.

They will not be added into the migration list in the first place. Why
waste time to try to migrate the unmovable?

>
> (I've been repeatedly puzzled by your comment in queue_folios_pte_range()
>                 if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
>                         /* MPOL_MF_STRICT must be specified if we get here */
>                         if (!vma_migratable(vma)) {
> Does that commment about MPOL_MF_STRICT actually belong inside the
> !vma_migratable(vma) block?  Sometimes I think so, but sometimes I
> remember that the interaction of those flags, and the skipping arranged
> by queue_pages_test_walk(), is subtler than I imagine.)

It is because the below code snippet from queue_pages_test_walk():

if (!vma_migratable(vma) &&
    !(flags & MPOL_MF_STRICT))
return 1;

When queue_pages_test_walk() returns 1, queue_folios_pte_range() will
be skipped. So if queue_folios_pte_range() sees unmigratable vma, it
means MPOL_MF_STRICT must be set.

>
> > It sounds like a regression. I will take a look at it.
>
> Thanks! Please do, I don't have the time for it.
>
> >
> > So the logic should conceptually look like:
> >
> > if (MPOL_MF_MOVE|MOVEALL)
> >     continue;
> > if (MPOL_MF_STRICT)
> >     break;
> >
> > So it is still possible that some VMAs are not locked if only
> > MPOL_MF_STRICT is set.
>
> Conditionally, I'll agree; but it's too easy for me to agree in the
> course of trying to get an email out, but on later reflection come
> to disagree.  STRICT|MOVE behavior arguable.

I thought the code should conceptually do:

if (MPOL_MF_MOVE|MOVEALL)
    scan all vmas
    try to migrate the existing pages
    return success
else if (MPOL_MF_MOVE* | MPOL_MF_STRICT)
    scan all vmas
    try to migrate the existing pages
    return -EIO if unmovable or migration failed
else /* MPOL_MF_STRICT alone */
    break early if meets unmovable and don't call mbind_range() at all

So the vma scan will just be skipped when MPOL_MF_STRICT alone is
specified and mbind_range() won't be called in this case. So Suren's
fix may not be needed.

>
> I think the best I can do is send you (privately) my approx-v5.2 patch
> for this (which I never got time to put into even a Google-internal
> kernel, though an earlier version was there).  In part because I did
> more research back then, and its commit message cites several even
> older commits than you cite above, which might help to shed more light
> on the history (or might just be wrong).  And in part because it may
> give you some more ideas of what needs doing: notably qp->nr_failed,
> because "man 2 migrate_pages" says "On success migrate_pages() returns
> the number of pages that could not be moved", but we seem to have
> lost sight of that (from which one may conclude that it's not very
> important, but I did find it useful when testing); but of course
> the usual doubts about the right way to count a page when compound.
>
> I'll check how easily that patch applies to a known base such as
> v5.2, maybe trim it to fit better, then send it off to you.

I'm thinking about the below fix (build test against the latest
mm-unstable only):

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 42b5567e3773..c9b768a042a8 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -426,6 +426,7 @@ struct queue_pages {
  unsigned long start;
  unsigned long end;
  struct vm_area_struct *first;
+ bool has_unmovable;
 };

 /*
@@ -446,9 +447,8 @@ static inline bool queue_folio_required(struct folio *folio,
 /*
  * queue_folios_pmd() has three possible return values:
  * 0 - folios are placed on the right node or queued successfully, or
- *     special page is met, i.e. huge zero page.
- * 1 - there is unmovable folio, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
- *     specified.
+ *     special page is met, i.e. zero page, or unmovable page is found
+ *     but continue walking (indicated by queue_pages.has_unmovable).
  * -EIO - is migration entry or only MPOL_MF_STRICT was specified and an
  *        existing folio was already on a node that does not follow the
  *        policy.
@@ -479,7 +479,7 @@ static int queue_folios_pmd(pmd_t *pmd, spinlock_t
*ptl, unsigned long addr,
  if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
  if (!vma_migratable(walk->vma) ||
      migrate_folio_add(folio, qp->pagelist, flags)) {
- ret = 1;
+ qp->has_unmovable |= 1;
  goto unlock;
  }
  } else
@@ -495,9 +495,8 @@ static int queue_folios_pmd(pmd_t *pmd, spinlock_t
*ptl, unsigned long addr,
  *
  * queue_folios_pte_range() has three possible return values:
  * 0 - folios are placed on the right node or queued successfully, or
- *     special page is met, i.e. zero page.
- * 1 - there is unmovable folio, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
- *     specified.
+ *     special page is met, i.e. zero page, or unmovable page is found
+ *     but continue walking (indicated by queue_pages.has_unmovable).
  * -EIO - only MPOL_MF_STRICT was specified and an existing folio was already
  *        on a node that does not follow the policy.
  */
@@ -538,10 +537,13 @@ static int queue_folios_pte_range(pmd_t *pmd,
unsigned long addr,
  if (!queue_folio_required(folio, qp))
  continue;
  if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
- /* MPOL_MF_STRICT must be specified if we get here */
+ /*
+ * MPOL_MF_STRICT must be specified if we get here.
+ * Continue walking vmas due to MPOL_MF_MOVE* flags.
+ */
  if (!vma_migratable(vma)) {
- has_unmovable = true;
- break;
+ qp->has_unmovable |= 1;
+ continue;
  }

  /*
@@ -550,16 +552,13 @@ static int queue_folios_pte_range(pmd_t *pmd,
unsigned long addr,
  * need migrate other LRU pages.
  */
  if (migrate_folio_add(folio, qp->pagelist, flags))
- has_unmovable = true;
+ has_unmovable |= 1;
  } else
  break;
  }
  pte_unmap_unlock(mapped_pte, ptl);
  cond_resched();

- if (has_unmovable)
- return 1;
-
  return addr != end ? -EIO : 0;
 }

@@ -599,7 +598,7 @@ static int queue_folios_hugetlb(pte_t *pte,
unsigned long hmask,
  * Detecting misplaced folio but allow migrating folios which
  * have been queued.
  */
- ret = 1;
+ qp->has_unmovable |= 1;
  goto unlock;
  }

@@ -620,7 +619,7 @@ static int queue_folios_hugetlb(pte_t *pte,
unsigned long hmask,
  * Failed to isolate folio but allow migrating pages
  * which have been queued.
  */
- ret = 1;
+ qp->has_unmovable |= 1;
  }
 unlock:
  spin_unlock(ptl);
@@ -756,12 +755,15 @@ queue_pages_range(struct mm_struct *mm, unsigned
long start, unsigned long end,
  .start = start,
  .end = end,
  .first = NULL,
+ .has_unmovable = false,
  };
  const struct mm_walk_ops *ops = lock_vma ?
  &queue_pages_lock_vma_walk_ops : &queue_pages_walk_ops;

  err = walk_page_range(mm, start, end, ops, &qp);

+ if (qp.has_unmovable)
+ err = 1;
  if (!qp.first)
  /* whole range in hole */
  err = -EFAULT;
@@ -1358,7 +1360,7 @@ static long do_mbind(unsigned long start,
unsigned long len,
  putback_movable_pages(&pagelist);
  }

- if ((ret > 0) || (nr_failed && (flags & MPOL_MF_STRICT)))
+ if (((ret > 0) || nr_failed) && (flags & MPOL_MF_STRICT))
  err = -EIO;
  } else {
 up_out:

>
> Hugh

Powered by blists - more mailing lists