[LTP] [PATCH 1/1] mremap06: Add mremap() reproducer for 7e7757876f25

[LTP] [PATCH 1/1] mremap06: Add mremap() reproducer for 7e7757876f25

[Petr Vorel]

Affected system reports:

mremap06.c:69: TINFO: all pages with compatible mapping
mremap06.c:90: TPASS: mmap/mremap work properly
mremap06.c:69: TINFO: third page's mapping incompatible
mremap06.c:90: TPASS: mmap/mremap work properly
mremap06.c:69: TINFO: first page's mapping incompatible
mremap06.c:56: TFAIL: page 1 wrong value 2 (0x32)
mremap06.c:56: TFAIL: page 2 wrong value 3 (0x33)

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
@Cyril: I plan to add SAFE_FALLOCATE() (3 other sources),
SAFE_MPROTECT() (7 other sources) and SAFE_MREMAP()
(2 other sources), but as a separate effort.

Kind regards,
Petr

 runtest/syscalls                            |   1 +
 testcases/kernel/syscalls/mremap/.gitignore |   1 +
 testcases/kernel/syscalls/mremap/mremap06.c | 129 ++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 testcases/kernel/syscalls/mremap/mremap06.c

diff --git a/runtest/syscalls b/runtest/syscalls
index b29151186..008bca508 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -845,6 +845,7 @@ mremap02 mremap02
 mremap03 mremap03
 mremap04 mremap04
 mremap05 mremap05
+mremap06 mremap06
 
 msgctl01 msgctl01
 msgctl02 msgctl02
diff --git a/testcases/kernel/syscalls/mremap/.gitignore b/testcases/kernel/syscalls/mremap/.gitignore
index 833e1b883..ec15a19cd 100644
--- a/testcases/kernel/syscalls/mremap/.gitignore
+++ b/testcases/kernel/syscalls/mremap/.gitignore
@@ -3,3 +3,4 @@
 /mremap03
 /mremap04
 /mremap05
+/mremap06
diff --git a/testcases/kernel/syscalls/mremap/mremap06.c b/testcases/kernel/syscalls/mremap/mremap06.c
new file mode 100644
index 000000000..b7aa5549f
--- /dev/null
+++ b/testcases/kernel/syscalls/mremap/mremap06.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 SUSE LLC
+ * Author: Vlastimil Babka <vbabka@suse.cz>
+ * LTP port: Petr Vorel <pvorel@suse.cz>
+ */
+
+/*\
+ * [Description]
+ *
+ * Bug reproducer for 7e7757876f25 ("mm/mremap: fix vm_pgoff in vma_merge() case 3")
+ */
+
+#define _GNU_SOURCE
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <fcntl.h>
+#include <sys/mman.h>
+
+#include "tst_test.h"
+#include "tst_safe_macros.h"
+
+#define PAGE_SIZE 4096
+#define MMAP_SIZE ((ARRAY_SIZE(tcases)+1)*PAGE_SIZE)
+#define MREMAP_SIZE (ARRAY_SIZE(tcases)*PAGE_SIZE)
+
+static int fd;
+static char *buf, *buf2;
+
+static struct tcase {
+	size_t incompatible;
+	const char *desc;
+} tcases[] = {
+	{
+		.desc = "all pages with compatible mapping",
+	},
+	{
+		.incompatible = 3,
+		.desc = "third page's mapping incompatible",
+	},
+	{
+		.incompatible = 1,
+		.desc = "first page's mapping incompatible",
+	},
+};
+
+static int check_pages(void)
+{
+	int fail = 0, i;
+	char val;
+
+	for (i = 0; i < (int)ARRAY_SIZE(tcases); i++) {
+		val = buf[i * PAGE_SIZE];
+		if (val != 0x30 + i) {
+			tst_res(TFAIL, "page %d wrong value %d (0x%x)", i, val - 0x30, val);
+			fail = 1;
+		}
+	}
+
+	return fail;
+}
+
+static void do_test(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
+	int ret;
+
+	tst_res(TINFO, "%s", tc->desc);
+
+	buf = SAFE_MMAP(0, MMAP_SIZE, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
+
+	buf2 = mremap(buf + PAGE_SIZE, PAGE_SIZE, PAGE_SIZE,
+			MREMAP_MAYMOVE|MREMAP_FIXED, buf + MREMAP_SIZE);
+	if (buf2 == MAP_FAILED)
+		tst_brk(TBROK, "mremap() failed");
+
+	if (tc->incompatible) {
+		ret = mprotect(buf + (tc->incompatible-1)*PAGE_SIZE, PAGE_SIZE, PROT_READ);
+		if (ret == -1)
+			tst_brk(TBROK, "mprotect() failed");
+	}
+
+	buf2 = mremap(buf + MREMAP_SIZE, PAGE_SIZE, PAGE_SIZE,
+			MREMAP_MAYMOVE|MREMAP_FIXED, buf + PAGE_SIZE);
+	if (buf2 == MAP_FAILED)
+		tst_brk(TBROK, "mremap() failed");
+
+	if (!check_pages())
+		tst_res(TPASS, "mmap/mremap work properly");
+
+	SAFE_MUNMAP(buf, MREMAP_SIZE);
+}
+
+static void setup(void)
+{
+	int ret, i;
+
+	fd = SAFE_OPEN("testfile", O_CREAT | O_RDWR | O_TRUNC, 0600);
+
+	ret = fallocate(fd, 0, 0, MMAP_SIZE);
+	if (ret == -1)
+		tst_brk(TBROK, "fallocate() failed");
+
+	buf = SAFE_MMAP(0, MMAP_SIZE, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
+
+	for (i = 0; i < (int)ARRAY_SIZE(tcases)+1; i++)
+		buf[i * PAGE_SIZE] = 0x30 + i;
+
+	/* clear the page tables */
+	SAFE_MUNMAP(buf, MMAP_SIZE);
+}
+
+static void cleanup(void)
+{
+	if (fd > 0)
+		SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
+	.test = do_test,
+	.tcnt = ARRAY_SIZE(tcases),
+	.tags = (struct tst_tag[]) {
+		{"linux-git", "7e7757876f25"},
+		{}
+	},
+};
-- 
2.40.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] mremap06: Add mremap() reproducer for 7e7757876f25

[Petr Vorel]

Hi all,

I forget to add link to the original bug report
https://bugzilla.suse.com/show_bug.cgi?id=1210903
(I'll add it to the commit message)

and the original reproducer (I'll add it to the sources).

Kind regards,
Petr

+++ testcases/kernel/syscalls/mremap/mremap06.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (c) 2023 SUSE LLC
  * Author: Vlastimil Babka <vbabka@suse.cz>
+ * https://bugzilla.suse.com/attachment.cgi?id=867254
  * LTP port: Petr Vorel <pvorel@suse.cz>
  */
 

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] mremap06: Add mremap() reproducer for 7e7757876f25

[Petr Vorel]

Hi,

[ drop kernel developers, add Jan, Li ]

> ---
> @Cyril: I plan to add SAFE_FALLOCATE() (3 other sources),
> SAFE_MPROTECT() (7 other sources) and SAFE_MREMAP()
> (2 other sources), but as a separate effort.

@Jan, you added in 9120d8a22 ("safe_macros: turn functions with off_t parameter
into static inline") note "following functions are inline because the behaviour
may depend on -D_FILE_OFFSET_BITS=64 -DOFF_T=__off64_t compile flags". IMHO the
only source which uses SAFE_MMAP() is testcases/kernel/mem/mmapstress/mmapstress01.c
I'm asking because I wonder if SAFE_MPROTECT() and SAFE_MREMAP() should be also
static inline, IMHO it's not needed.

@all: SAFE_MPROTECT() would be needed also on some still old API sources
(testcases/kernel/syscalls/signal/signal06.c,
testcases/kernel/syscalls/mprotect/mprotect02.c,
testcases/kernel/syscalls/mprotect/mprotect03.c)
Should I ignore that and add it just to new API?

Kind regards,
Petr

> Kind regards,
> Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] mremap06: Add mremap() reproducer for 7e7757876f25

[Cyril Hrubis]

Hi!
> mremap06.c:69: TINFO: all pages with compatible mapping
> mremap06.c:90: TPASS: mmap/mremap work properly
> mremap06.c:69: TINFO: third page's mapping incompatible
> mremap06.c:90: TPASS: mmap/mremap work properly
> mremap06.c:69: TINFO: first page's mapping incompatible
> mremap06.c:56: TFAIL: page 1 wrong value 2 (0x32)
> mremap06.c:56: TFAIL: page 2 wrong value 3 (0x33)
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> @Cyril: I plan to add SAFE_FALLOCATE() (3 other sources),
> SAFE_MPROTECT() (7 other sources) and SAFE_MREMAP()
> (2 other sources), but as a separate effort.
> 
> Kind regards,
> Petr
> 
>  runtest/syscalls                            |   1 +
>  testcases/kernel/syscalls/mremap/.gitignore |   1 +
>  testcases/kernel/syscalls/mremap/mremap06.c | 129 ++++++++++++++++++++
>  3 files changed, 131 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/mremap/mremap06.c
> 
> diff --git a/runtest/syscalls b/runtest/syscalls
> index b29151186..008bca508 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -845,6 +845,7 @@ mremap02 mremap02
>  mremap03 mremap03
>  mremap04 mremap04
>  mremap05 mremap05
> +mremap06 mremap06
>  
>  msgctl01 msgctl01
>  msgctl02 msgctl02
> diff --git a/testcases/kernel/syscalls/mremap/.gitignore b/testcases/kernel/syscalls/mremap/.gitignore
> index 833e1b883..ec15a19cd 100644
> --- a/testcases/kernel/syscalls/mremap/.gitignore
> +++ b/testcases/kernel/syscalls/mremap/.gitignore
> @@ -3,3 +3,4 @@
>  /mremap03
>  /mremap04
>  /mremap05
> +/mremap06
> diff --git a/testcases/kernel/syscalls/mremap/mremap06.c b/testcases/kernel/syscalls/mremap/mremap06.c
> new file mode 100644
> index 000000000..b7aa5549f
> --- /dev/null
> +++ b/testcases/kernel/syscalls/mremap/mremap06.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 SUSE LLC
> + * Author: Vlastimil Babka <vbabka@suse.cz>
> + * LTP port: Petr Vorel <pvorel@suse.cz>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Bug reproducer for 7e7757876f25 ("mm/mremap: fix vm_pgoff in vma_merge() case 3")
> + */
> +
> +#define _GNU_SOURCE
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <sys/mman.h>
> +
> +#include "tst_test.h"
> +#include "tst_safe_macros.h"
> +
> +#define PAGE_SIZE 4096

This wouldn't work on 64k page size.

> +#define MMAP_SIZE ((ARRAY_SIZE(tcases)+1)*PAGE_SIZE)
> +#define MREMAP_SIZE (ARRAY_SIZE(tcases)*PAGE_SIZE)

These have to be variables initialized on the fly with getpagesize()
instead of PAGE_SIZE.

> +static int fd;
> +static char *buf, *buf2;
> +
> +static struct tcase {
> +	size_t incompatible;
> +	const char *desc;
> +} tcases[] = {
> +	{
> +		.desc = "all pages with compatible mapping",
> +	},
> +	{
> +		.incompatible = 3,
> +		.desc = "third page's mapping incompatible",
> +	},
> +	{
> +		.incompatible = 1,
> +		.desc = "first page's mapping incompatible",
> +	},
> +};
> +
> +static int check_pages(void)
> +{
> +	int fail = 0, i;
> +	char val;
> +
> +	for (i = 0; i < (int)ARRAY_SIZE(tcases); i++) {
> +		val = buf[i * PAGE_SIZE];
> +		if (val != 0x30 + i) {
> +			tst_res(TFAIL, "page %d wrong value %d (0x%x)", i, val - 0x30, val);

Woudn't this generate too many FAILURE messages? Maybe we should just
break the for cycle here.

> +			fail = 1;
> +		}
> +	}
> +
> +	return fail;
> +}
> +
> +static void do_test(unsigned int n)
> +{
> +	struct tcase *tc = &tcases[n];
> +	int ret;
> +
> +	tst_res(TINFO, "%s", tc->desc);
> +
> +	buf = SAFE_MMAP(0, MMAP_SIZE, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> +
> +	buf2 = mremap(buf + PAGE_SIZE, PAGE_SIZE, PAGE_SIZE,
> +			MREMAP_MAYMOVE|MREMAP_FIXED, buf + MREMAP_SIZE);
> +	if (buf2 == MAP_FAILED)
> +		tst_brk(TBROK, "mremap() failed");
> +
> +	if (tc->incompatible) {
> +		ret = mprotect(buf + (tc->incompatible-1)*PAGE_SIZE, PAGE_SIZE, PROT_READ);
> +		if (ret == -1)
> +			tst_brk(TBROK, "mprotect() failed");
> +	}
> +
> +	buf2 = mremap(buf + MREMAP_SIZE, PAGE_SIZE, PAGE_SIZE,
> +			MREMAP_MAYMOVE|MREMAP_FIXED, buf + PAGE_SIZE);
> +	if (buf2 == MAP_FAILED)
> +		tst_brk(TBROK, "mremap() failed");
> +
> +	if (!check_pages())
> +		tst_res(TPASS, "mmap/mremap work properly");
> +
> +	SAFE_MUNMAP(buf, MREMAP_SIZE);
> +}
> +
> +static void setup(void)
> +{
> +	int ret, i;
> +
> +	fd = SAFE_OPEN("testfile", O_CREAT | O_RDWR | O_TRUNC, 0600);
> +
> +	ret = fallocate(fd, 0, 0, MMAP_SIZE);
> +	if (ret == -1)
> +		tst_brk(TBROK, "fallocate() failed");
> +
> +	buf = SAFE_MMAP(0, MMAP_SIZE, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> +
> +	for (i = 0; i < (int)ARRAY_SIZE(tcases)+1; i++)
> +		buf[i * PAGE_SIZE] = 0x30 + i;
> +
> +	/* clear the page tables */
> +	SAFE_MUNMAP(buf, MMAP_SIZE);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (fd > 0)
> +		SAFE_CLOSE(fd);
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test = do_test,
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.tags = (struct tst_tag[]) {
> +		{"linux-git", "7e7757876f25"},
> +		{}
> +	},
> +};
> -- 
> 2.40.1
> 

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] mremap06: Add mremap() reproducer for 7e7757876f25

[Cyril Hrubis]

Hi!
> > @Cyril: I plan to add SAFE_FALLOCATE() (3 other sources),
> > SAFE_MPROTECT() (7 other sources) and SAFE_MREMAP()
> > (2 other sources), but as a separate effort.
> 
> @Jan, you added in 9120d8a22 ("safe_macros: turn functions with off_t parameter
> into static inline") note "following functions are inline because the behaviour
> may depend on -D_FILE_OFFSET_BITS=64 -DOFF_T=__off64_t compile flags". IMHO the
> only source which uses SAFE_MMAP() is testcases/kernel/mem/mmapstress/mmapstress01.c
> I'm asking because I wonder if SAFE_MPROTECT() and SAFE_MREMAP() should be also
> static inline, IMHO it's not needed.

As long as the return value or function parameters does not include
off_t it's not needed.

> @all: SAFE_MPROTECT() would be needed also on some still old API sources
> (testcases/kernel/syscalls/signal/signal06.c,
> testcases/kernel/syscalls/mprotect/mprotect02.c,
> testcases/kernel/syscalls/mprotect/mprotect03.c)
> Should I ignore that and add it just to new API?

Just add it to the new API, the old test should be cleaned up and
converted anyways.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] mremap06: Add mremap() reproducer for 7e7757876f25

[Jan Stancek]

On Mon, Jul 3, 2023 at 10:07 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi,
>
> [ drop kernel developers, add Jan, Li ]
>
> > ---
> > @Cyril: I plan to add SAFE_FALLOCATE() (3 other sources),
> > SAFE_MPROTECT() (7 other sources) and SAFE_MREMAP()
> > (2 other sources), but as a separate effort.
>
> @Jan, you added in 9120d8a22 ("safe_macros: turn functions with off_t parameter
> into static inline") note "following functions are inline because the behaviour
> may depend on -D_FILE_OFFSET_BITS=64 -DOFF_T=__off64_t compile flags". IMHO the
> only source which uses SAFE_MMAP() is testcases/kernel/mem/mmapstress/mmapstress01.c

sendfile04.c potentially too, it's also compiled with 64bit offsets,
though offset is always 0

> I'm asking because I wonder if SAFE_MPROTECT() and SAFE_MREMAP() should be also
> static inline, IMHO it's not needed.

Doesn't appear to be needed here.

>
> @all: SAFE_MPROTECT() would be needed also on some still old API sources
> (testcases/kernel/syscalls/signal/signal06.c,
> testcases/kernel/syscalls/mprotect/mprotect02.c,
> testcases/kernel/syscalls/mprotect/mprotect03.c)
> Should I ignore that and add it just to new API?
>
> Kind regards,
> Petr
>
> > Kind regards,
> > Petr
>


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] mremap06: Add mremap() reproducer for 7e7757876f25

[Petr Vorel]

> On Mon, Jul 3, 2023 at 10:07 PM Petr Vorel <pvorel@suse.cz> wrote:

> > Hi,

> > [ drop kernel developers, add Jan, Li ]

> > > ---
> > > @Cyril: I plan to add SAFE_FALLOCATE() (3 other sources),
> > > SAFE_MPROTECT() (7 other sources) and SAFE_MREMAP()
> > > (2 other sources), but as a separate effort.

> > @Jan, you added in 9120d8a22 ("safe_macros: turn functions with off_t parameter
> > into static inline") note "following functions are inline because the behaviour
> > may depend on -D_FILE_OFFSET_BITS=64 -DOFF_T=__off64_t compile flags". IMHO the
> > only source which uses SAFE_MMAP() is testcases/kernel/mem/mmapstress/mmapstress01.c

> sendfile04.c potentially too, it's also compiled with 64bit offsets,
> though offset is always 0

> > I'm asking because I wonder if SAFE_MPROTECT() and SAFE_MREMAP() should be also
> > static inline, IMHO it's not needed.

> Doesn't appear to be needed here.

Hi Jan,

great, thanks for info.

Kind regards,
Petr


> > @all: SAFE_MPROTECT() would be needed also on some still old API sources
> > (testcases/kernel/syscalls/signal/signal06.c,
> > testcases/kernel/syscalls/mprotect/mprotect02.c,
> > testcases/kernel/syscalls/mprotect/mprotect03.c)
> > Should I ignore that and add it just to new API?

> > Kind regards,
> > Petr

> > > Kind regards,
> > > Petr



-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] mremap06: Add mremap() reproducer for 7e7757876f25

[Petr Vorel]

> Hi!
> > > @Cyril: I plan to add SAFE_FALLOCATE() (3 other sources),
> > > SAFE_MPROTECT() (7 other sources) and SAFE_MREMAP()
> > > (2 other sources), but as a separate effort.

> > @Jan, you added in 9120d8a22 ("safe_macros: turn functions with off_t parameter
> > into static inline") note "following functions are inline because the behaviour
> > may depend on -D_FILE_OFFSET_BITS=64 -DOFF_T=__off64_t compile flags". IMHO the
> > only source which uses SAFE_MMAP() is testcases/kernel/mem/mmapstress/mmapstress01.c
> > I'm asking because I wonder if SAFE_MPROTECT() and SAFE_MREMAP() should be also
> > static inline, IMHO it's not needed.

> As long as the return value or function parameters does not include
> off_t it's not needed.

+1

> > @all: SAFE_MPROTECT() would be needed also on some still old API sources
> > (testcases/kernel/syscalls/signal/signal06.c,
> > testcases/kernel/syscalls/mprotect/mprotect02.c,
> > testcases/kernel/syscalls/mprotect/mprotect03.c)
> > Should I ignore that and add it just to new API?

> Just add it to the new API, the old test should be cleaned up and
> converted anyways.

+1

Thanks,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] mremap06: Add mremap() reproducer for 7e7757876f25

[Petr Vorel]

Hi Cyril,

> > +#define PAGE_SIZE 4096

> This wouldn't work on 64k page size.

> > +#define MMAP_SIZE ((ARRAY_SIZE(tcases)+1)*PAGE_SIZE)
> > +#define MREMAP_SIZE (ARRAY_SIZE(tcases)*PAGE_SIZE)

> These have to be variables initialized on the fly with getpagesize()
> instead of PAGE_SIZE.

Yep, I thought about getpagesize().

...
> > +static int check_pages(void)
> > +{
> > +	int fail = 0, i;
> > +	char val;
> > +
> > +	for (i = 0; i < (int)ARRAY_SIZE(tcases); i++) {
> > +		val = buf[i * PAGE_SIZE];
> > +		if (val != 0x30 + i) {
> > +			tst_res(TFAIL, "page %d wrong value %d (0x%x)", i, val - 0x30, val);

> Woudn't this generate too many FAILURE messages? Maybe we should just
> break the for cycle here.

It could be. I wasn't sure if it's important to know which pages were wrong.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] mremap06: Add mremap() reproducer for 7e7757876f25

[Vlastimil Babka]

On 7/4/23 13:46, Petr Vorel wrote:
> Hi Cyril,
> 
>> > +#define PAGE_SIZE 4096
> 
>> This wouldn't work on 64k page size.
> 
>> > +#define MMAP_SIZE ((ARRAY_SIZE(tcases)+1)*PAGE_SIZE)
>> > +#define MREMAP_SIZE (ARRAY_SIZE(tcases)*PAGE_SIZE)
> 
>> These have to be variables initialized on the fly with getpagesize()
>> instead of PAGE_SIZE.
> 
> Yep, I thought about getpagesize().

Agreed.

Also IMHO it's awkward to have the number of pages and the iterations
expressed using ARRAY_SIZE(tcases). I'd say it's just an accident that the
number of pages involved in the merging and the number of testcases is both
3. I'd rather separate those.

> ...
>> > +static int check_pages(void)
>> > +{
>> > +	int fail = 0, i;
>> > +	char val;
>> > +
>> > +	for (i = 0; i < (int)ARRAY_SIZE(tcases); i++) {
>> > +		val = buf[i * PAGE_SIZE];
>> > +		if (val != 0x30 + i) {
>> > +			tst_res(TFAIL, "page %d wrong value %d (0x%x)", i, val - 0x30, val);
> 
>> Woudn't this generate too many FAILURE messages? Maybe we should just
>> break the for cycle here.
> 
> It could be. I wasn't sure if it's important to know which pages were wrong.

There's just 3 pages to print so it won't be that many messages, I'd just
print them all.

> Kind regards,
> Petr


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/1] mremap06: Add mremap() reproducer for 7e7757876f25

[Petr Vorel]

> On 7/4/23 13:46, Petr Vorel wrote:
> > Hi Cyril,

> >> > +#define PAGE_SIZE 4096

> >> This wouldn't work on 64k page size.

> >> > +#define MMAP_SIZE ((ARRAY_SIZE(tcases)+1)*PAGE_SIZE)
> >> > +#define MREMAP_SIZE (ARRAY_SIZE(tcases)*PAGE_SIZE)

> >> These have to be variables initialized on the fly with getpagesize()
> >> instead of PAGE_SIZE.

> > Yep, I thought about getpagesize().

> Agreed.

> Also IMHO it's awkward to have the number of pages and the iterations
> expressed using ARRAY_SIZE(tcases). I'd say it's just an accident that the
> number of pages involved in the merging and the number of testcases is both
> 3. I'd rather separate those.

Thanks for info. I'll define NUM_PAGES 3 then.

> > ...
> >> > +static int check_pages(void)
> >> > +{
> >> > +	int fail = 0, i;
> >> > +	char val;
> >> > +
> >> > +	for (i = 0; i < (int)ARRAY_SIZE(tcases); i++) {
> >> > +		val = buf[i * PAGE_SIZE];
> >> > +		if (val != 0x30 + i) {
> >> > +			tst_res(TFAIL, "page %d wrong value %d (0x%x)", i, val - 0x30, val);

> >> Woudn't this generate too many FAILURE messages? Maybe we should just
> >> break the for cycle here.

> > It could be. I wasn't sure if it's important to know which pages were wrong.

> There's just 3 pages to print so it won't be that many messages, I'd just
> print them all.

+1

Kind regards,
Petr

> > Kind regards,
> > Petr


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp