[LTP] [PATCH v1] unshare03.c: Add test coverage for dup_fd() failure handling in unshare_fd()

[Andrea Cervesato]

Hi!

On 12/19/24 09:19, Wei Gao via ltp wrote:
> Signed-off-by: Wei Gao<wegao@suse.com>
Missing an explanation of the commit.
> ---
>   runtest/syscalls                              |  1 +
>   testcases/kernel/syscalls/unshare/.gitignore  |  1 +
>   testcases/kernel/syscalls/unshare/unshare03.c | 91 +++++++++++++++++++
>   3 files changed, 93 insertions(+)
>   create mode 100644 testcases/kernel/syscalls/unshare/unshare03.c
>
> diff --git a/runtest/syscalls b/runtest/syscalls
> index ded035ee8..10800c1a3 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1715,6 +1715,7 @@ unlinkat01 unlinkat01
>   
>   unshare01 unshare01
>   unshare02 unshare02
> +unshare03 unshare03
>   
>   #
>   # These tests require an unmounted block device
> diff --git a/testcases/kernel/syscalls/unshare/.gitignore b/testcases/kernel/syscalls/unshare/.gitignore
> index 855ffd055..e5b5c261d 100644
> --- a/testcases/kernel/syscalls/unshare/.gitignore
> +++ b/testcases/kernel/syscalls/unshare/.gitignore
> @@ -1,2 +1,3 @@
>   /unshare01
>   /unshare02
> +/unshare03
> diff --git a/testcases/kernel/syscalls/unshare/unshare03.c b/testcases/kernel/syscalls/unshare/unshare03.c
> new file mode 100644
> index 000000000..0ff40b242
> --- /dev/null
> +++ b/testcases/kernel/syscalls/unshare/unshare03.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2024 Al Viro<viro@zeniv.linux.org.uk>
> + * Copyright (C) 2024 Wei Gao<wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
No needed anymore.
> + *
> + * Test case is adapted from the kernel self test unshare_test.c.
> + * Test coverage for dup_fd() failure handling in unshare_fd()
The test is verifying whether unshare() raises EMFILE error when we 
attempt to release the file descriptor table shared with the parent 
process, after opening a new file descriptor in the parent and modifying 
the maximum number of file descriptors in the child.
> + */
> +
> +#define _GNU_SOURCE
> +
> +#include <stdio.h>
> +#include <sys/wait.h>
> +#include <sys/types.h>
> +#include <sys/param.h>
> +#include <sys/syscall.h>
> +#include <sched.h>
> +#include <limits.h>
> +#include <unistd.h>
All these imports are not needed.
> +
> +#include "tst_test.h"
> +#include "config.h"
> +#include "lapi/sched.h"
> +
> +#define FS_NR_OPEN "/proc/sys/fs/nr_open"
> +
> +#ifdef HAVE_UNSHARE
> +
> +static void run(void)
> +{
> +	int nr_open;
> +	struct rlimit rlimit;
> +	pid_t pid;
> +	struct clone_args args = {
> +		.flags = CLONE_FILES,
> +		.exit_signal = SIGCHLD,
> +	};
> +
> +	SAFE_FILE_SCANF(FS_NR_OPEN, "%d", &nr_open);
> +
> +	SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_open + 1024);
> +
> +	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);

I don't get the point of this call, even in the kselftests. The limits 
are overridden in the next 2 lines..

> +
> +	rlimit.rlim_cur = nr_open + 1024;
> +	rlimit.rlim_max = nr_open + 1024;
> +
> +	SAFE_SETRLIMIT(RLIMIT_NOFILE, &rlimit);
> +
> +	SAFE_DUP2(2, nr_open + 64);
We have the control over file descriptors in this process, so I don't 
see why adding such a big number of file descriptors, but I guess 
nothing happens if we are adding high numbers. Maybe 
rlimit.rlim_cur/rlim_max + 16 would be enough. And then here we can use 
nr_open + 8 ...
> +
> +	pid = clone3(&args, sizeof(args));
> +
> +	if (pid < 0) {
We can use SAFE_CLONE() here
> +		tst_res(TFAIL | TTERRNO, "clone3() failed");
> +		return;
> +	}
> +
> +	if (!pid) {
> +		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_open);
> +		TST_EXP_FAIL(unshare(CLONE_FILES), EMFILE);
> +		exit(0);
> +	}
> +
> +	SAFE_WAITPID(pid, NULL, 0);
No need for this, tst_reap_children() is running automatically at the 
end of run().
> +}
> +
> +static void setup(void)
> +{
> +	clone3_supported_by_kernel();
> +}
> +
> +static struct tst_test test = {
> +	.forks_child = 1,
> +	.needs_tmpdir = 1,
Temporary folder is not used.
> +	.needs_root = 1,
> +	.test_all = run,
> +	.setup = setup,
> +	.save_restore = (const struct tst_path_val[]) {
> +		{FS_NR_OPEN, NULL, TST_SR_TCONF},
> +		{}
> +	},
> +};
> +
> +#else
> +TST_TEST_TCONF("unshare is undefined.");
> +#endif
>
Kind regards,
Andrea Cervesato