[LTP] [PATCH v3] userfaultfd: Minor fixes

[LTP] [PATCH v3] userfaultfd: Minor fixes

[Ricardo Branco]

- Use POSIX semantics for thread function
- Set cleanup to call reset_pages

Signed-off-by: Ricardo Branco <rbranco@suse.de>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
 .../kernel/syscalls/userfaultfd/userfaultfd01.c   | 13 ++++++++++---
 .../kernel/syscalls/userfaultfd/userfaultfd02.c   | 13 ++++++++++---
 .../kernel/syscalls/userfaultfd/userfaultfd03.c   | 15 +++++++++++----
 .../kernel/syscalls/userfaultfd/userfaultfd04.c   |  8 ++++++--
 .../kernel/syscalls/userfaultfd/userfaultfd05.c   | 12 ++++++++----
 .../kernel/syscalls/userfaultfd/userfaultfd06.c   |  4 ++--
 6 files changed, 47 insertions(+), 18 deletions(-)

diff --git a/testcases/kernel/syscalls/userfaultfd/userfaultfd01.c b/testcases/kernel/syscalls/userfaultfd/userfaultfd01.c
index 7368d3863..c3a37370e 100644
--- a/testcases/kernel/syscalls/userfaultfd/userfaultfd01.c
+++ b/testcases/kernel/syscalls/userfaultfd/userfaultfd01.c
@@ -53,11 +53,17 @@ static void set_pages(void)
 
 static void reset_pages(void)
 {
-	SAFE_MUNMAP(page, page_size);
-	SAFE_MUNMAP(copy_page, page_size);
+	if (page) {
+		SAFE_MUNMAP(page, page_size);
+		page = NULL;
+	}
+	if (copy_page) {
+		SAFE_MUNMAP(copy_page, page_size);
+		copy_page = NULL;
+	}
 }
 
-static void *handle_thread(void)
+static void *handle_thread(void *arg LTP_ATTRIBUTE_UNUSED)
 {
 	static struct uffd_msg msg;
 	struct uffdio_copy uffdio_copy = {};
@@ -129,4 +135,5 @@ static struct tst_test test = {
 	.setup = setup,
 	.test = run,
 	.tcnt = ARRAY_SIZE(tcases),
+	.cleanup = reset_pages,
 };
diff --git a/testcases/kernel/syscalls/userfaultfd/userfaultfd02.c b/testcases/kernel/syscalls/userfaultfd/userfaultfd02.c
index 2fd5ba5d8..7450c443b 100644
--- a/testcases/kernel/syscalls/userfaultfd/userfaultfd02.c
+++ b/testcases/kernel/syscalls/userfaultfd/userfaultfd02.c
@@ -32,11 +32,17 @@ static void set_pages(void)
 
 static void reset_pages(void)
 {
-	SAFE_MUNMAP(page, page_size);
-	SAFE_MUNMAP(move_page, page_size);
+	if (page) {
+		SAFE_MUNMAP(page, page_size);
+		page = NULL;
+	}
+	if (move_page) {
+		SAFE_MUNMAP(move_page, page_size);
+		move_page = NULL;
+	}
 }
 
-static void *handle_thread(void)
+static void *handle_thread(void *arg LTP_ATTRIBUTE_UNUSED)
 {
 	static struct uffd_msg msg;
 	struct uffdio_move uffdio_move = {};
@@ -94,4 +100,5 @@ static void run(void)
 static struct tst_test test = {
 	.test_all = run,
 	.min_kver = "6.8",
+	.cleanup = reset_pages,
 };
diff --git a/testcases/kernel/syscalls/userfaultfd/userfaultfd03.c b/testcases/kernel/syscalls/userfaultfd/userfaultfd03.c
index b65f39eca..727e551f6 100644
--- a/testcases/kernel/syscalls/userfaultfd/userfaultfd03.c
+++ b/testcases/kernel/syscalls/userfaultfd/userfaultfd03.c
@@ -57,11 +57,17 @@ static void set_pages(void)
 
 static void reset_pages(void)
 {
-	SAFE_MUNMAP(page, page_size);
-	SAFE_MUNMAP(copy_page, page_size);
+	if (page) {
+		SAFE_MUNMAP(page, page_size);
+		page = NULL;
+	}
+	if (copy_page) {
+		SAFE_MUNMAP(copy_page, page_size);
+		copy_page = NULL;
+	}
 }
 
-static void *handle_thread(void)
+static void *handle_thread(void *arg LTP_ATTRIBUTE_UNUSED)
 {
 	static struct uffd_msg msg;
 	struct uffdio_copy uffdio_copy = {};
@@ -132,5 +138,6 @@ static struct tst_test test = {
 	.needs_kconfigs = (const char *[]) {
 		"CONFIG_USERFAULTFD=y",
 		NULL
-	}
+	},
+	.cleanup = reset_pages,
 };
diff --git a/testcases/kernel/syscalls/userfaultfd/userfaultfd04.c b/testcases/kernel/syscalls/userfaultfd/userfaultfd04.c
index 4eb811e45..3273d20cb 100644
--- a/testcases/kernel/syscalls/userfaultfd/userfaultfd04.c
+++ b/testcases/kernel/syscalls/userfaultfd/userfaultfd04.c
@@ -30,10 +30,13 @@ static void set_pages(void)
 
 static void reset_pages(void)
 {
-	SAFE_MUNMAP(page, page_size);
+	if (page) {
+		SAFE_MUNMAP(page, page_size);
+		page = NULL;
+	}
 }
 
-static void *handle_thread(void)
+static void *handle_thread(void *arg LTP_ATTRIBUTE_UNUSED)
 {
 	static struct uffd_msg msg;
 	struct uffdio_zeropage uffdio_zeropage = {};
@@ -98,4 +101,5 @@ static void run(void)
 
 static struct tst_test test = {
 	.test_all = run,
+	.cleanup = reset_pages,
 };
diff --git a/testcases/kernel/syscalls/userfaultfd/userfaultfd05.c b/testcases/kernel/syscalls/userfaultfd/userfaultfd05.c
index e25a227cf..eadb2f8c3 100644
--- a/testcases/kernel/syscalls/userfaultfd/userfaultfd05.c
+++ b/testcases/kernel/syscalls/userfaultfd/userfaultfd05.c
@@ -33,10 +33,13 @@ static void set_pages(void)
 
 static void reset_pages(void)
 {
-	SAFE_MUNMAP(page, page_size);
+	if (page) {
+		SAFE_MUNMAP(page, page_size);
+		page = NULL;
+	}
 }
 
-static void *handle_thread(void)
+static void *handle_thread(void *arg LTP_ATTRIBUTE_UNUSED)
 {
 	static struct uffd_msg msg;
 	struct uffdio_writeprotect uffdio_writeprotect = {};
@@ -53,7 +56,7 @@ static void *handle_thread(void)
 	SAFE_READ(1, uffd, &msg, sizeof(msg));
 
 	if (msg.event != UFFD_EVENT_PAGEFAULT)
-		tst_brk(TFAIL, "Received unexpected UFFD_EVENT %d", msg.event);
+		tst_brk(TFAIL | TERRNO, "Received unexpected UFFD_EVENT %d", msg.event);
 
 	if (!(msg.arg.pagefault.flags & UFFD_PAGEFAULT_FLAG_WP) ||
 	    !(msg.arg.pagefault.flags & UFFD_PAGEFAULT_FLAG_WRITE)) {
@@ -127,5 +130,6 @@ static struct tst_test test = {
 	.needs_kconfigs = (const char *[]) {
 		"CONFIG_HAVE_ARCH_USERFAULTFD_WP=y",
 		NULL
-	}
+	},
+	.cleanup = reset_pages,
 };
diff --git a/testcases/kernel/syscalls/userfaultfd/userfaultfd06.c b/testcases/kernel/syscalls/userfaultfd/userfaultfd06.c
index 5b1252c35..bc12bc5f2 100644
--- a/testcases/kernel/syscalls/userfaultfd/userfaultfd06.c
+++ b/testcases/kernel/syscalls/userfaultfd/userfaultfd06.c
@@ -58,7 +58,7 @@ static void reset_pages(void)
 	}
 }
 
-static void *handle_thread(void)
+static void *handle_thread(void *arg LTP_ATTRIBUTE_UNUSED)
 {
 	static struct uffd_msg msg;
 	struct uffdio_poison uffdio_poison = {};
@@ -74,7 +74,7 @@ static void *handle_thread(void)
 	SAFE_READ(1, uffd, &msg, sizeof(msg));
 
 	if (msg.event != UFFD_EVENT_PAGEFAULT)
-		tst_brk(TFAIL, "Received unexpected UFFD_EVENT %d", msg.event);
+		tst_brk(TFAIL | TERRNO, "Received unexpected UFFD_EVENT %d", msg.event);
 
 	tst_atomic_store(1, &poison_fault_seen);
 
-- 
2.53.0


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

Re: [LTP] [PATCH v3] userfaultfd: Minor fixes

[Petr Vorel]

Hi all,

> - Use POSIX semantics for thread function
> - Set cleanup to call reset_pages

LGTM.

As a separate effort: Cyril raised a question if (as a separate effort) should
all test have some kconfig check (e.g. CONFIG_USERFAULTFD=y)

IMHO it's a question if we are ok with wide spread of needs_kconfig even if it's
not necessary (i.e. here SAFE_USERFAULTFD() check. In that case the benefit
would be 1) documentation of dependencies for testers in the test catalog 2)
check earlier that before running test code. And of course disadvantage to
really drag kconfig dependency, but we have accepted that already.

Ideally we'd decide on some policy (which may also includes .needs_drivers).
@all I planned to reword my original policy patch [1] to Li's suggestion +
Cyril's kconfig changes [2], but that still does not have "use kconfig to
document dependencies".

Kind regards,
Petr

[1] https://patchwork.ozlabs.org/project/ltp/patch/20260128080121.18878-1-pvorel@suse.cz/
[2] https://patchwork.ozlabs.org/comment/3662031/

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

Re: [LTP] [PATCH v3] userfaultfd: Minor fixes

[Cyril Hrubis]

Hi!
> As a separate effort: Cyril raised a question if (as a separate effort) should
> all test have some kconfig check (e.g. CONFIG_USERFAULTFD=y)
> 
> IMHO it's a question if we are ok with wide spread of needs_kconfig even if it's
> not necessary (i.e. here SAFE_USERFAULTFD() check. In that case the benefit
> would be 1) documentation of dependencies for testers in the test catalog 2)
> check earlier that before running test code. And of course disadvantage to
> really drag kconfig dependency, but we have accepted that already.
> 
> Ideally we'd decide on some policy (which may also includes .needs_drivers).
> @all I planned to reword my original policy patch [1] to Li's suggestion +
> Cyril's kconfig changes [2], but that still does not have "use kconfig to
> document dependencies".

I think that we can get rid of needs_drivers if we add more runtime
checks.

We have needs_drivers in:

- kvm tests -> CONFIG_KVM + runtime check for kvm module
- zram tests -> CONFIG_ZRAM + runtime check for zram module
- squasfs tests -> CONFIG_SQUASHFS + runtime check for squashfs module
- setxattr02 -> CONFIG_BLK_DEV_RAM + runtime check for brd module
- ioctl08 -> has both filesystems and needs_drivers set to btrfs
             likely we do not need the needs_drivers there
- ioctl_loop tests -> CONFIG_BLK_DEV_LOOP + runtime check for loop module
- madvise11 -> CONFIG_HWPOISON_INJECT + runtime check for hwpoison_inject module
- quotactl tests -> CONFIG_QUOTA_V2 + runtime check for quota_v2 module
- uinput tests -> CONFIG_BLK_DEV_LOOP + CONFIG_TUN + CONFIG_INPUT_UINPUT
- can tests -> CONFIG_CAN_VCAN + CONFIG_CAN_RAW + CONFIG_CAN_BCM


If we agree that we want to switch from needs_drivers to needs_kconfig +
runtime checks I will add mappings from congfigs to module names into
tst_kconfig and convert the tests..

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v3] userfaultfd: Minor fixes

[Petr Vorel]

> Hi!
> > As a separate effort: Cyril raised a question if (as a separate effort) should
> > all test have some kconfig check (e.g. CONFIG_USERFAULTFD=y)

> > IMHO it's a question if we are ok with wide spread of needs_kconfig even if it's
> > not necessary (i.e. here SAFE_USERFAULTFD() check. In that case the benefit
> > would be 1) documentation of dependencies for testers in the test catalog 2)
> > check earlier that before running test code. And of course disadvantage to
> > really drag kconfig dependency, but we have accepted that already.

> > Ideally we'd decide on some policy (which may also includes .needs_drivers).
> > @all I planned to reword my original policy patch [1] to Li's suggestion +
> > Cyril's kconfig changes [2], but that still does not have "use kconfig to
> > document dependencies".

> I think that we can get rid of needs_drivers if we add more runtime
> checks.

> We have needs_drivers in:

> - kvm tests -> CONFIG_KVM + runtime check for kvm module
Maybe some of KVM might be more specific.

> - zram tests -> CONFIG_ZRAM + runtime check for zram module
> - squasfs tests -> CONFIG_SQUASHFS + runtime check for squashfs module
> - setxattr02 -> CONFIG_BLK_DEV_RAM + runtime check for brd module
> - ioctl08 -> has both filesystems and needs_drivers set to btrfs
>              likely we do not need the needs_drivers there
> - ioctl_loop tests -> CONFIG_BLK_DEV_LOOP + runtime check for loop module
> - madvise11 -> CONFIG_HWPOISON_INJECT + runtime check for hwpoison_inject module
> - quotactl tests -> CONFIG_QUOTA_V2 + runtime check for quota_v2 module
And quota_remount_test01.sh shell test.

> - uinput tests -> CONFIG_BLK_DEV_LOOP + CONFIG_TUN + CONFIG_INPUT_UINPUT
> - can tests -> CONFIG_CAN_VCAN + CONFIG_CAN_RAW + CONFIG_CAN_BCM

FYI a bit more, just few of them, but fortunately all for have config.
I'd be surprised if there was a kernel module without it.

testcases/cve/tcindex01.c dummy -> CONFIG_DUMMY

shell has more tests (fortunately already supports $TST_NEEDS_KCONFIGS):
tst_net.sh (if using nets) netns_lib.sh: veth -> CONFIG_VETH
binfmt_misc_lib.sh: binfmt_misc -> CONFIG_BINFMT_MISC + runtime check for binfmt_misc module
+ many more

> If we agree that we want to switch from needs_drivers to needs_kconfig +
> runtime checks I will add mappings from congfigs to module names into
> tst_kconfig and convert the tests..

+1

Kind regards,
Petr

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

Re: [LTP] [PATCH v3] userfaultfd: Minor fixes

[Andrea Cervesato via ltp]

Hi!

>  	if (msg.event != UFFD_EVENT_PAGEFAULT)
> -		tst_brk(TFAIL, "Received unexpected UFFD_EVENT %d", msg.event);
> +		tst_brk(TFAIL | TERRNO, "Received unexpected UFFD_EVENT %d", msg.event);

Looking closer..

TERRNO here is not needed. At this point SAFE_READ() passed and we are going
to print the errno from a previous failing call (most probably from a previous
`-i` iteration) if we receive the wrong event.

We can fix before merge if other accept this review.

Reviewed-by: Andrea Cervesato <andrea.cervesato@suse.com>

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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