On Fri, May 19, 2023 at 12:09 PM Joel Fernandes (Google) joel@joelfernandes.org 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