[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a6890607-4418-4356-a7f8-155f01ede14d@suse.cz>
Date: Thu, 22 Feb 2024 22:32:12 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>,
Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, Lorenzo Stoakes <lstoakes@...il.com>,
Michal Hocko <mhocko@...e.com>, stable@...r.kernel.org
Subject: Re: [PATCH] mm, mmap: fix vma_merge() case 7 with vma_ops->close
On 2/22/24 22:19, Vlastimil Babka wrote:
> On 2/22/24 20:27, Liam R. Howlett wrote:
>> * Liam R. Howlett <Liam.Howlett@...cle.com> [240222 13:56]:
>>> * Vlastimil Babka <vbabka@...e.cz> [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
>>> >
>
Powered by blists - more mailing lists