diff mbox series

[v3,2/3] selftests: soft-dirty: Add test for mprotect

Message ID 20220721183338.27871-3-peterx@redhat.com (mailing list archive)
State New
Headers show
Series mm/mprotect: Fix soft-dirty checks | expand

Commit Message

Peter Xu July 21, 2022, 6:33 p.m. UTC 
Add two soft-diryt test cases for mprotect() on both anon or file.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

Comments

David Hildenbrand July 22, 2022, 7:17 a.m. UTC | #1 
On 21.07.22 20:33, Peter Xu wrote:
> Add two soft-diryt test cases for mprotect() on both anon or file.

s/soft-diryt/soft-dirty/

> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++++-
>  1 file changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/vm/soft-dirty.c b/tools/testing/selftests/vm/soft-dirty.c
> index 08ab62a4a9d0..7d93906aa43f 100644
> --- a/tools/testing/selftests/vm/soft-dirty.c
> +++ b/tools/testing/selftests/vm/soft-dirty.c
> @@ -121,13 +121,78 @@ static void test_hugepage(int pagemap_fd, int pagesize)
>  	free(map);
>  }
>  
> +static void test_mprotect(int pagemap_fd, int pagesize, bool anon)
> +{
> +	const char *type[] = {"file", "anon"};
> +	const char *fname = "./soft-dirty-test-file";
> +	int test_fd;
> +	char *map;

Instead of fname, unlink, open, close, unlink  you can use a tmpfile

FILE *file;

file = tmpfile();
if (!file) {
	ksft_test_result_fail("tmpfile() failed\n");
	return;
}
test_fd = fileno(file);

> +
> +	if (anon) {
> +		map = mmap(NULL, pagesize, PROT_READ|PROT_WRITE,
> +			   MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> +		if (!map)
> +			ksft_exit_fail_msg("anon mmap failed\n");
> +	} else {
> +		unlink(fname);
> +		test_fd = open(fname, O_RDWR | O_CREAT);
> +		if (test_fd < 0) {
> +			ksft_test_result_skip("Test %s huge page allocation\n", __func__);

Wrong copy-paste I assume :)

> +			return;
> +		}
> +		ftruncate(test_fd, pagesize);
> +		map = mmap(NULL, pagesize, PROT_READ|PROT_WRITE,
> +			   MAP_SHARED, test_fd, 0);
> +		if (!map)
> +			ksft_exit_fail_msg("file mmap failed\n");
> +	}
> +

Apart from that LGTM.
Peter Xu July 22, 2022, 1:44 p.m. UTC | #2 
On Fri, Jul 22, 2022 at 09:17:34AM +0200, David Hildenbrand wrote:
> On 21.07.22 20:33, Peter Xu wrote:
> > Add two soft-diryt test cases for mprotect() on both anon or file.
> 
> s/soft-diryt/soft-dirty/

Fixed.

> 
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++++-
> >  1 file changed, 68 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/vm/soft-dirty.c b/tools/testing/selftests/vm/soft-dirty.c
> > index 08ab62a4a9d0..7d93906aa43f 100644
> > --- a/tools/testing/selftests/vm/soft-dirty.c
> > +++ b/tools/testing/selftests/vm/soft-dirty.c
> > @@ -121,13 +121,78 @@ static void test_hugepage(int pagemap_fd, int pagesize)
> >  	free(map);
> >  }
> >  
> > +static void test_mprotect(int pagemap_fd, int pagesize, bool anon)
> > +{
> > +	const char *type[] = {"file", "anon"};
> > +	const char *fname = "./soft-dirty-test-file";
> > +	int test_fd;
> > +	char *map;
> 
> Instead of fname, unlink, open, close, unlink  you can use a tmpfile
> 
> FILE *file;
> 
> file = tmpfile();
> if (!file) {
> 	ksft_test_result_fail("tmpfile() failed\n");
> 	return;
> }
> test_fd = fileno(file);

Note that tmpfile() should by default fetch from /tmp which is very
possibly a tmpfs afaict.  It's tricky in this special test case since I
don't think tmpfs can trigger this bug (shmem doesn't define page_mkwrite).

I wanted to create under this dir to make the best possible bet to trigger
the bug. E.g. major fs will works like xfs, btrfs, extN.  It'll stop work
if someone clones the Linux repo under tmpfs but it's rare.

> 
> > +
> > +	if (anon) {
> > +		map = mmap(NULL, pagesize, PROT_READ|PROT_WRITE,
> > +			   MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> > +		if (!map)
> > +			ksft_exit_fail_msg("anon mmap failed\n");
> > +	} else {
> > +		unlink(fname);
> > +		test_fd = open(fname, O_RDWR | O_CREAT);
> > +		if (test_fd < 0) {
> > +			ksft_test_result_skip("Test %s huge page allocation\n", __func__);
> 
> Wrong copy-paste I assume :)

Yeh :) I'll fix it.

> 
> > +			return;
> > +		}
> > +		ftruncate(test_fd, pagesize);
> > +		map = mmap(NULL, pagesize, PROT_READ|PROT_WRITE,
> > +			   MAP_SHARED, test_fd, 0);
> > +		if (!map)
> > +			ksft_exit_fail_msg("file mmap failed\n");
> > +	}
> > +
> 
> Apart from that LGTM.

Thanks,
David Hildenbrand July 22, 2022, 2 p.m. UTC | #3 
On 22.07.22 15:44, Peter Xu wrote:
> On Fri, Jul 22, 2022 at 09:17:34AM +0200, David Hildenbrand wrote:
>> On 21.07.22 20:33, Peter Xu wrote:
>>> Add two soft-diryt test cases for mprotect() on both anon or file.
>>
>> s/soft-diryt/soft-dirty/
> 
> Fixed.
> 
>>
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>  tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++++-
>>>  1 file changed, 68 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/vm/soft-dirty.c b/tools/testing/selftests/vm/soft-dirty.c
>>> index 08ab62a4a9d0..7d93906aa43f 100644
>>> --- a/tools/testing/selftests/vm/soft-dirty.c
>>> +++ b/tools/testing/selftests/vm/soft-dirty.c
>>> @@ -121,13 +121,78 @@ static void test_hugepage(int pagemap_fd, int pagesize)
>>>  	free(map);
>>>  }
>>>  
>>> +static void test_mprotect(int pagemap_fd, int pagesize, bool anon)
>>> +{
>>> +	const char *type[] = {"file", "anon"};
>>> +	const char *fname = "./soft-dirty-test-file";
>>> +	int test_fd;
>>> +	char *map;
>>
>> Instead of fname, unlink, open, close, unlink  you can use a tmpfile
>>
>> FILE *file;
>>
>> file = tmpfile();
>> if (!file) {
>> 	ksft_test_result_fail("tmpfile() failed\n");
>> 	return;
>> }
>> test_fd = fileno(file);
> 
> Note that tmpfile() should by default fetch from /tmp which is very
> possibly a tmpfs afaict.  It's tricky in this special test case since I
> don't think tmpfs can trigger this bug (shmem doesn't define page_mkwrite).
> 

I don't think we need that? SOFTDIRTY tracking enabled should be
sufficient, or what am I missing?
David Hildenbrand July 22, 2022, 2:07 p.m. UTC | #4 
On 22.07.22 16:00, David Hildenbrand wrote:
> On 22.07.22 15:44, Peter Xu wrote:
>> On Fri, Jul 22, 2022 at 09:17:34AM +0200, David Hildenbrand wrote:
>>> On 21.07.22 20:33, Peter Xu wrote:
>>>> Add two soft-diryt test cases for mprotect() on both anon or file.
>>>
>>> s/soft-diryt/soft-dirty/
>>
>> Fixed.
>>
>>>
>>>>
>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>>> ---
>>>>  tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++++-
>>>>  1 file changed, 68 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/testing/selftests/vm/soft-dirty.c b/tools/testing/selftests/vm/soft-dirty.c
>>>> index 08ab62a4a9d0..7d93906aa43f 100644
>>>> --- a/tools/testing/selftests/vm/soft-dirty.c
>>>> +++ b/tools/testing/selftests/vm/soft-dirty.c
>>>> @@ -121,13 +121,78 @@ static void test_hugepage(int pagemap_fd, int pagesize)
>>>>  	free(map);
>>>>  }
>>>>  
>>>> +static void test_mprotect(int pagemap_fd, int pagesize, bool anon)
>>>> +{
>>>> +	const char *type[] = {"file", "anon"};
>>>> +	const char *fname = "./soft-dirty-test-file";
>>>> +	int test_fd;
>>>> +	char *map;
>>>
>>> Instead of fname, unlink, open, close, unlink  you can use a tmpfile
>>>
>>> FILE *file;
>>>
>>> file = tmpfile();
>>> if (!file) {
>>> 	ksft_test_result_fail("tmpfile() failed\n");
>>> 	return;
>>> }
>>> test_fd = fileno(file);
>>
>> Note that tmpfile() should by default fetch from /tmp which is very
>> possibly a tmpfs afaict.  It's tricky in this special test case since I
>> don't think tmpfs can trigger this bug (shmem doesn't define page_mkwrite).
>>
> 
> I don't think we need that? SOFTDIRTY tracking enabled should be
> sufficient, or what am I missing?
> 

I think you're right that it doesn't work with tmpfile. I do wonder why,
because I'd have thought that it's sufficient for
vma_wants_writenotify() to return "1" due to the
vma_soft_dirty_enabled() check.

Hm ....
Peter Xu July 22, 2022, 2:37 p.m. UTC | #5 
On Fri, Jul 22, 2022 at 04:07:47PM +0200, David Hildenbrand wrote:
> On 22.07.22 16:00, David Hildenbrand wrote:
> > On 22.07.22 15:44, Peter Xu wrote:
> >> On Fri, Jul 22, 2022 at 09:17:34AM +0200, David Hildenbrand wrote:
> >>> On 21.07.22 20:33, Peter Xu wrote:
> >>>> Add two soft-diryt test cases for mprotect() on both anon or file.
> >>>
> >>> s/soft-diryt/soft-dirty/
> >>
> >> Fixed.
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Peter Xu <peterx@redhat.com>
> >>>> ---
> >>>>  tools/testing/selftests/vm/soft-dirty.c | 69 ++++++++++++++++++++++++-
> >>>>  1 file changed, 68 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/tools/testing/selftests/vm/soft-dirty.c b/tools/testing/selftests/vm/soft-dirty.c
> >>>> index 08ab62a4a9d0..7d93906aa43f 100644
> >>>> --- a/tools/testing/selftests/vm/soft-dirty.c
> >>>> +++ b/tools/testing/selftests/vm/soft-dirty.c
> >>>> @@ -121,13 +121,78 @@ static void test_hugepage(int pagemap_fd, int pagesize)
> >>>>  	free(map);
> >>>>  }
> >>>>  
> >>>> +static void test_mprotect(int pagemap_fd, int pagesize, bool anon)
> >>>> +{
> >>>> +	const char *type[] = {"file", "anon"};
> >>>> +	const char *fname = "./soft-dirty-test-file";
> >>>> +	int test_fd;
> >>>> +	char *map;
> >>>
> >>> Instead of fname, unlink, open, close, unlink  you can use a tmpfile
> >>>
> >>> FILE *file;
> >>>
> >>> file = tmpfile();
> >>> if (!file) {
> >>> 	ksft_test_result_fail("tmpfile() failed\n");
> >>> 	return;
> >>> }
> >>> test_fd = fileno(file);
> >>
> >> Note that tmpfile() should by default fetch from /tmp which is very
> >> possibly a tmpfs afaict.  It's tricky in this special test case since I
> >> don't think tmpfs can trigger this bug (shmem doesn't define page_mkwrite).
> >>
> > 
> > I don't think we need that? SOFTDIRTY tracking enabled should be
> > sufficient, or what am I missing?
> > 
> 
> I think you're right that it doesn't work with tmpfile. I do wonder why,
> because I'd have thought that it's sufficient for
> vma_wants_writenotify() to return "1" due to the
> vma_soft_dirty_enabled() check.

I can't say I know the whole rational of this, but I think it's below that
will start to return 0 already before the soft-dirty check:

	if (pgprot_val(vm_page_prot) !=
	    pgprot_val(vm_pgprot_modify(vm_page_prot, vm_flags)))
		return 0;

in vma_wants_writenotify().
diff mbox series

Patch

diff --git a/tools/testing/selftests/vm/soft-dirty.c b/tools/testing/selftests/vm/soft-dirty.c
index 08ab62a4a9d0..7d93906aa43f 100644
--- a/tools/testing/selftests/vm/soft-dirty.c
+++ b/tools/testing/selftests/vm/soft-dirty.c
@@ -121,13 +121,78 @@  static void test_hugepage(int pagemap_fd, int pagesize)
 	free(map);
 }
 
+static void test_mprotect(int pagemap_fd, int pagesize, bool anon)
+{
+	const char *type[] = {"file", "anon"};
+	const char *fname = "./soft-dirty-test-file";
+	int test_fd;
+	char *map;
+
+	if (anon) {
+		map = mmap(NULL, pagesize, PROT_READ|PROT_WRITE,
+			   MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
+		if (!map)
+			ksft_exit_fail_msg("anon mmap failed\n");
+	} else {
+		unlink(fname);
+		test_fd = open(fname, O_RDWR | O_CREAT);
+		if (test_fd < 0) {
+			ksft_test_result_skip("Test %s huge page allocation\n", __func__);
+			return;
+		}
+		ftruncate(test_fd, pagesize);
+		map = mmap(NULL, pagesize, PROT_READ|PROT_WRITE,
+			   MAP_SHARED, test_fd, 0);
+		if (!map)
+			ksft_exit_fail_msg("file mmap failed\n");
+	}
+
+	*map = 1;
+	ksft_test_result(pagemap_is_softdirty(pagemap_fd, map) == 1,
+			 "Test %s-%s dirty bit of new written page\n",
+			 __func__, type[anon]);
+	clear_softdirty();
+	ksft_test_result(pagemap_is_softdirty(pagemap_fd, map) == 0,
+			 "Test %s-%s soft-dirty clear after clear_refs\n",
+			 __func__, type[anon]);
+	mprotect(map, pagesize, PROT_READ);
+	ksft_test_result(pagemap_is_softdirty(pagemap_fd, map) == 0,
+			 "Test %s-%s soft-dirty clear after marking RO\n",
+			 __func__, type[anon]);
+	mprotect(map, pagesize, PROT_READ|PROT_WRITE);
+	ksft_test_result(pagemap_is_softdirty(pagemap_fd, map) == 0,
+			 "Test %s-%s soft-dirty clear after marking RW\n",
+			 __func__, type[anon]);
+	*map = 2;
+	ksft_test_result(pagemap_is_softdirty(pagemap_fd, map) == 1,
+			 "Test %s-%s soft-dirty after rewritten\n",
+			 __func__, type[anon]);
+
+	munmap(map, pagesize);
+
+	if (!anon) {
+		close(test_fd);
+		unlink(fname);
+	}
+}
+
+static void test_mprotect_anon(int pagemap_fd, int pagesize)
+{
+	test_mprotect(pagemap_fd, pagesize, true);
+}
+
+static void test_mprotect_file(int pagemap_fd, int pagesize)
+{
+	test_mprotect(pagemap_fd, pagesize, false);
+}
+
 int main(int argc, char **argv)
 {
 	int pagemap_fd;
 	int pagesize;
 
 	ksft_print_header();
-	ksft_set_plan(5);
+	ksft_set_plan(15);
 
 	pagemap_fd = open(PAGEMAP_FILE_PATH, O_RDONLY);
 	if (pagemap_fd < 0)
@@ -138,6 +203,8 @@  int main(int argc, char **argv)
 	test_simple(pagemap_fd, pagesize);
 	test_vma_reuse(pagemap_fd, pagesize);
 	test_hugepage(pagemap_fd, pagesize);
+	test_mprotect_anon(pagemap_fd, pagesize);
+	test_mprotect_file(pagemap_fd, pagesize);
 
 	close(pagemap_fd);