On Fri, May 19, 2023 at 12:09 PM Joel Fernandes (Google)
<joel@xxxxxxxxxxxxxxxxx> wrote:
>
> +static bool check_addr_in_prev(struct vm_area_struct *vma, unsigned long addr,
> + unsigned long mask)
> +{
> + int addr_masked = addr & mask;
> + struct vm_area_struct *prev = NULL, *cur = NULL;
> +
> + /* If the masked address is within vma, there is no prev mapping of concern. */
> + if (vma->vm_start <= addr_masked)
> + return false;
Hmm.
I should have caught this last time, but I didn't.
That test smells bad to me. Or maybe it's just the comment.
I *suspect* that the test is literally just for the stack movement
case by execve, where it catches the case where we're doing the
movement entirely within the one vma we set up.
But in the *general* case I think the above is horribly wrong: if you
want to move pages within an existing mapping, the page moving code
can't just randomly say "I'll expand the area you wanted to move".
Again, in that general mremap() case (as opposed to the special stack
moving case for execve), I *think* that the caller has already split
the vma's at the point of the move, and this test simply cannot ever
trigger.
So I think the _code_ works, but I think the comment in particular is
questionable, and I'm a bit surprised about the code too,. because I
thought execve() only expanded to exactly the moving area.
End result: I think the patch on the whole looks nice, and smaller
than I expected. I also suspect it works in practice, but I'd like
that test clarified. Does it *actually* trigger for the stack moving
case? Because I think it must (never* trigger for the mremap case?
And maybe I'm the one confused here, and all I really need is an
explanation with small words and simple grammar starting with "No,
Linus, this is for case xyz"
Linus
