Re: [LTP] [PATCH 1/8] Hugetlb: Migrating libhugetlbfs fork-cow

Richard Palethorpe
2022-12-05T12:50:14Z

Hello,

Tarun Sahu <tsahu@linux.ibm.com> writes:

> Migrating the libhugetlbfs/testcases/fork-cow.c test
>
> Test Description: This checks copy-on-write semantics, specifically the
> semantics of a MAP_PRIVATE mapping across a fork(). Some versions of the
> powerpc kernel had a bug in huge_ptep_set_wrprotect() which would fail to
> flush the hash table after setting the write protect bit in the parent's
> page tables, thus allowing the parent to pollute the child's mapping.
>
> Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
> ---
> runtest/hugetlb | 2 +
> testcases/kernel/mem/.gitignore | 1 +
> .../kernel/mem/hugetlb/hugefork/Makefile | 10 ++
> .../kernel/mem/hugetlb/hugefork/hugefork01.c | 108 ++++++++++++++++++
> 4 files changed, 121 insertions(+)
> create mode 100644 testcases/kernel/mem/hugetlb/hugefork/Makefile
> create mode 100644 testcases/kernel/mem/hugetlb/hugefork/hugefork01.c
>
> diff --git a/runtest/hugetlb b/runtest/hugetlb
n> index ec1fc2515..4c16e1e7c 100644
> --- a/runtest/hugetlb
> +++ b/runtest/hugetlb
> @@ -1,6 +1,8 @@
> hugefallocate01 hugefallocate01
> hugefallocate02 hugefallocate02
>
> +hugefork01 hugefork01
> +
> hugemmap01 hugemmap01
> hugemmap02 hugemmap02
> hugemmap04 hugemmap04
> diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore
> index c0906f3d3..adea760c7 100644
> --- a/testcases/kernel/mem/.gitignore
> +++ b/testcases/kernel/mem/.gitignore
> @@ -1,6 +1,7 @@
> /cpuset/cpuset01
> /hugetlb/hugefallocate/hugefallocate01
> /hugetlb/hugefallocate/hugefallocate02
> +/hugetlb/hugefork/hugefork01
> /hugetlb/hugemmap/hugemmap01
> /hugetlb/hugemmap/hugemmap02
> /hugetlb/hugemmap/hugemmap04
> diff --git a/testcases/kernel/mem/hugetlb/hugefork/Makefile b/testcases/kernel/mem/hugetlb/hugefork/Makefile
> new file mode 100644
> index 000000000..77ebb0aef
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugefork/Makefile
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2009, Cisco Systems Inc.
> +# Ngie Cooper, July 2009
> +
> +top_srcdir ?= ../../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +include $(abs_srcdir)/../Makefile.inc
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> +
> diff --git a/testcases/kernel/mem/hugetlb/hugefork/hugefork01.c b/testcases/kernel/mem/hugetlb/hugefork/hugefork01.c
> new file mode 100644
> index 000000000..b59c461e3
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugefork/hugefork01.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: LGPL-2.1-or-later
> +/*
> + * Copyright (C) 2008 David Gibson, IBM Corporation.
> + * Author: David Gibson
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This checks copy-on-write semantics, specifically the semantics of a
> + * MAP_PRIVATE mapping across a fork(). Some versions of the powerpc
> + * kernel had a bug in huge_ptep_set_wrprotect() which would fail to
> + * flush the hash table after setting the write protect bit in the parent's
> + * page tables, thus allowing the parent to pollute the child's mapping.
> + *
> + */
> +
> +#include <sys/wait.h>
> +#include <sys/mman.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +
> +#include "hugetlb.h"
> +
> +#define RANDOM_CONSTANT 0x1234ABCD
> +#define OTHER_CONSTANT 0xfeef5678

It seems their are actually 3 constants as "random" is inverted. I'd
prefer it if they had names like C1, C2, C3 with no connotations.

> +#define MNTPOINT "hugetlbfs/"
> +static int fd = -1;
> +static long hpage_size;
> +
> +static void run_test(void)
> +{
> + int status;
> + volatile unsigned int *p;
> + volatile unsigned int *child_readback;
> + int parent_readback;
> + pid_t pid;
> +
> + child_readback = SAFE_MMAP(NULL, getpagesize(), PROT_READ|PROT_WRITE,
> + MAP_SHARED|MAP_ANONYMOUS, -1, 0);
> + *child_readback = 0;
> +
> + p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
> + *p = RANDOM_CONSTANT;
> +
> + pid = SAFE_FORK();
> + if (pid != 0) {
> + *p = ~RANDOM_CONSTANT;
> + TST_CHECKPOINT_WAKE_AND_WAIT(0);
> + parent_readback = *p;
> + TST_CHECKPOINT_WAKE(0);
> + } else {
> + TST_CHECKPOINT_WAIT(0);
> + *child_readback = *p;
> + *p = OTHER_CONSTANT;
> + TST_CHECKPOINT_WAKE_AND_WAIT(0);
> + exit(0);
> + }
> +
> + SAFE_WAITPID(pid, &status, 0);
> + if (WEXITSTATUS(status) != 0) {
> + tst_res(TFAIL, "Child failed: %d", WEXITSTATUS(status));
> + goto cleanup;
> + }

This can be replaced with tst_reap_children();

> +
> + tst_res(TINFO, "child_readback = 0x%x, parent_readback = 0x%x",
> + *child_readback, parent_readback);
> +
> + if (*child_readback != RANDOM_CONSTANT) {
> + tst_res(TFAIL, "Child read back 0x%x instead of 0x%x",
> + *child_readback, RANDOM_CONSTANT);

I think this could be checked at the end of the child and the extra mmap
for child_readback removed. The LTP lib already creats some shared
memory with children to propagate results.

Assuming that mmap is not needed for the original bug reproducer.

> + goto cleanup;

I don't think this is necessary.

> + }
> + if (parent_readback != ~RANDOM_CONSTANT) {

These comparisons could be replaced with TST_EXP_EQ_LU or TST_EXP_EXPR.

--
Thank you,
Richard.