[LTP] [PATCH] syscalls/openat2: New tests

[LTP] [PATCH] syscalls/openat2: New tests

[Viresh Kumar]

Add tests to check working of openat2() syscall.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 configure.ac                                  |  1 +
 include/lapi/openat2.h                        | 66 ++++++++++++++
 runtest/syscalls                              |  3 +
 testcases/kernel/syscalls/openat2/.gitignore  |  2 +
 testcases/kernel/syscalls/openat2/Makefile    |  9 ++
 testcases/kernel/syscalls/openat2/openat201.c | 87 +++++++++++++++++++
 testcases/kernel/syscalls/openat2/openat202.c | 62 +++++++++++++
 7 files changed, 230 insertions(+)
 create mode 100644 include/lapi/openat2.h
 create mode 100644 testcases/kernel/syscalls/openat2/.gitignore
 create mode 100644 testcases/kernel/syscalls/openat2/Makefile
 create mode 100644 testcases/kernel/syscalls/openat2/openat201.c
 create mode 100644 testcases/kernel/syscalls/openat2/openat202.c

diff --git a/configure.ac b/configure.ac
index c9ec39fce2df..238d1cde85f2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -96,6 +96,7 @@ AC_CHECK_FUNCS([ \
     name_to_handle_at \
     open_tree \
     openat \
+    openat2 \
     pidfd_open \
     pidfd_send_signal \
     pkey_mprotect \
diff --git a/include/lapi/openat2.h b/include/lapi/openat2.h
new file mode 100644
index 000000000000..2212a84c4d93
--- /dev/null
+++ b/include/lapi/openat2.h
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Linaro Limited. All rights reserved.
+ * Author: Viresh Kumar <viresh.kumar@linaro.org>
+ */
+
+#ifndef OPENAT2_H
+#define OPENAT2_H
+
+#include <sys/syscall.h>
+#include <linux/types.h>
+
+#include "lapi/syscalls.h"
+
+#include "config.h"
+
+#ifndef HAVE_OPENAT2
+/*
+ * Arguments for how openat2(2) should open the target path. If only @flags and
+ * @mode are non-zero, then openat2(2) operates very similarly to openat(2).
+ *
+ * However, unlike openat(2), unknown or invalid bits in @flags result in
+ * -EINVAL rather than being silently ignored. @mode must be zero unless one of
+ * {O_CREAT, O_TMPFILE} are set.
+ *
+ * @flags: O_* flags.
+ * @mode: O_CREAT/O_TMPFILE file mode.
+ * @resolve: RESOLVE_* flags.
+ */
+struct open_how {
+	__u64 flags;
+	__u64 mode;
+	__u64 resolve;
+};
+
+/* how->resolve flags for openat2(2). */
+#define RESOLVE_NO_XDEV                0x01 /* Block mount-point crossings
+                                       (includes bind-mounts). */
+#define RESOLVE_NO_MAGICLINKS  0x02 /* Block traversal through procfs-style
+                                       "magic-links". */
+#define RESOLVE_NO_SYMLINKS    0x04 /* Block traversal through all symlinks
+                                       (implies OEXT_NO_MAGICLINKS) */
+#define RESOLVE_BENEATH                0x08 /* Block "lexical" trickery like
+                                       "..", symlinks, and absolute
+                                       paths which escape the dirfd. */
+#define RESOLVE_IN_ROOT                0x10 /* Make all jumps to "/" and ".."
+                                       be scoped inside the dirfd
+                                       (similar to chroot(2)). */
+
+int openat2(int dfd, const char *pathname, struct open_how *how, size_t size)
+{
+	return tst_syscall(__NR_openat2, dfd, pathname, how, size);
+}
+#endif
+
+void openat2_supported_by_kernel(void)
+{
+	if ((tst_kvercmp(5, 6, 0)) < 0) {
+		/* Check if the syscall is backported on an older kernel */
+		TEST(syscall(__NR_openat2, -1, NULL, NULL, 0));
+		if (TST_RET == -1 && TST_ERR == ENOSYS)
+			tst_brk(TCONF, "Test not supported on kernel version < v5.2");
+	}
+}
+
+#endif /* OPENAT2_H */
diff --git a/runtest/syscalls b/runtest/syscalls
index 14df8d34338e..b85ba83a4095 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -845,6 +845,9 @@ openat01 openat01
 openat02 openat02
 openat03 openat03
 
+openat201 openat201
+openat202 openat202
+
 open_tree01 open_tree01
 open_tree02 open_tree02
 
diff --git a/testcases/kernel/syscalls/openat2/.gitignore b/testcases/kernel/syscalls/openat2/.gitignore
new file mode 100644
index 000000000000..3da61e6e40c1
--- /dev/null
+++ b/testcases/kernel/syscalls/openat2/.gitignore
@@ -0,0 +1,2 @@
+openat201
+openat202
diff --git a/testcases/kernel/syscalls/openat2/Makefile b/testcases/kernel/syscalls/openat2/Makefile
new file mode 100644
index 000000000000..c26cffd37f39
--- /dev/null
+++ b/testcases/kernel/syscalls/openat2/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+top_srcdir		?= ../../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+
+LDLIBS			+= $(AIO_LIBS)
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/openat2/openat201.c b/testcases/kernel/syscalls/openat2/openat201.c
new file mode 100644
index 000000000000..b35cea6725b3
--- /dev/null
+++ b/testcases/kernel/syscalls/openat2/openat201.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * Basic openat2() test.
+ */
+#include "tst_test.h"
+#include "lapi/openat2.h"
+
+#define TEST_FILE "test_file"
+#define TEST_DIR "test_dir"
+
+static int dir_fd = -1, fd_atcwd = AT_FDCWD;
+
+static struct tcase {
+	int *dfd;
+	const char *pathname;
+	__u64 flags;
+	__u64 mode;
+	__u64 resolve;
+} tcases[] = {
+	{&dir_fd, TEST_FILE, O_RDWR, S_IRWXU, 0},
+	{&dir_fd, TEST_FILE, O_RDONLY, S_IRUSR, 0},
+	{&dir_fd, TEST_FILE, O_WRONLY, S_IWUSR, 0},
+	{&dir_fd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_NO_XDEV},
+	{&dir_fd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_NO_MAGICLINKS},
+	{&dir_fd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_NO_SYMLINKS},
+	{&dir_fd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_BENEATH},
+	{&dir_fd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_IN_ROOT},
+	{&fd_atcwd, TEST_FILE, O_RDWR, S_IRWXU, 0},
+	{&fd_atcwd, TEST_FILE, O_RDONLY, S_IRUSR, 0},
+	{&fd_atcwd, TEST_FILE, O_WRONLY, S_IWUSR, 0},
+	{&fd_atcwd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_NO_XDEV},
+	{&fd_atcwd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_NO_MAGICLINKS},
+	{&fd_atcwd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_NO_SYMLINKS},
+	{&fd_atcwd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_BENEATH},
+	{&fd_atcwd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_IN_ROOT},
+};
+
+static void cleanup(void)
+{
+	if (dir_fd != -1)
+		SAFE_CLOSE(dir_fd);
+}
+
+static void setup(void)
+{
+	openat2_supported_by_kernel();
+
+	SAFE_MKDIR(TEST_DIR, 0700);
+	dir_fd = SAFE_OPEN(TEST_DIR, O_DIRECTORY);
+}
+
+static void run(unsigned int n)
+{
+	int fd;
+	struct stat file_stat;
+	struct tcase *tc = &tcases[n];
+	struct open_how how = {
+		.flags = tc->flags | O_CREAT,
+		.mode = tc->mode,
+		.resolve = tc->resolve
+	};
+
+	TEST(fd = openat2(*tc->dfd, tc->pathname, &how, sizeof(how)));
+	if (fd == -1) {
+		tst_res(TFAIL | TTERRNO, "openat2() failed (%d)", n);
+		return;
+	}
+
+	SAFE_FSTAT(fd, &file_stat);
+
+	if (file_stat.st_size == 0)
+		tst_res(TPASS, "openat2() passed (%d)", n);
+	else
+		tst_res(TFAIL, "fstat() didn't work as expected (%d)", n);
+
+	SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+	.tcnt = ARRAY_SIZE(tcases),
+	.test = run,
+	.setup = setup,
+	.cleanup = cleanup,
+	.needs_tmpdir = 1,
+};
diff --git a/testcases/kernel/syscalls/openat2/openat202.c b/testcases/kernel/syscalls/openat2/openat202.c
new file mode 100644
index 000000000000..aef2246d098d
--- /dev/null
+++ b/testcases/kernel/syscalls/openat2/openat202.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * Basic openat2() test to check various failures.
+ */
+#include "tst_test.h"
+#include "lapi/openat2.h"
+
+#define TEST_FILE "test_file"
+#define TEST_DIR "test_dir"
+
+static struct tcase {
+	int dfd;
+	const char *pathname;
+	__u64 flags;
+	__u64 mode;
+	__u64 resolve;
+	size_t size;
+	int exp_errno;
+} tcases[] = {
+	{-1, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, sizeof(struct open_how), EBADF},
+	{AT_FDCWD, NULL, O_RDONLY | O_CREAT, S_IRUSR, 0, sizeof(struct open_how), EFAULT},
+	{AT_FDCWD, TEST_FILE, O_RDONLY, S_IWUSR, 0, sizeof(struct open_how), EINVAL},
+	{AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, -1, 0, sizeof(struct open_how), EINVAL},
+	{AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, -1, sizeof(struct open_how), EINVAL},
+	{AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, 0, EINVAL},
+	{AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, 2 * sizeof(struct open_how), E2BIG},
+};
+
+static void run(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
+	struct open_how how = {
+		.flags = tc->flags,
+		.mode = tc->mode,
+		.resolve = tc->resolve
+	};
+
+	TEST(openat2(tc->dfd, tc->pathname, &how, tc->size));
+
+	if (TST_RET != -1) {
+		SAFE_CLOSE(TST_RET);
+		tst_res(TFAIL, "openat2() passed unexpectedly (%d)", n);
+		return;
+	}
+
+	if (tc->exp_errno != TST_ERR) {
+		tst_res(TFAIL | TTERRNO, "openat2() should fail with %s (%d)",
+			tst_strerrno(tc->exp_errno), n);
+		return;
+	}
+
+	tst_res(TPASS | TTERRNO, "openat2() failed as expected (%d)", n);
+}
+
+static struct tst_test test = {
+	.tcnt = ARRAY_SIZE(tcases),
+	.test = run,
+	.setup = openat2_supported_by_kernel,
+	.needs_tmpdir = 1,
+};
-- 
2.21.0.rc0.269.g1a574e7a288b

[LTP] [PATCH] syscalls/openat2: New tests

[Cyril Hrubis]

Hi!
> ---
>  configure.ac                                  |  1 +
>  include/lapi/openat2.h                        | 66 ++++++++++++++
>  runtest/syscalls                              |  3 +
>  testcases/kernel/syscalls/openat2/.gitignore  |  2 +
>  testcases/kernel/syscalls/openat2/Makefile    |  9 ++
>  testcases/kernel/syscalls/openat2/openat201.c | 87 +++++++++++++++++++
>  testcases/kernel/syscalls/openat2/openat202.c | 62 +++++++++++++
>  7 files changed, 230 insertions(+)
>  create mode 100644 include/lapi/openat2.h
>  create mode 100644 testcases/kernel/syscalls/openat2/.gitignore
>  create mode 100644 testcases/kernel/syscalls/openat2/Makefile
>  create mode 100644 testcases/kernel/syscalls/openat2/openat201.c
>  create mode 100644 testcases/kernel/syscalls/openat2/openat202.c
> 
> diff --git a/configure.ac b/configure.ac
> index c9ec39fce2df..238d1cde85f2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -96,6 +96,7 @@ AC_CHECK_FUNCS([ \
>      name_to_handle_at \
>      open_tree \
>      openat \
> +    openat2 \
>      pidfd_open \
>      pidfd_send_signal \
>      pkey_mprotect \
> diff --git a/include/lapi/openat2.h b/include/lapi/openat2.h
> new file mode 100644
> index 000000000000..2212a84c4d93
> --- /dev/null
> +++ b/include/lapi/openat2.h
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2020 Linaro Limited. All rights reserved.
> + * Author: Viresh Kumar <viresh.kumar@linaro.org>
> + */
> +
> +#ifndef OPENAT2_H
> +#define OPENAT2_H
> +
> +#include <sys/syscall.h>
> +#include <linux/types.h>
> +
> +#include "lapi/syscalls.h"
> +
> +#include "config.h"
> +
> +#ifndef HAVE_OPENAT2
> +/*
> + * Arguments for how openat2(2) should open the target path. If only @flags and
> + * @mode are non-zero, then openat2(2) operates very similarly to openat(2).
> + *
> + * However, unlike openat(2), unknown or invalid bits in @flags result in
> + * -EINVAL rather than being silently ignored. @mode must be zero unless one of
> + * {O_CREAT, O_TMPFILE} are set.
> + *
> + * @flags: O_* flags.
> + * @mode: O_CREAT/O_TMPFILE file mode.
> + * @resolve: RESOLVE_* flags.
> + */
> +struct open_how {
> +	__u64 flags;
> +	__u64 mode;
> +	__u64 resolve;
> +};
> +
> +/* how->resolve flags for openat2(2). */
> +#define RESOLVE_NO_XDEV                0x01 /* Block mount-point crossings
> +                                       (includes bind-mounts). */
> +#define RESOLVE_NO_MAGICLINKS  0x02 /* Block traversal through procfs-style
> +                                       "magic-links". */
> +#define RESOLVE_NO_SYMLINKS    0x04 /* Block traversal through all symlinks
> +                                       (implies OEXT_NO_MAGICLINKS) */
> +#define RESOLVE_BENEATH                0x08 /* Block "lexical" trickery like
> +                                       "..", symlinks, and absolute
> +                                       paths which escape the dirfd. */
> +#define RESOLVE_IN_ROOT                0x10 /* Make all jumps to "/" and ".."
> +                                       be scoped inside the dirfd
> +                                       (similar to chroot(2)). */
> +
> +int openat2(int dfd, const char *pathname, struct open_how *how, size_t size)
> +{
> +	return tst_syscall(__NR_openat2, dfd, pathname, how, size);
> +}
> +#endif
> +
> +void openat2_supported_by_kernel(void)
> +{
> +	if ((tst_kvercmp(5, 6, 0)) < 0) {
> +		/* Check if the syscall is backported on an older kernel */
> +		TEST(syscall(__NR_openat2, -1, NULL, NULL, 0));
> +		if (TST_RET == -1 && TST_ERR == ENOSYS)
> +			tst_brk(TCONF, "Test not supported on kernel version < v5.2");
> +	}
> +}
> +
> +#endif /* OPENAT2_H */
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 14df8d34338e..b85ba83a4095 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -845,6 +845,9 @@ openat01 openat01
>  openat02 openat02
>  openat03 openat03
>  
> +openat201 openat201
> +openat202 openat202
> +
>  open_tree01 open_tree01
>  open_tree02 open_tree02
>  
> diff --git a/testcases/kernel/syscalls/openat2/.gitignore b/testcases/kernel/syscalls/openat2/.gitignore
> new file mode 100644
> index 000000000000..3da61e6e40c1
> --- /dev/null
> +++ b/testcases/kernel/syscalls/openat2/.gitignore
> @@ -0,0 +1,2 @@
> +openat201
> +openat202
> diff --git a/testcases/kernel/syscalls/openat2/Makefile b/testcases/kernel/syscalls/openat2/Makefile
> new file mode 100644
> index 000000000000..c26cffd37f39
> --- /dev/null
> +++ b/testcases/kernel/syscalls/openat2/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +top_srcdir		?= ../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +
> +LDLIBS			+= $(AIO_LIBS)

What do we need the AIO_LIBS for? Is this copy&paste mistake?

> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/openat2/openat201.c b/testcases/kernel/syscalls/openat2/openat201.c
> new file mode 100644
> index 000000000000..b35cea6725b3
> --- /dev/null
> +++ b/testcases/kernel/syscalls/openat2/openat201.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org>
> + *
> + * Basic openat2() test.
> + */
> +#include "tst_test.h"
> +#include "lapi/openat2.h"
> +
> +#define TEST_FILE "test_file"
> +#define TEST_DIR "test_dir"
> +
> +static int dir_fd = -1, fd_atcwd = AT_FDCWD;
> +
> +static struct tcase {
> +	int *dfd;
> +	const char *pathname;
> +	__u64 flags;
> +	__u64 mode;
> +	__u64 resolve;

I slightly prefer using uint64_t in userspace code, but that's minor.

> +} tcases[] = {
> +	{&dir_fd, TEST_FILE, O_RDWR, S_IRWXU, 0},
> +	{&dir_fd, TEST_FILE, O_RDONLY, S_IRUSR, 0},
> +	{&dir_fd, TEST_FILE, O_WRONLY, S_IWUSR, 0},
> +	{&dir_fd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_NO_XDEV},
> +	{&dir_fd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_NO_MAGICLINKS},
> +	{&dir_fd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_NO_SYMLINKS},
> +	{&dir_fd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_BENEATH},
> +	{&dir_fd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_IN_ROOT},
> +	{&fd_atcwd, TEST_FILE, O_RDWR, S_IRWXU, 0},
> +	{&fd_atcwd, TEST_FILE, O_RDONLY, S_IRUSR, 0},
> +	{&fd_atcwd, TEST_FILE, O_WRONLY, S_IWUSR, 0},
> +	{&fd_atcwd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_NO_XDEV},
> +	{&fd_atcwd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_NO_MAGICLINKS},
> +	{&fd_atcwd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_NO_SYMLINKS},
> +	{&fd_atcwd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_BENEATH},
> +	{&fd_atcwd, TEST_FILE, O_RDWR, S_IRWXU, RESOLVE_IN_ROOT},
> +};
> +
> +static void cleanup(void)
> +{
> +	if (dir_fd != -1)
> +		SAFE_CLOSE(dir_fd);
> +}
> +
> +static void setup(void)
> +{
> +	openat2_supported_by_kernel();
> +
> +	SAFE_MKDIR(TEST_DIR, 0700);
> +	dir_fd = SAFE_OPEN(TEST_DIR, O_DIRECTORY);
> +}
> +
> +static void run(unsigned int n)
> +{
> +	int fd;
> +	struct stat file_stat;
> +	struct tcase *tc = &tcases[n];
> +	struct open_how how = {
> +		.flags = tc->flags | O_CREAT,
> +		.mode = tc->mode,
> +		.resolve = tc->resolve
> +	};

This structure should be allocated tst_buffers, see capget01.c for
example.

> +	TEST(fd = openat2(*tc->dfd, tc->pathname, &how, sizeof(how)));
> +	if (fd == -1) {

I would check here for fd < 0, because we would end up passing negative
fd to the stat() below in case of kernel bug.

> +		tst_res(TFAIL | TTERRNO, "openat2() failed (%d)", n);
> +		return;
> +	}
> +
> +	SAFE_FSTAT(fd, &file_stat);
> +
> +	if (file_stat.st_size == 0)
> +		tst_res(TPASS, "openat2() passed (%d)", n);
> +	else
> +		tst_res(TFAIL, "fstat() didn't work as expected (%d)", n);

So this is very basic test that just checks that openat() can open a
file and we would need a few more for each of the newly introduced
RESOLVE_* flags.

> +	SAFE_CLOSE(fd);
> +}
> +
> +static struct tst_test test = {
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_tmpdir = 1,
> +};
> diff --git a/testcases/kernel/syscalls/openat2/openat202.c b/testcases/kernel/syscalls/openat2/openat202.c
> new file mode 100644
> index 000000000000..aef2246d098d
> --- /dev/null
> +++ b/testcases/kernel/syscalls/openat2/openat202.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org>
> + *
> + * Basic openat2() test to check various failures.
> + */
> +#include "tst_test.h"
> +#include "lapi/openat2.h"
> +
> +#define TEST_FILE "test_file"
> +#define TEST_DIR "test_dir"

The TEST_DIR is not used in this test, right?

> +static struct tcase {
> +	int dfd;
> +	const char *pathname;
> +	__u64 flags;
> +	__u64 mode;
> +	__u64 resolve;

Here as well.

> +	size_t size;
> +	int exp_errno;
> +} tcases[] = {
> +	{-1, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, sizeof(struct open_how), EBADF},
> +	{AT_FDCWD, NULL, O_RDONLY | O_CREAT, S_IRUSR, 0, sizeof(struct open_how), EFAULT},
> +	{AT_FDCWD, TEST_FILE, O_RDONLY, S_IWUSR, 0, sizeof(struct open_how), EINVAL},
> +	{AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, -1, 0, sizeof(struct open_how), EINVAL},
> +	{AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, -1, sizeof(struct open_how), EINVAL},
> +	{AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, 0, EINVAL},
> +	{AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, 2 * sizeof(struct open_how), E2BIG},
                                                             ^
							     What
							     happends if
							     we pass
							     size
							     smaller
							     than the
							     sizeof(struct open_how) ?

Do we get EINVAL just like for 0?

> +};
> +
> +static void run(unsigned int n)
> +{
> +	struct tcase *tc = &tcases[n];
> +	struct open_how how = {
> +		.flags = tc->flags,
> +		.mode = tc->mode,
> +		.resolve = tc->resolve
> +	};

Here as well.

> +	TEST(openat2(tc->dfd, tc->pathname, &how, tc->size));
> +
> +	if (TST_RET != -1) {
> +		SAFE_CLOSE(TST_RET);
> +		tst_res(TFAIL, "openat2() passed unexpectedly (%d)", n);
> +		return;
> +	}
> +
> +	if (tc->exp_errno != TST_ERR) {
> +		tst_res(TFAIL | TTERRNO, "openat2() should fail with %s (%d)",
> +			tst_strerrno(tc->exp_errno), n);
> +		return;
> +	}
> +
> +	tst_res(TPASS | TTERRNO, "openat2() failed as expected (%d)", n);
                                                               ^
We usually keep short description in the test structure, something as
"invalid fd" so that we can use it in messages like this one.

> +}
> +
> +static struct tst_test test = {
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = run,
> +	.setup = openat2_supported_by_kernel,
> +	.needs_tmpdir = 1,
> +};
> -- 
> 2.21.0.rc0.269.g1a574e7a288b
> 

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/openat2: New tests

[Viresh Kumar]

On 28-02-20, 14:22, Cyril Hrubis wrote:
> > +static void run(unsigned int n)
> > +{
> > +	int fd;
> > +	struct stat file_stat;
> > +	struct tcase *tc = &tcases[n];
> > +	struct open_how how = {
> > +		.flags = tc->flags | O_CREAT,
> > +		.mode = tc->mode,
> > +		.resolve = tc->resolve
> > +	};
> 
> This structure should be allocated tst_buffers, see capget01.c for
> example.

This changed few things.

I am getting a build warning now (same happen if I build bpf stuff as
well). I don't understand why this warning comes though.

openat202.c:69:1: warning: missing initializer for field 'caps' of 'struct tst_test' [-Wmissing-field-initializers]
 };
 ^
In file included from openat202.c:7:0:
../../../../include/tst_test.h:236:18: note: 'caps' declared here
  struct tst_cap *caps;
                  ^~~~


Also for the failure test where larger size was passed, the error
reported now is EFAULT as kernel can't access out of bound dynamically
allocated memory (instead of stack one earlier). In order to get
E2BIG, I need to add some hacks (allocate more memory and write
non-zero value to excess memory) and I don't think that would be worth
it, so my test will expect EFAULT now.

> > +	TEST(fd = openat2(*tc->dfd, tc->pathname, &how, sizeof(how)));
> > +	if (fd == -1) {
> > +		tst_res(TFAIL | TTERRNO, "openat2() failed (%d)", n);
> > +		return;
> > +	}
> > +
> > +	SAFE_FSTAT(fd, &file_stat);
> > +
> > +	if (file_stat.st_size == 0)
> > +		tst_res(TPASS, "openat2() passed (%d)", n);
> > +	else
> > +		tst_res(TFAIL, "fstat() didn't work as expected (%d)", n);
> 
> So this is very basic test that just checks that openat() can open a
> file and we would need a few more for each of the newly introduced
> RESOLVE_* flags.

Hmm, this file is already testing openat2() with all different type of
resolve flags. What kind of further tests are you suggesting here ?

> > +	{AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, 2 * sizeof(struct open_how), E2BIG},
>                                                              ^
> 							     What
> 							     happends if
> 							     we pass
> 							     size
> 							     smaller
> 							     than the
> 							     sizeof(struct open_how) ?
> 
> Do we get EINVAL just like for 0?

Yes, added a test for that as well.

-- 
viresh

[LTP] [PATCH] syscalls/openat2: New tests

[Petr Vorel]

Hi Viresh,

> I am getting a build warning now (same happen if I build bpf stuff as
> well). I don't understand why this warning comes though.

> openat202.c:69:1: warning: missing initializer for field 'caps' of 'struct tst_test' [-Wmissing-field-initializers]
>  };
>  ^
> In file included from openat202.c:7:0:
> ../../../../include/tst_test.h:236:18: note: 'caps' declared here
>   struct tst_cap *caps;
>                   ^~~~

This is false positive warning. Unfortunately we don't know how to avoid it.

Kind regards,
Petr

[LTP] [PATCH] syscalls/openat2: New tests

[Cyril Hrubis]

Hi!
> > > +static void run(unsigned int n)
> > > +{
> > > +	int fd;
> > > +	struct stat file_stat;
> > > +	struct tcase *tc = &tcases[n];
> > > +	struct open_how how = {
> > > +		.flags = tc->flags | O_CREAT,
> > > +		.mode = tc->mode,
> > > +		.resolve = tc->resolve
> > > +	};
> > 
> > This structure should be allocated tst_buffers, see capget01.c for
> > example.
> 
> This changed few things.
> 
> I am getting a build warning now (same happen if I build bpf stuff as
> well). I don't understand why this warning comes though.
> 
> openat202.c:69:1: warning: missing initializer for field 'caps' of 'struct tst_test' [-Wmissing-field-initializers]
>  };
>  ^
> In file included from openat202.c:7:0:
> ../../../../include/tst_test.h:236:18: note: 'caps' declared here
>   struct tst_cap *caps;

The compiler is confused by different styles of initialization
apparently.

> Also for the failure test where larger size was passed, the error
> reported now is EFAULT as kernel can't access out of bound dynamically
> allocated memory (instead of stack one earlier). In order to get
> E2BIG, I need to add some hacks (allocate more memory and write
> non-zero value to excess memory) and I don't think that would be worth
> it, so my test will expect EFAULT now.

Hmm, I guess that it makes sense to add the pointer to how to the tcase
structure and allocate exact size for the E2BIG case. That should work
fine, right?

> > > +	TEST(fd = openat2(*tc->dfd, tc->pathname, &how, sizeof(how)));
> > > +	if (fd == -1) {
> > > +		tst_res(TFAIL | TTERRNO, "openat2() failed (%d)", n);
> > > +		return;
> > > +	}
> > > +
> > > +	SAFE_FSTAT(fd, &file_stat);
> > > +
> > > +	if (file_stat.st_size == 0)
> > > +		tst_res(TPASS, "openat2() passed (%d)", n);
> > > +	else
> > > +		tst_res(TFAIL, "fstat() didn't work as expected (%d)", n);
> > 
> > So this is very basic test that just checks that openat() can open a
> > file and we would need a few more for each of the newly introduced
> > RESOLVE_* flags.
> 
> Hmm, this file is already testing openat2() with all different type of
> resolve flags. What kind of further tests are you suggesting here ?

Well do not test that the flags actually works, right?

So for example for RESOLVE_BENATH we need to pass paths with ".." or
symlinks pointing upwards in the filesystem and expect openat2() to
fail. And the same for the rest of the flags.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/openat2: New tests

[Viresh Kumar]

On 02-03-20, 09:46, Cyril Hrubis wrote:
> Hmm, I guess that it makes sense to add the pointer to how to the tcase
> structure and allocate exact size for the E2BIG case. That should work
> fine, right?

We need to hack it a bit more to initialize rest of the memory as
well. This is what I did before I dropped the idea.

+static void setup(void)
+{
+       uint *temp;
+
+       openat2_supported_by_kernel();
+
+       temp = (uint *)(how + 1);
+       *temp = 0xDEAD;
+}
+
 static void run(unsigned int n)
 {
        struct tcase *tc = &tcases[n];
@@ -60,10 +70,10 @@ static void run(unsigned int n)
 static struct tst_test test = {
        .tcnt = ARRAY_SIZE(tcases),
        .test = run,
-       .setup = openat2_supported_by_kernel,
+       .setup = setup,
        .needs_tmpdir = 1,
        .bufs = (struct tst_buffers []) {
-               {&how, .size = sizeof(*how)},
+               {&how, .size = 2 * sizeof(*how)},
                {},
        }
 };

-- 
viresh

[LTP] [PATCH] syscalls/openat2: New tests

[Cyril Hrubis]

Hi!
> > Hmm, I guess that it makes sense to add the pointer to how to the tcase
> > structure and allocate exact size for the E2BIG case. That should work
> > fine, right?
> 
> We need to hack it a bit more to initialize rest of the memory as
> well. This is what I did before I dropped the idea.

Looking at the kernel the code that we are trying to trigger is
copy_struct_from_user() in include/linux/uacess.h. And indeed the area
after the kernel how structure has to be non-zero in order for the
structure to be deemed incompatible.

I guess that the easier way how to test this would be:

* Create an extended how structure
* Set bytes at the end of the extended structure to non-zero

The code you had there in the first place was passing by accident
because the were non-zero bytes on the stack after the structure, which
is pretty bad, because if it started to fail randomly nobody would know
why.

I guess that it would make sense to put this in a separate test and test
that everthing works fine as long as padd is zero and that we got a
failure with padd != 0.

So something as:

struct how_ext {
	uint64_t flags;
	uint64_t mode;
	uint64_t resolve;
	char padd[20];
};

Then setup the flags, mode and resolve and do two calls to openat2() one
with padd 0 => should work and one with pad != 0 should fail.

Does that sound reasonable?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/openat2: New tests

[Viresh Kumar]

On 02-03-20, 10:36, Cyril Hrubis wrote:
> The code you had there in the first place was passing by accident
> because the were non-zero bytes on the stack after the structure, which
> is pretty bad, because if it started to fail randomly nobody would know
> why.

I know :)

What about this? This doesn't test the success case with pad = 0 though, as it
is a success case. Don't want to add a separate file for it. :)

diff --git a/testcases/kernel/syscalls/openat2/openat202.c b/testcases/kernel/syscalls/openat2/openat202.c
index 6889c89ab68d..c945d279dcdf 100644
--- a/testcases/kernel/syscalls/openat2/openat202.c
+++ b/testcases/kernel/syscalls/openat2/openat202.c
@@ -9,7 +9,10 @@
 
 #define TEST_FILE "test_file"
 
-static struct open_how *how;
+struct open_how_pad {
+       struct open_how how;
+       uint64_t pad;
+} *phow;
 
 static struct tcase {
        const char *name;
@@ -18,28 +21,31 @@ static struct tcase {
        uint64_t flags;
        uint64_t mode;
        uint64_t resolve;
+       uint64_t pad;
        size_t size;
        int exp_errno;
 } tcases[] = {
-       {"invalid-dfd", -1, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, sizeof(struct open_how), EBADF},
-       {"invalid-pathname", AT_FDCWD, NULL, O_RDONLY | O_CREAT, S_IRUSR, 0, sizeof(struct open_how), EFAULT},
-       {"invalid-flags", AT_FDCWD, TEST_FILE, O_RDONLY, S_IWUSR, 0, sizeof(struct open_how), EINVAL},
-       {"invalid-mode", AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, -1, 0, sizeof(struct open_how), EINVAL},
-       {"invalid-resolve", AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, -1, sizeof(struct open_how), EINVAL},
-       {"invalid-size-0", AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, 0, EINVAL},
-       {"invalid-size-big", AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, 2 * sizeof(struct open_how), EFAULT},
-       {"invalid-size-small", AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, sizeof(struct open_how) - 1, EINVAL},
+       {"invalid-dfd", -1, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, 0, sizeof(struct open_how), EBADF},
+       {"invalid-pathname", AT_FDCWD, NULL, O_RDONLY | O_CREAT, S_IRUSR, 0, 0, sizeof(struct open_how), EFAULT},
+       {"invalid-flags", AT_FDCWD, TEST_FILE, O_RDONLY, S_IWUSR, 0, 0, sizeof(struct open_how), EINVAL},
+       {"invalid-mode", AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, -1, 0, 0, sizeof(struct open_how), EINVAL},
+       {"invalid-resolve", AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, -1, 0, sizeof(struct open_how), EINVAL},
+       {"invalid-size-0", AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, 0, 0, EINVAL},
+       {"invalid-size-big-with-pad", AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, 1, sizeof(struct open_how_pad), E2BIG},
+       {"invalid-size-big", AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, 0, 2 * sizeof(struct open_how_pad), EFAULT},
+       {"invalid-size-small", AT_FDCWD, TEST_FILE, O_RDWR | O_CREAT, S_IRWXU, 0, 0, sizeof(struct open_how) - 1, EINVAL},
 };
 
 static void run(unsigned int n)
 {
        struct tcase *tc = &tcases[n];
 
-       how->flags = tc->flags;
-       how->mode = tc->mode;
-       how->resolve = tc->resolve;
+       phow->how.flags = tc->flags;
+       phow->how.mode = tc->mode;
+       phow->how.resolve = tc->resolve;
+       phow->pad = tc->pad;
 
-       TEST(openat2(tc->dfd, tc->pathname, how, tc->size));
+       TEST(openat2(tc->dfd, tc->pathname, &phow->how, tc->size));
 
        if (TST_RET >= 0) {
                SAFE_CLOSE(TST_RET);
@@ -63,7 +69,7 @@ static struct tst_test test = {
        .setup = openat2_supported_by_kernel,
        .needs_tmpdir = 1,
        .bufs = (struct tst_buffers []) {
-               {&how, .size = sizeof(*how)},
+               {&phow, .size = sizeof(*phow)},
                {},
        }
 };

-- 
viresh

[LTP] [PATCH] syscalls/openat2: New tests

[Cyril Hrubis]

Hi!
> > The code you had there in the first place was passing by accident
> > because the were non-zero bytes on the stack after the structure, which
> > is pretty bad, because if it started to fail randomly nobody would know
> > why.
> 
> I know :)
> 
> What about this? This doesn't test the success case with pad = 0 though, as it
> is a success case. Don't want to add a separate file for it. :)

This has still one small drawback, the whole purpose of the test buffers
is that there is a PROT_NONE page allocated right behind the end of the
buffer. This is implemented so that we can make sure that kernel does
not touch any data outside of it. So the buffer has to be sized exactly
as the tc->size otherwise there is no point in using it. So I guess we
still need two buffers.

Also I guess that we can add the success case to the openat201.c test
but that would mean that we need two buffers there as well.

-- 
Cyril Hrubis
chrubis@suse.cz