Re: [LTP] [PATCH] syscalls/fork02: new test for fork()

Re: [LTP] [PATCH] syscalls/fork02: new test for fork()

[zhaogongyi via ltp]

Hi Cyril,

> 
> Hi!
> > > > Verify that current workdir correctly inherited by child.
> > >
> > > Why test this?  Does fork() man-pages menthion this?
> > >
> >
> > Man-pages does not menthion this, and the other field inherited does
> not mentioned too.
> 
> I guess that this is described in POSIX in:
> 
> "The new process (child process) shall be an exact copy of the calling
> process (parent process) except as detailed below:"
> 
> at https://pubs.opengroup.org/onlinepubs/9699919799/
> 
> And there is nothing about working dir in the list of special cases hence
> working dir is exactly the same as for parent since it has to be exact copy
> of parent.
> 

It seems reasonable to not test the result of full inheritance from the parent process, otherwise there are a lot of testcases.

Regards,

Gongyi

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

Re: [LTP] [PATCH] syscalls/fork02: new test for fork()

[Cyril Hrubis]

Hi!
> > > Man-pages does not menthion this, and the other field inherited does
> > not mentioned too.
> > 
> > I guess that this is described in POSIX in:
> > 
> > "The new process (child process) shall be an exact copy of the calling
> > process (parent process) except as detailed below:"
> > 
> > at https://pubs.opengroup.org/onlinepubs/9699919799/
> > 
> > And there is nothing about working dir in the list of special cases hence
> > working dir is exactly the same as for parent since it has to be exact copy
> > of parent.
> > 
> 
> It seems reasonable to not test the result of full inheritance from the parent process, otherwise there are a lot of testcases.

Certainly the interesting part would be testing all the exceptions, but
having one test that checks a few things that are supposed to be the
same wouldn't harm either.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] syscalls/fork02: new test for fork()

[zhaogongyi via ltp]

Hi!

> 
> Hello,
> 
> Zhao Gongyi via ltp <ltp@lists.linux.it> writes:
> 
> > Verify that current workdir correctly inherited by child.
> 
> I'm not convinced by Cyrils comments that this should be included. I doubt
> LTP would work if the CWD wasn't inherited by children. At least in the
> common case. The test needs to do something more IMO.
> 
> Perhaps there has been a kernel bug involving fork and CWD? Perhaps
> relating to new file systems and mount options. I doubt we will see issues
> on a common setup. This isn't adding any test coverage in that case.
> 
> Also the path to the CWD and the CWD are two different things. The
> kernel generates the path on each invocation of getcwd IIRC.
> 
> Marking this as changes requested in patchwork. Although really, I think
> effort should be directed at clone3 which is lacking a lot of coverage.
> 
> --
> Thank you,
> Richard.

the pwd is one member of fs_struct:

struct fs_struct {
	int users;
	spinlock_t lock;
	seqcount_spinlock_t seq;
	int umask;
	int in_exec;
	struct path root, pwd;
} __randomize_layout;

And fs_struct is one member of struct task_struct:

	/* Filesystem information: */
	struct fs_struct		*fs;

In my opinion, the pwd will inherit from parent naturally if there is no a explicit change in fork.

Regards,
Gongyi


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

Re: [LTP] [PATCH] syscalls/fork02: new test for fork()

[zhaogongyi via ltp]

Hi,

> 
> Hi Zhao
> 
> > Verify that current workdir correctly inherited by child.
> 
> Why test this?  Does fork() man-pages menthion this?
> 

Man-pages does not menthion this, and the other field inherited does not mentioned too.

As far as I know, the child process inherite:

Uids and gids
Environment
Stack
Share memory
Opened fd
Flags of close-on-exec
Pgid
Cwd
Rootdir
Mask
Rlimit
Tty

Maybe we can verify them similarly.


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

Re: [LTP] [PATCH] syscalls/fork02: new test for fork()

[Cyril Hrubis]

Hi!
> > > Verify that current workdir correctly inherited by child.
> > 
> > Why test this?  Does fork() man-pages menthion this?
> > 
> 
> Man-pages does not menthion this, and the other field inherited does not mentioned too.

I guess that this is described in POSIX in:

"The new process (child process) shall be an exact copy of the calling
 process (parent process) except as detailed below:"

at https://pubs.opengroup.org/onlinepubs/9699919799/

And there is nothing about working dir in the list of special cases
hence working dir is exactly the same as for parent since it has to be
exact copy of parent.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[LTP] [PATCH] syscalls/fork02: new test for fork()

[Zhao Gongyi via ltp]

Verify that current workdir correctly inherited by child.

Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
---
 runtest/syscalls                          |  1 +
 testcases/kernel/syscalls/fork/.gitignore |  1 +
 testcases/kernel/syscalls/fork/fork02.c   | 51 +++++++++++++++++++++++
 3 files changed, 53 insertions(+)
 create mode 100644 testcases/kernel/syscalls/fork/fork02.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 7a425439a..39b7f21f6 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -364,6 +364,7 @@ flock06 flock06
 fmtmsg01 fmtmsg01

 fork01 fork01
+fork02 fork02
 fork03 fork03
 fork04 fork04
 fork05 fork05
diff --git a/testcases/kernel/syscalls/fork/.gitignore b/testcases/kernel/syscalls/fork/.gitignore
index b817e9c05..8a08c586b 100644
--- a/testcases/kernel/syscalls/fork/.gitignore
+++ b/testcases/kernel/syscalls/fork/.gitignore
@@ -1,4 +1,5 @@
 /fork01
+/fork02
 /fork03
 /fork04
 /fork05
diff --git a/testcases/kernel/syscalls/fork/fork02.c b/testcases/kernel/syscalls/fork/fork02.c
new file mode 100644
index 000000000..54ad50712
--- /dev/null
+++ b/testcases/kernel/syscalls/fork/fork02.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright(c) 2022 Huawei Technologies Co., Ltd
+ * Author: Zhao Gongyi <zhaogongyi@huawei.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Verify that current workdir correctly inherited by child.
+ */
+
+#include "tst_test.h"
+
+#define NEW_DIR "new_dir"
+
+static char parent_cwd[PATH_MAX];
+static char child_cwd[PATH_MAX];
+
+static void run(void)
+{
+	pid_t pid = SAFE_FORK();
+
+	if (!pid) {
+		SAFE_GETCWD(child_cwd, sizeof(child_cwd));
+		if (strcmp(child_cwd, parent_cwd)) {
+			tst_res(TFAIL, "checking of cwd failed: "
+					"child_cwd %s parent_cwd %s ",
+					child_cwd, parent_cwd);
+		} else
+			tst_res(TPASS, "Checking of cwd passed");
+		return;
+	}
+
+	SAFE_WAITPID(pid, NULL, 0);
+}
+
+static void setup(void)
+{
+	SAFE_MKDIR(NEW_DIR, 00770);
+	SAFE_CHDIR(NEW_DIR);
+	SAFE_GETCWD(parent_cwd, sizeof(parent_cwd));
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.test_all = run,
+	.needs_tmpdir = 1,
+	.forks_child = 1,
+};
+
--
2.17.1


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

Re: [LTP] [PATCH] syscalls/fork02: new test for fork()

[xuyang2018.jy]

Hi Zhao

> Verify that current workdir correctly inherited by child.

Why test this?  Does fork() man-pages menthion this?

Best Regards
Yang Xu

> 
> Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> ---
>   runtest/syscalls                          |  1 +
>   testcases/kernel/syscalls/fork/.gitignore |  1 +
>   testcases/kernel/syscalls/fork/fork02.c   | 51 +++++++++++++++++++++++
>   3 files changed, 53 insertions(+)
>   create mode 100644 testcases/kernel/syscalls/fork/fork02.c
> 
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 7a425439a..39b7f21f6 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -364,6 +364,7 @@ flock06 flock06
>   fmtmsg01 fmtmsg01
> 
>   fork01 fork01
> +fork02 fork02
>   fork03 fork03
>   fork04 fork04
>   fork05 fork05
> diff --git a/testcases/kernel/syscalls/fork/.gitignore b/testcases/kernel/syscalls/fork/.gitignore
> index b817e9c05..8a08c586b 100644
> --- a/testcases/kernel/syscalls/fork/.gitignore
> +++ b/testcases/kernel/syscalls/fork/.gitignore
> @@ -1,4 +1,5 @@
>   /fork01
> +/fork02
>   /fork03
>   /fork04
>   /fork05
> diff --git a/testcases/kernel/syscalls/fork/fork02.c b/testcases/kernel/syscalls/fork/fork02.c
> new file mode 100644
> index 000000000..54ad50712
> --- /dev/null
> +++ b/testcases/kernel/syscalls/fork/fork02.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright(c) 2022 Huawei Technologies Co., Ltd
> + * Author: Zhao Gongyi <zhaogongyi@huawei.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Verify that current workdir correctly inherited by child.
> + */
> +
> +#include "tst_test.h"
> +
> +#define NEW_DIR "new_dir"
> +
> +static char parent_cwd[PATH_MAX];
> +static char child_cwd[PATH_MAX];
> +
> +static void run(void)
> +{
> +	pid_t pid = SAFE_FORK();
> +
> +	if (!pid) {
> +		SAFE_GETCWD(child_cwd, sizeof(child_cwd));
> +		if (strcmp(child_cwd, parent_cwd)) {
> +			tst_res(TFAIL, "checking of cwd failed: "
> +					"child_cwd %s parent_cwd %s ",
> +					child_cwd, parent_cwd);
> +		} else
> +			tst_res(TPASS, "Checking of cwd passed");
> +		return;
> +	}
> +
> +	SAFE_WAITPID(pid, NULL, 0);
> +}
> +
> +static void setup(void)
> +{
> +	SAFE_MKDIR(NEW_DIR, 00770);
> +	SAFE_CHDIR(NEW_DIR);
> +	SAFE_GETCWD(parent_cwd, sizeof(parent_cwd));
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.test_all = run,
> +	.needs_tmpdir = 1,
> +	.forks_child = 1,
> +};
> +
> --
> 2.17.1
> 
> 

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

Re: [LTP] [PATCH] syscalls/fork02: new test for fork()

[Richard Palethorpe]

Hello,

Zhao Gongyi via ltp <ltp@lists.linux.it> writes:

> Verify that current workdir correctly inherited by child.

I'm not convinced by Cyrils comments that this should be included. I
doubt LTP would work if the CWD wasn't inherited by children. At least
in the common case. The test needs to do something more IMO.

Perhaps there has been a kernel bug involving fork and CWD? Perhaps
relating to new file systems and mount options. I doubt we will see
issues on a common setup. This isn't adding any test coverage in that
case.

Also the path to the CWD and the CWD are two different things. The
kernel generates the path on each invocation of getcwd IIRC.

Marking this as changes requested in patchwork. Although really, I think
effort should be directed at clone3 which is lacking a lot of coverage.

-- 
Thank you,
Richard.

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