linux-kernel - Re: [PATCH v2] selftest/vm: add mremap expand merge offset test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <54da9c76-a506-0acd-3f97-60a0cce87c8a@redhat.com>
Date:   Mon, 2 Jan 2023 16:53:20 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Lorenzo Stoakes <lstoakes@...il.com>
Cc:     linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org, Vlastimil Babka <vbabka@...e.cz>,
        Jakub Matena <matenajakub@...il.com>,
        Matthew Wilcox <willy@...radead.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Michal Hocko <mhocko@...nel.org>
Subject: Re: [PATCH v2] selftest/vm: add mremap expand merge offset test

On 02.01.23 16:49, Lorenzo Stoakes wrote:
> On Mon, Jan 02, 2023 at 04:34:14PM +0100, David Hildenbrand wrote:
>> On 02.01.23 15:44, Lorenzo Stoakes wrote:
>>> Add a test to assert that we can mremap() and expand a mapping starting
>>> from an offset within an existing mapping. We unmap the last page in a 3
>>> page mapping to ensure that the remap should always succeed, before
>>> remapping from the 2nd page.
>>>
>>> This is additionally a regression test for the issue solved in "mm, mremap:
>>> fix mremap() expanding vma with addr inside vma" and confirmed to fail
>>> prior to the change and pass after it.
>>>
>>> Finally, this patch updates the existing mremap expand merge test to check
>>> error conditions and reduce code duplication between the two tests.
>>>
>>> Signed-off-by: Lorenzo Stoakes <lstoakes@...il.com>
>>> ---
>>>    tools/testing/selftests/vm/mremap_test.c | 115 ++++++++++++++++++-----
>>>    1 file changed, 93 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
>>
>>
>> ...
>>
>>> +
>>> +	start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
>>> +		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>>> +
>>> +	if (start == MAP_FAILED) {
>>> +		ksft_print_msg("mmap failed: %s\n", strerror(errno));
>>
>> I'd
>>
>> 	ksft_test_result_fail(...)
>> 	return;
>>
>>> +		goto out;
>>> +	}
>>> +
>>> +	munmap(start + page_size, page_size);
>>> +	remap = mremap(start, page_size, 2 * page_size, 0);
>>> +	if (remap == MAP_FAILED) {
>>> +		ksft_print_msg("mremap failed: %s\n", strerror(errno));
>>> +		munmap(start, page_size);
>>> +		munmap(start + 2 * page_size, page_size);
>>> +		goto out;
>>
>> dito
>>
>> 	ksft_test_result_fail(...)
>> 	...
>> 	return;
>>
>>> +	}
>>> +
>>> +	success = is_range_mapped(maps_fp, start, start + 3 * page_size);
>>> +	munmap(start, 3 * page_size);
>>> +
>>> +out:
>>
>> then you can drop the out label.
>>
> 
> I have to disagree on this, to be consistent with the other tests the
> failure messages should include the test name, and putting the
> ksft_test_result_fail("test name") in each branch as well as the error
> message would just be wilful duplication.
> 
> I do think it's a pity C lacks mechanisms such that gotos are sometimes
> necessary, but I can only right so many wrongs in this patch :)
> 

Let's agree to disagree ;) Too bad we don't have prefix push/pop 
functionality as we have in other similar testing frameworks -- to avoid 
exactly that duplication.

[...]

>> I'd simply use a global variable, same applies for page_size. But passing it
>> around is also ok.
>>
> 
> I am trying to keep things consistent with what's gone before in this code,
> and given page_size is being passed around I think the 'when in Rome'
> principle applies equally to passing the fp around I think.

Other tests we have handle it "easier". :)

-- 
Thanks,

David / dhildenb