Re: [PATCH] mm, mmap: fix vma_merge() case 7 with vma_ops->close

[Vlastimil Babka]

On 2/22/24 22:19, Vlastimil Babka wrote:
> On 2/22/24 20:27, Liam R. Howlett wrote:
>> * Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> [240222 13:56]:
>>> * Vlastimil Babka <vbabka@xxxxxxx> [240222 11:56]:
>>> This separates the check for potentially merging previous to a later
>>> failure case.  Would it be better to check:
>>> 	if (curr && curr->vm_ops && curr->vm_ops->close)
>>> 
>>> and not set merge_prev = true, ie we cannot merge with the predecessor?
> 
> Good suggestion, thanks!

Or actually as Lorenzo informed me, it would affect case 5 as well and we
don't want that. And special casing 5 vs 7 that early would be ugly again.

So I'll just do the code dedup Lorenzo suggested...

>>> That way we would exit as merge_prev == false.
>>> 
>>> We would have the added benefit of not having to look at merge_prev &
>>> merge_next case with this vm_ops->close in mind (case 1 and 6).. because
>>> I'm pretty sure we can currently get to case 6 in this way:
>>> 
>>> merge_prev = true
>>> check for merge_next.. can_vma_merge_before(next...);
>>> is_mergeable_vma(next.... , true);
>>> if (true && next->vm_ops && next->vm_ops->close) /* Fine for next.. */
>>> 
>>> Remove curr by case 6 without checking curr->vm_ops &&
>>> curr->vm_ops->close
>>> 
>>> If I am correct, then are we blaming the right commit?
> 
> It was bisected with no nondeterminism in the test, so yeah.
> 
>> I am not correct.
>> The file check will ensure the same ops, so the file and ops must match.
>> As long as both are checked on one VMA then it will work as required.
> 
> Right, otherwise we would have bigger issues even before the buggy commit,
> we were never checking curr's vma_ops before.
> 
>>> 
>>> Perhaps we should just fail earlier when we find a curr with the close
>>> ops?
>> 
>> I'd rather fail earlier, but it's not a big deal.
> 
> Your suggestion will indeed result in a nicer and more obvious code, so will
> do, thanks!
> 
>>> 
>>> >  			} else {			/* case 5 */
>>> > +				err = dup_anon_vma(prev, curr, &anon_dup);
>>> >  				adjust = curr;
>>> >  				adj_start = (end - curr->vm_start);
>>> >  			}
>>> > -- 
>>> > 2.43.1
>>> > 
>