Message-ID: <997d99e4-6ebd-c23e-ca1e-b62155701cb0@suse.cz>
Date:   Thu, 16 Mar 2023 09:35:39 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH 10/10] mm/mremap: simplify vma expansion again
Content-Language: en-US
To:     Lorenzo Stoakes <lstoakes@gmail.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        "Liam R. Howlett" <Liam.Howlett@oracle.com>,
        Matthew Wilcox <willy@infradead.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, patches@lists.linux.dev,
        maple-tree@lists.infradead.org
References: <20230309111258.24079-1-vbabka@suse.cz>
 <20230309111258.24079-11-vbabka@suse.cz>
 <7a9ca4a6-9713-4e31-9c0f-11ec31817c7a@lucifer.local>
From:   Vlastimil Babka <vbabka@suse.cz>
In-Reply-To: <7a9ca4a6-9713-4e31-9c0f-11ec31817c7a@lucifer.local>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/15/23 23:20, Lorenzo Stoakes wrote:
> On Thu, Mar 09, 2023 at 12:12:58PM +0100, Vlastimil Babka wrote:
>> This effectively reverts d014cd7c1c35 ("mm, mremap: fix mremap()
>> expanding for vma's with vm_ops->close()"). After the recent changes,
>> vma_merge() is able to handle the expansion properly even when the vma
>> being expanded has a vm_ops->close operation, so we don't need to
>> special case it anymore.
>>
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>> ---
>>  mm/mremap.c | 20 ++++----------------
>>  1 file changed, 4 insertions(+), 16 deletions(-)
>>
>> diff --git a/mm/mremap.c b/mm/mremap.c
>> index 411a85682b58..65f5b545601e 100644
>> --- a/mm/mremap.c
>> +++ b/mm/mremap.c
>> @@ -1040,23 +1040,11 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
>>  			 * vma (expand operation itself) and possibly also with
>>  			 * the next vma if it becomes adjacent to the expanded
>>  			 * vma and  otherwise compatible.
>> -			 *
>> -			 * However, vma_merge() can currently fail due to
>> -			 * is_mergeable_vma() check for vm_ops->close (see the
>> -			 * comment there). Yet this should not prevent vma
>> -			 * expanding, so perform a simple expand for such vma.
>> -			 * Ideally the check for close op should be only done
>> -			 * when a vma would be actually removed due to a merge.
>>  			 */
>> -			if (!vma->vm_ops || !vma->vm_ops->close) {
>> -				vma = vma_merge(&vmi, mm, vma, extension_start,
>> -					extension_end, vma->vm_flags, vma->anon_vma,
>> -					vma->vm_file, extension_pgoff, vma_policy(vma),
>> -					vma->vm_userfaultfd_ctx, anon_vma_name(vma));
>> -			} else if (vma_expand(&vmi, vma, vma->vm_start,
>> -					addr + new_len, vma->vm_pgoff, NULL)) {
>> -				vma = NULL;
>> -			}
>> +			vma = vma_merge(&vmi, mm, vma, extension_start,
>> +				extension_end, vma->vm_flags, vma->anon_vma,
>> +				vma->vm_file, extension_pgoff, vma_policy(vma),
>> +				vma->vm_userfaultfd_ctx, anon_vma_name(vma));
>>  			if (!vma) {
>>  				vm_unacct_memory(pages);
>>  				ret = -ENOMEM;
>> --
>> 2.39.2
>>
> 
> Good to eliminate this edge case! Do we have a self-test for this case to assert
> that the issue is fixed by this? I guess a little tricky due to the need for the
> the owning VMA to have ->close() specified.

Yeah that's the problem, it needs some specific setup, unlike the existing
tests.

> In any case, the changes you have made in the previous patch should ensure the
> edge case is no longer required, hence:-
> 
> Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com>

Thanks!