Received-SPF: pass (google.com: domain of linux-kernel+bounces-77405-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <a6890607-4418-4356-a7f8-155f01ede14d@suse.cz>
Date: Thu, 22 Feb 2024 22:32:12 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mm, mmap: fix vma_merge() case 7 with vma_ops->close
Content-Language: en-US
From: Vlastimil Babka <vbabka@suse.cz>
To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
 Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
 linux-kernel@vger.kernel.org, Lorenzo Stoakes <lstoakes@gmail.com>,
 Michal Hocko <mhocko@suse.com>, stable@vger.kernel.org
References: <20240222165549.32753-2-vbabka@suse.cz>
 <20240222185642.rmnp76oquu5wfo6c@revolver>
 <20240222192746.cb65qvtmhaikfeko@revolver>
 <18565605-7f68-4950-b66f-496c1f3c393b@suse.cz>
In-Reply-To: <18565605-7f68-4950-b66f-496c1f3c393b@suse.cz>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 2/22/24 22:19, Vlastimil Babka wrote:
> On 2/22/24 20:27, Liam R. Howlett wrote:
>> * Liam R. Howlett <Liam.Howlett@Oracle.com> [240222 13:56]:
>>> * Vlastimil Babka <vbabka@suse.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
>>> > 
>