[LTP] [PATCH] [PATCH] [V2] Migrating the libhugetlbfs/testcases/ptrace-write-hugepage.c

[LTP] [PATCH] [PATCH] [V2] Migrating the libhugetlbfs/testcases/ptrace-write-hugepage.c

[Pavithra]

Changed testcase to use new API

Signed-off-by: Pavithra <pavrampu@linux.ibm.com>
---
 runtest/hugetlb                               |   1 +
 testcases/kernel/mem/.gitignore               |   1 +
 .../kernel/mem/hugetlb/hugemmap/hugemmap41.c  | 147 ++++++++++++++++++
 3 files changed, 149 insertions(+)
 create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap41.c

diff --git a/runtest/hugetlb b/runtest/hugetlb
index 0896d3c94..14ca64018 100644
--- a/runtest/hugetlb
+++ b/runtest/hugetlb
@@ -36,6 +36,7 @@ hugemmap30 hugemmap30
 hugemmap31 hugemmap31
 hugemmap32 hugemmap32
 hugemmap34 hugemmap34
+hugemmap41 hugemmap41
 hugemmap05_1 hugemmap05 -m
 hugemmap05_2 hugemmap05 -s
 hugemmap05_3 hugemmap05 -s -m
diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore
index b4455de51..4bd7481b0 100644
--- a/testcases/kernel/mem/.gitignore
+++ b/testcases/kernel/mem/.gitignore
@@ -36,6 +36,7 @@
 /hugetlb/hugemmap/hugemmap31
 /hugetlb/hugemmap/hugemmap32
 /hugetlb/hugemmap/hugemmap34
+/hugetlb/hugemmap/hugemmap41
 /hugetlb/hugeshmat/hugeshmat01
 /hugetlb/hugeshmat/hugeshmat02
 /hugetlb/hugeshmat/hugeshmat03
diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap41.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap41.c
new file mode 100644
index 000000000..3ea6d9e75
--- /dev/null
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap41.c
@@ -0,0 +1,147 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2005-2006 David Gibson & Adam Litke, IBM Corporation.
+ */
+
+/*\
+ * [Description]
+ *
+ * Test ptrace write to hugepage memory.
+ * This test verifies that ptrace POKEDATA and PEEKDATA work correctly
+ * on hugepage-backed memory regions.
+ */
+
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/ptrace.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include "tst_test.h"
+#include "hugetlb.h"
+
+#define CONST	0xdeadbeefL
+#define MNTPOINT "hugetlbfs/"
+
+static long hpage_size;
+static volatile int ready_to_trace;
+static int fd = -1;
+
+static void sigchld_handler(int signum, siginfo_t *si, void *uc)
+{
+	int status;
+
+	wait(&status);
+	if (WIFEXITED(status))
+		exit(WEXITSTATUS(status));
+	else if (WIFSIGNALED(status))
+		exit(status);
+
+	ready_to_trace = 1;
+}
+
+static void child(int hugefd, int pipefd)
+{
+	void *p;
+
+	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED,
+		      hugefd, 0);
+
+	memset(p, 0, hpage_size);
+
+	tst_res(TINFO, "Child mapped data at %p", p);
+
+	SAFE_WRITE(SAFE_WRITE_ALL, pipefd, &p, sizeof(p));
+
+	pause();
+}
+
+static void do_poke(pid_t pid, void *p)
+{
+	long err;
+
+	tst_res(TINFO, "Poking at %p...", p);
+	err = ptrace(PTRACE_POKEDATA, pid, p, (void *)CONST);
+	if (err)
+		tst_brk(TFAIL | TERRNO, "ptrace(POKEDATA) failed");
+
+	tst_res(TINFO, "Peeking at %p...", p);
+	errno = 0;
+	err = ptrace(PTRACE_PEEKDATA, pid, p, NULL);
+	if (err == -1 && errno)
+		tst_brk(TFAIL | TERRNO, "ptrace(PEEKDATA) failed");
+
+	if (err != CONST)
+		tst_brk(TFAIL, "Value mismatch: got %lx, expected %lx", err, CONST);
+}
+
+static void run_test(void)
+{
+	int pipefd[2];
+	long err;
+	pid_t cpid;
+	void *p;
+	int signal;
+
+	struct sigaction sa = {
+		.sa_sigaction = sigchld_handler,
+		.sa_flags = SA_SIGINFO,
+	};
+	struct sigaction old_sa;
+
+	hpage_size = tst_get_hugepage_size();
+	fd = tst_creat_unlinked(MNTPOINT, 0, 0600);
+
+	SAFE_SIGACTION(SIGCHLD, &sa, &old_sa);
+	SAFE_PIPE(pipefd);
+
+	cpid = SAFE_FORK();
+
+	if (cpid == 0) {
+		child(fd, pipefd[1]);
+		exit(0);
+	}
+
+	/* Parent */
+	SAFE_READ(1, pipefd[0], &p, sizeof(p));
+
+	tst_res(TINFO, "Parent received address %p", p);
+
+	err = ptrace(PTRACE_ATTACH, cpid, NULL, NULL);
+	if (err)
+		tst_brk(TFAIL | TERRNO, "ptrace(ATTACH) failed");
+
+	while (!ready_to_trace)
+		;
+
+	do_poke(cpid, p);
+	do_poke(cpid, p + getpagesize());
+
+	SAFE_SIGACTION(SIGCHLD, &old_sa, NULL);
+
+	SAFE_PTRACE(PTRACE_KILL, cpid, NULL, NULL);
+	SAFE_WAITPID(cpid, &signal, 0);
+
+	tst_res(TPASS, "ptrace write to hugepage succeeded");
+}
+
+static void cleanup(void)
+{
+	if (fd >= 0)
+		SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+	.tags = (struct tst_tag[]) {
+		{"linux-git", "ebed4bfc8da8"},
+		{}
+	},
+	.needs_root = 1,
+	.mntpoint = MNTPOINT,
+	.needs_hugetlbfs = 1,
+	.hugepages = {4, TST_NEEDS},
+	.forks_child = 1,
+	.cleanup = cleanup,
+	.test_all = run_test,
+};
-- 
2.53.0


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

Re: [LTP] [PATCH] [PATCH] [V2] Migrating the libhugetlbfs/testcases/ptrace-write-hugepage.c

[Andrea Cervesato via ltp]

Hi Pavithra,

Thanks for the migration. A few things to address below.

First, the commit subject should follow the LTP convention, e.g.:

  hugemmap41: Migrate ptrace-write-hugepage from libhugetlbfs

The body "Changed testcase to use new API" is too brief. Please explain
what the original test does and why it is being migrated (preserving
regression coverage for the ptrace hugepage fix, etc.).

> +/*\
> + * [Description]
> + *

The [Description] tag is deprecated and must not be used. Just remove
the line and keep the description text directly after the /*\ opener.

> +static volatile int ready_to_trace;

[...]

> +	ready_to_trace = 1;

[...]

> +	while (!ready_to_trace)
> +		;

This busy-wait spin loop combined with a SIGCHLD handler is fragile and
wastes CPU. The whole signal handler + spin loop can be replaced with
TST_PROCESS_STATE_WAIT() after PTRACE_ATTACH:

    err = ptrace(PTRACE_ATTACH, cpid, NULL, NULL);
    if (err)
        tst_brk(TFAIL | TERRNO, "ptrace(ATTACH) failed");

    TST_PROCESS_STATE_WAIT(cpid, 't');

This waits for the child to enter tracing stop, which is exactly what
the original code was trying to achieve. It also eliminates the need for
sigchld_handler() and the ready_to_trace variable entirely.

> +static void run_test(void)
> +{

[...]

> +	hpage_size = tst_get_hugepage_size();
> +	fd = tst_creat_unlinked(MNTPOINT, 0, 0600);

Both hpage_size and fd are static variables set inside run_test(). When
run with -i N, fd is overwritten each iteration without closing the
previous one, leaking file descriptors. Move these to a setup() callback
so they are initialized once, or close fd at the end of each iteration.

Also, ready_to_trace is never reset to 0 at the top of run_test(), so
on -i N runs the spin loop is skipped on iteration 2+, causing a race.
(This goes away if you adopt TST_PROCESS_STATE_WAIT() as suggested
above.)

> +	SAFE_PIPE(pipefd);

[...]

> +	SAFE_READ(1, pipefd[0], &p, sizeof(p));

The pipe file descriptors are never closed. Close pipefd[1] in the
parent after fork, and close pipefd[0] after the read. In the child,
close pipefd[0] before use and close pipefd[1] after the write.

> +static void child(int hugefd, int pipefd)
> +{
> +	void *p;
> +
> +	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED,
> +		      hugefd, 0);

[...]

> +	pause();
> +}

The child maps a hugepage but never calls SAFE_MUNMAP(). Add a munmap
before pause(), or restructure so the mapping is cleaned up.

> +	int signal;

[...]

> +	SAFE_WAITPID(cpid, &signal, 0);

Minor: the variable name "signal" shadows the signal() function. Rename
it to "status" for clarity.

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

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