Re: [LTP] [PATCH v3] mount_setattr02.c: Check mount_setattr attr propagation

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
>  move_mount01 move_mount01
>  move_mount02 move_mount02
> diff --git a/testcases/kernel/syscalls/mount_setattr/.gitignore b/testcases/kernel/syscalls/mount_setattr/.gitignore
> index 52a77b9ca..1654f27de 100644
> --- a/testcases/kernel/syscalls/mount_setattr/.gitignore
> +++ b/testcases/kernel/syscalls/mount_setattr/.gitignore
> @@ -1 +1,2 @@
>  /mount_setattr01
> +/mount_setattr02
> diff --git a/testcases/kernel/syscalls/mount_setattr/mount_setattr02.c b/testcases/kernel/syscalls/mount_setattr/mount_setattr02.c
> new file mode 100644
> index 000000000..fcc088e3b
> --- /dev/null
> +++ b/testcases/kernel/syscalls/mount_setattr/mount_setattr02.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2025 SUSE LLC Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * Basic mount_setattr() test.
> + *
> + * Test basic propagation mount attributes are set correctly.

This is too sparse. We should at least write here which parameters we
are testing, obviously the test is checking if the propagation field of
the structure is handled properly.

And then we should list what we are trying to test

- When propagation is set to 0 it's not changed
- EINVAL with propagation set to -1
- MS_SHARED turns propagation on
- MS_PRIVATE turns propagation off
...

> + */
> +
> +#define _GNU_SOURCE
> +
> +#include <sys/statvfs.h>
> +#include "tst_test.h"
> +#include "lapi/fsmount.h"
> +#include "tst_safe_stdio.h"
> +
> +static char *tmpdir;
> +
> +static int mounted;
> +
> +static bool is_shared_mount(const char *path)
> +{
> +	FILE *file = SAFE_FOPEN("/proc/self/mountinfo", "r");
> +
> +	char line[PATH_MAX];
> +	bool found = false;
> +
> +	while (fgets(line, sizeof(line), file)) {
> +		char mntpoint[PATH_MAX];
> +		char opts[256];
> +
> +		if (sscanf(line, "%*d %*d %*d:%*d %*s %255s %*s %255s",
> +					mntpoint, opts) != 2)
> +			continue;
> +
> +		if (strcmp(mntpoint, path) != 0)
> +			continue;
> +
> +		if (strstr(opts, "shared:") != NULL) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	fclose(file);
> +	return found;
> +}
> +
> +static void cleanup(void)
> +{
> +	if (mounted)
> +		SAFE_UMOUNT(tmpdir);
> +}
> +
> +static void setup(void)
> +{
> +	tmpdir = tst_tmpdir_path();
> +	SAFE_UNSHARE(CLONE_NEWNS);
> +	SAFE_MOUNT(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0);
> +	SAFE_MOUNT("testing", tmpdir, "tmpfs", MS_NOATIME | MS_NODEV, "");
                     ^
		     We tend to include ltp in names that are visible on
		     the system so that's clear where they came from.

		     It would be a bit better if the name of the mount
		     was "ltp-mount_setattr" or something that makes it clear
		     which test was mounting it.
> +	mounted = 1;
> +}
> +
> +static void run(void)
> +{
> +	struct mount_attr attr = {
> +		.attr_set       = MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOEXEC | MOUNT_ATTR_RELATIME,
> +		.attr_clr       = MOUNT_ATTR__ATIME,


I suppose that we can as well set these to zero as we are trying to test
only the propagation part. Or is there a reason why we are specifying
any attributes here?

> +	};
> +
> +	TST_EXP_PASS_SILENT(mount_setattr(-1, tmpdir, 0, &attr, sizeof(attr)));
> +	TST_EXP_EQ_LI(is_shared_mount(tmpdir), 0);
> +
> +	attr.propagation = -1;
> +	TST_EXP_FAIL_SILENT(mount_setattr(-1, tmpdir, 0, &attr, sizeof(attr)), EINVAL);
> +	TST_EXP_EQ_LI(is_shared_mount(tmpdir), 0);
> +
> +	attr.propagation = MS_SHARED;
> +	TST_EXP_PASS_SILENT(mount_setattr(-1, tmpdir, 0, &attr, sizeof(attr)));
> +	TST_EXP_EQ_LI(is_shared_mount(tmpdir), 1);

Here we should once more check with propagation = 0 and expect that the
mount is shared after that call.

> +	attr.propagation = MS_PRIVATE;
> +	TST_EXP_PASS_SILENT(mount_setattr(-1, tmpdir, 0, &attr, sizeof(attr)));
> +	TST_EXP_EQ_LI(is_shared_mount(tmpdir), 0);
> +
> +	attr.propagation = MS_SLAVE;
> +	TST_EXP_PASS_SILENT(mount_setattr(-1, tmpdir, 0, &attr, sizeof(attr)));
> +	TST_EXP_EQ_LI(is_shared_mount(tmpdir), 0);

Can we figure out slave mount from the mountinfo? I suppose there will
be slave in in the opts.

> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.needs_tmpdir = 1,
> +};
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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