Re: WARN_ON in move_normal_pmd

[Joel Fernandes]

Hi Linus,

On Fri, Mar 24, 2023 at 04:38:03PM -0700, Linus Torvalds wrote:
> On Fri, Mar 24, 2023 at 6:43 AM Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
> >
> > Wouldn't it be better to instead fix it from the caller side? Like
> > making it non-overlapping.
> 
> I wonder if we could just do something like this in mremap() instead
> 
>  - if old/new are mutually PMD_ALIGNED
> 
>  - *and* there is no vma below new within the same PMD
> 
>  - then just expand the mremap to be PMD-aligned downwards
> 
> IOW, the problem with the exec stack moving case isn't really that
> it's overlapping: that part is fine. We're moving downwards, and we
> start from the bottom, so the moving part works fine.
> 
> No, the problem is that we *start* by moving individual pages, and
> then by the time we've a few pages down by a whole PMD, we finish the
> source PMD (and we've cleared all the contents of it), but it still
> exists.
> 
> And at *that* point, when we go and start copying the next page, we're
> suddenly fully PMD-aligned, and now we try to copy a whole PMD, and
> then that code is unhappy about the fact that the old (empty) PMD is
> there in the target.
> 

You are very right. I am able to also trigger the warning with a synthetic
program that just mremaps a range and moves it in the same way that triggers
this issue, however I had to hack the kernel to prevent mremap from erroring
out if ranges overlap (unlike exec, mremap does some initial checks for
that). Also had to do other hacks but I did reproduce it consistently.

The issue is that even though the PMD is empty, it is allocated. So
pmd_none() is kind of a lie in some sense, it is pointing to empty PTEs when
the range is really empty.

How about we replace the warning with something like the following?

+       if (unlikely(!pmd_none(*new_pmd))) {
+               // Check if any ptes in the pmd are non-empty. Doing this here
+               // is ok since this is not a fast path.
+               bool pmd_empty = true;
+               unsigned long tmp_addr = new_addr;
+               pte_t* check_pte = pte_offset_map(new_pmd, new_addr);
+
+               new_ptl = pte_lockptr(mm, new_pmd);
+               spin_lock_nested(new_ptl, SINGLE_DEPTH_NESTING);
+               for (; tmp_addr < old_addr + PMD_SIZE; check_pte++, tmp_addr += PAGE_SIZE) {
+                       if (!pte_none(*check_pte)) {
+                               pmd_empty = false;
+                               break;
+                       }
+               }

+               WARN_ON_ONCE(!pmd_empty);
+               spin_unlock(new_ptl);
+       }
+

> And for all of this to happen, we need to move things by an exact
> multiple of PMD size, because otherwise we'd never get to that aligned
> situation at all, and we'd always do all the movement in individual
> pages, and everything would be just fine.
> 
> And more importantly, if we had just *started* with moving a whole
> PMD, this also wouldn't have happened. But we didn't. We started
> moving individual pages.
> 
> So you could see the warning not as a "this range overlaps" warning
> (it's fine, and happens all the time, and we do individual pages that
> way quite happily), but really as a "hey, this was very inefficient -
> you shouldn't have done those individual pages as several small
> independent invidual pages in the first place" warning.
> 

Exactly.

> So some kind of
> 
>         /* Is the movement mutually PMD-aligned? */
>         if ((old_addr ^ new_addr) & ~PMD_MASK == 0) {
>                 .. try to extend the move_vma() down to the *aligned*
> PMD case ..
>         }
> 

I actually didn't follow what you meant by "mutually PMD-aligned". Could you
provide some example address numbers to explain?

AFAIK, only 2MB aligned memory addresses can be directly addressed by a PMD.
Otherwise how will you index the bytes in the 2MB page? You need bits in the
address for that.

> logic in move_page_tables() would get rid of the warning, and would
> make the move more efficient since you'd skip the "move individual
> pages and allocate a new PMD" case entirely.
> 
> This is all fairly com,plicated, and the "try to extend the move
> range" would also have to depend on CONFIG_HAVE_MOVE_PMD etc, so I'm
> not saying it's trivial.
> 
> But it would seem to be a really nice optimization, in addition to
> getting rid of the warning.
> 
> It could even help real world cases outside of this odd stack
> remapping case if users ever end up moving vma's by multiples of
> PMD_SIZE, and there aren't other vma's around the source/target that
> disable the optimization.
> 
> Hmm? Anybody want to look into that? It looks hairy enough that I
> think that "you could test this with mutually aligned mremap()
> source/targets in some test program" would be a good thing. Because
> the pure execve() case is rare enough that using *that* as a test-case
> seems like a fool's errand.
> 

Just to mention, mremap errors out if you try to move overlapping ranges
because this in  mremap_to():

       /* Ensure the old/new locations do not overlap
        if (addr + old_len > new_addr && new_addr + new_len > addr) {
                pr_err("%s: (%s) (%d)", __func__, __FILE__, __LINE__);
                goto out;
        }

Or is there an mremap trick I might be missing which actually allows
overlapping range moves?

thanks,

 - Joel