Re: [LTP] [PATCH] hugemmap/hugemmap41: Migrate ptrace-write-hugepage from libhugetlbfs

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> 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 8ee0e6f82..32412ed45 100644
> --- a/runtest/hugetlb
> +++ b/runtest/hugetlb
> @@ -37,6 +37,7 @@ hugemmap31 hugemmap31
>  hugemmap32 hugemmap32
>  hugemmap34 hugemmap34
>  hugemmap35 hugemmap35
> +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 0e59035df..29eb83d16 100644
> --- a/testcases/kernel/mem/.gitignore
> +++ b/testcases/kernel/mem/.gitignore
> @@ -37,6 +37,7 @@
>  /hugetlb/hugemmap/hugemmap32
>  /hugetlb/hugemmap/hugemmap34
>  /hugetlb/hugemmap/hugemmap35
> +/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..0fbe7b448
> --- /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.
> + * Copyright (c) 2026 Pavithra <pavrampu@linux.ibm.com>
> + */
> +
> +/*\
> + * Test :manpage:`ptrace(2)` write to hugepage memory.
> + *
> + * A child process maps a hugepage via hugetlbfs, zeroes it, and sends
> + * the mapped address to the parent. The parent attaches with
> + * :manpage:`ptrace(2)`, then uses PTRACE_POKEDATA and PTRACE_PEEKDATA
> + * to write and read back a known value at two different offsets within
> + * the hugepage, verifying that ptrace operates correctly on
> + * hugepage-backed memory regions.
> + *
> + * Requires root to mount hugetlbfs and for :manpage:`ptrace(2)`
> + * PTRACE_ATTACH to work across processes when ptrace_scope is
> + * restrictive.
> + */
> +
> +#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 int fd = -1;
> +
> +static void child(int hugefd, int pipefd[2])
> +{
> +	void *p;
> +
> +	SAFE_CLOSE(pipefd[0]);
> +
> +	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[1], &p, sizeof(p));
> +	SAFE_CLOSE(pipefd[1]);
> +
> +	pause();
> +	/* Child is killed by parent via SIGKILL, so cleanup is not reached */
> +}
> +
> +static void do_poke(pid_t pid, void *p)
> +{
> +	tst_res(TINFO, "Poking at %p...", p);
> +	TEST(ptrace(PTRACE_POKEDATA, pid, p, (void *)CONST));
> +	if (TST_RET != 0)
> +		tst_brk(TFAIL | TTERRNO, "ptrace(POKEDATA) failed");

We have SAFE_PTRACE() make use of that.

> +	tst_res(TINFO, "Peeking at %p...", p);
> +	errno = 0;

No need to clear errno, the TEST() macro does that.

> +	TEST(ptrace(PTRACE_PEEKDATA, pid, p, NULL));
> +	if (TST_RET == -1 && TST_ERR)
> +		tst_brk(TFAIL | TTERRNO, "ptrace(PEEKDATA) failed");

Here as well.

> +	if (TST_RET != CONST)
> +		tst_brk(TFAIL, "Value mismatch: got %lx, expected %lx",
> +			TST_RET, CONST);

This can be TST_EXP_EQ_LI()

> +}
> +
> +static void run_test(void)
> +{
> +	int pipefd[2];
> +	pid_t cpid;
> +	void *p;
> +	int status;
> +
> +	fd = tst_creat_unlinked(MNTPOINT, 0, 0600);
> +
> +	SAFE_PIPE(pipefd);
> +
> +	cpid = SAFE_FORK();
> +
> +	if (cpid == 0) {
> +		child(fd, pipefd);
> +		exit(0);
> +	}
> +
> +	/* Parent */

Please bo comments commenting the obvious like this one.

> +	SAFE_CLOSE(pipefd[1]);
> +	SAFE_READ(1, pipefd[0], &p, sizeof(p));
> +	SAFE_CLOSE(pipefd[0]);
> +
> +	tst_res(TINFO, "Parent received address %p", p);
> +
> +	if (ptrace(PTRACE_ATTACH, cpid, NULL, NULL)) {
> +		if (errno == EPERM)
> +			tst_brk(TCONF | TERRNO, "ptrace(ATTACH) not permitted (check /proc/sys/kernel/yama/ptrace_scope)");
> +		tst_brk(TBROK | TERRNO, "ptrace(ATTACH) failed");
> +	}

This should be just SAFE_PTRACE() if we need to check for EPERM because
of security checks it should be done, in a separate patch, in
tst_safe_ptrace() instead.

> +	TST_PROCESS_STATE_WAIT(cpid, 't', 0);

The more canonical way is to SAFE_WAITPID() and check for WIFSTOPPED().

> +	do_poke(cpid, p);
> +	do_poke(cpid, p + getpagesize());
> +
> +	SAFE_KILL(cpid, SIGKILL);
> +	SAFE_WAITPID(cpid, &status, 0);
> +
> +	if (fd != -1) {
> +		SAFE_CLOSE(fd);
> +		fd = -1;
> +	}

This should be just SAFE_CLOSE(fd)

- we cannot get here if the fd == -1 since the tst_create_unlinked()
  exits the test on a failure

- the fd is set to -1 by SAFE_CLOSE() macro.

> +	tst_res(TPASS, "ptrace write to hugepage succeeded");
> +}
> +
> +static void setup(void)
> +{
> +	hpage_size = tst_get_hugepage_size();
> +}
> +
> +static void cleanup(void)
> +{
> +	if (fd != -1)
> +		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},

The test needs just 1 hugepage.

> +	.forks_child = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run_test,
> +};
> -- 
> 2.55.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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