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

[Richard Palethorpe <rpalethorpe@suse.de>]

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.

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