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

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi!

On 3/3/25 10:42, Wei Gao via ltp wrote:
> This is new test case adapted from the kernel self test unshare_test.c.
> It verifies that the kernel correctly handles the EMFILE error condition
> during file descriptor table unsharing, specifically when the parent
> process modifies the file descriptor limits and the child process attempts
> to unshare(CLONE_FILES).
Add a test case based on kernel self-test unshare_test.c to check that 
the kernel handles the EMFILE error when a parent process changes file 
descriptor limits and the child process tries to unshare (CLONE_FILES).

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>   runtest/syscalls                              |  1 +
>   testcases/kernel/syscalls/unshare/.gitignore  |  1 +
>   testcases/kernel/syscalls/unshare/unshare03.c | 75 +++++++++++++++++++
>   3 files changed, 77 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..c8baecc10
> --- /dev/null
> +++ b/testcases/kernel/syscalls/unshare/unshare03.c
> @@ -0,0 +1,75 @@
> +// 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>
> + */
> +
> +/*\
> + * 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.
Probably we can use the commit message here :-)
> + */
> +
> +#define _GNU_SOURCE
> +
> +#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 tst_clone_args args = {
> +		.flags = CLONE_FILES,
> +		.exit_signal = SIGCHLD,
> +	};
> +
> +	SAFE_FILE_SCANF(FS_NR_OPEN, "%d", &nr_open);
Here we can print the number of open file descriptors, using tst_res(), 
to help debugging.
> +
> +	SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_open + 1024);

In the previous version I made a mistake in the review. The original 
test is using /proc/sys/fs/nr_open to set limits first, then it reads 
back them from the same file in order to have a starting limit. This is 
probably needed due to the kernel configurations. So please bring back 
the SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit); line I asked to remove. 
Sorry for that.

Also, if we are going to use new increments, we need to update the next 
increments as well according to the previous ones. So feel free to leave 
it as it was before.

I must have been distracted that day :-)

> +
> +	rlimit.rlim_cur = nr_open + 16;
> +	rlimit.rlim_max = nr_open + 16;
> +
> +	SAFE_SETRLIMIT(RLIMIT_NOFILE, &rlimit);
Here we can print the new limits after updating it, using tst_res(), to 
help debugging.
> +
> +	SAFE_DUP2(2, nr_open + 8);
> +
> +	if (!SAFE_CLONE(&args)) {
> +		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_open);
> +		TST_EXP_FAIL(unshare(CLONE_FILES), EMFILE);
> +		TST_CHECKPOINT_WAKE(0);
> +		exit(0);
> +	}
> +
> +	TST_CHECKPOINT_WAIT(0);
> +	tst_res(TPASS, "Verify EMFILE error pass");
We don't need this since we already have TST_EXP_FAIL inside the child 
process.
> +}
> +
> +static void setup(void)
> +{
> +	clone3_supported_by_kernel();
> +}
> +
> +static struct tst_test test = {
> +	.forks_child = 1,
> +	.needs_root = 1,
> +	.test_all = run,
> +	.setup = setup,
> +	.needs_checkpoints = 1,
> +	.save_restore = (const struct tst_path_val[]) {
> +		{FS_NR_OPEN, NULL, TST_SR_TCONF},
> +		{}
> +	},
> +};
> +
> +#else
> +TST_TEST_TCONF("unshare is undefined.");
unshare syscall is undefined.
> +#endif

The rest looks ok.

Andrea


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