diff mbox series

[v2,04/10] setxattr02: add setxattrat variant

Message ID 20251007-xattrat-v2-4-bf458fa66358@suse.com
State Changes Requested
Delegated to: Andrea Cervesato
Headers show
Series setxattrat coverage | expand

Checks

Context Check Description
ltpci/debian_oldstable_gcc fail failure
ltpci/debian_stable_s390x-linux-gnu-gcc_s390x success success
ltpci/debian_stable_powerpc64le-linux-gnu-gcc_ppc64el success success
ltpci/debian_stable_aarch64-linux-gnu-gcc_arm64 success success
ltpci/ubuntu_jammy_gcc success success
ltpci/quay-io-centos-centos_stream9_gcc success success
ltpci/debian_stable_gcc success success
ltpci/ubuntu_bionic_gcc success success
ltpci/opensuse-leap_latest_gcc success success
ltpci/debian_testing_clang success success
ltpci/fedora_latest_clang success success
ltpci/alpine_latest_gcc success success
ltpci/debian_testing_gcc success success
ltpci/debian_oldstable_clang success success
ltpci/debian_stable_gcc success success
ltpci/opensuse-archive_42-2_gcc success success
ltpci/debian_oldstable_gcc success success

Commit Message

Andrea Cervesato Oct. 7, 2025, 6:46 a.m. UTC 
From: Andrea Cervesato <andrea.cervesato@suse.com>

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 testcases/kernel/syscalls/setxattr/setxattr02.c | 79 +++++++++++++++++++------
 1 file changed, 60 insertions(+), 19 deletions(-)

Comments

Cyril Hrubis Oct. 7, 2025, 12:40 p.m. UTC | #1 
On Tue, Oct 07, 2025 at 08:46:56AM +0200, Andrea Cervesato wrote:
> From: Andrea Cervesato <andrea.cervesato@suse.com>
> 
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  testcases/kernel/syscalls/setxattr/setxattr02.c | 79 +++++++++++++++++++------
>  1 file changed, 60 insertions(+), 19 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/setxattr/setxattr02.c b/testcases/kernel/syscalls/setxattr/setxattr02.c
> index 9f5f998da..b5042a0df 100644
> --- a/testcases/kernel/syscalls/setxattr/setxattr02.c
> +++ b/testcases/kernel/syscalls/setxattr/setxattr02.c
> @@ -19,6 +19,10 @@
>   */
>  
>  #include "config.h"
> +#include "tst_test.h"
> +
> +#ifdef HAVE_SYS_XATTR_H
> +
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/sysmacros.h>
> @@ -30,12 +34,10 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> -#ifdef HAVE_SYS_XATTR_H
> -# include <sys/xattr.h>
> -#endif
> -#include "tst_test.h"
> +#include "lapi/xattr.h"
> +#include "lapi/fcntl.h"
> +#include <sys/xattr.h>
>  
> -#ifdef HAVE_SYS_XATTR_H
>  #define XATTR_TEST_KEY "user.testkey"
>  #define XATTR_TEST_VALUE "this is a test value"
>  #define XATTR_TEST_VALUE_SIZE 20
> @@ -49,6 +51,8 @@
>  #define BLK      "setxattr02blk"
>  #define SOCK     "setxattr02sock"
>  
> +static int tmpdir_fd = -1;
> +
>  struct test_case {
>  	char *fname;
>  	char *key;
> @@ -120,39 +124,58 @@ static struct test_case tc[] = {
>  
>  static void verify_setxattr(unsigned int i)
>  {
> +	char *sysname;
> +
>  	/* some tests might require existing keys for each iteration */
>  	if (tc[i].needskeyset) {
>  		SAFE_SETXATTR(tc[i].fname, tc[i].key, tc[i].value, tc[i].size,
> -				XATTR_CREATE);
> +			XATTR_CREATE);
>  	}
>  
> -	TEST(setxattr(tc[i].fname, tc[i].key, tc[i].value, tc[i].size,
> -			tc[i].flags));
> +	if (tst_variant) {
> +		sysname = "setxattrat";
> +
> +		struct xattr_args args = {
> +			.value = (uint64_t)tc[i].value,
> +			.size = tc[i].size,
> +			.flags = tc[i].flags,
> +		};
> +
> +		int at_flags = tc[i].needskeyset ? 0 : AT_SYMLINK_NOFOLLOW;


I do not get why this is needed.

In kernel:

SYSCALL_DEFINE5(setxattr, const char __user *, pathname,
                const char __user *, name, const void __user *, value,
                size_t, size, int, flags)
{
        return path_setxattrat(AT_FDCWD, pathname, 0, name, value, size, flags);
	                                           ^
						   the setxattr() the
						   always sets the
						   at_flags to 0
}


So shouldn't setxattrat() just work the same if we pass 0 there?

It's the lsetxattr() syscall that passes AT_SYMLINK_NOFOLLOW:

SYSCALL_DEFINE5(lsetxattr, const char __user *, pathname,
                const char __user *, name, const void __user *, value,
                size_t, size, int, flags)
{
        return path_setxattrat(AT_FDCWD, pathname, AT_SYMLINK_NOFOLLOW, name,
                               value, size, flags);
}

>  static void setup(void)
> @@ -185,12 +208,30 @@ static void setup(void)
>  	SAFE_MKNOD(CHR, S_IFCHR | 0777, dev);
>  	SAFE_MKNOD(BLK, S_IFBLK | 0777, 0);
>  	SAFE_MKNOD(SOCK, S_IFSOCK | 0777, 0);
> +
> +	tmpdir_fd = SAFE_OPEN(tst_tmpdir_path(), O_DIRECTORY);

This is memleak, on the top of that can't we just use the AT_FDCWD
instead? Or if you want to make sure that the syscall works with a real
fd we can do SAFE_OPEN(".", O_DIRECTORY) instead....
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/setxattr/setxattr02.c b/testcases/kernel/syscalls/setxattr/setxattr02.c
index 9f5f998da..b5042a0df 100644
--- a/testcases/kernel/syscalls/setxattr/setxattr02.c
+++ b/testcases/kernel/syscalls/setxattr/setxattr02.c
@@ -19,6 +19,10 @@ 
  */
 
 #include "config.h"
+#include "tst_test.h"
+
+#ifdef HAVE_SYS_XATTR_H
+
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/sysmacros.h>
@@ -30,12 +34,10 @@ 
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#ifdef HAVE_SYS_XATTR_H
-# include <sys/xattr.h>
-#endif
-#include "tst_test.h"
+#include "lapi/xattr.h"
+#include "lapi/fcntl.h"
+#include <sys/xattr.h>
 
-#ifdef HAVE_SYS_XATTR_H
 #define XATTR_TEST_KEY "user.testkey"
 #define XATTR_TEST_VALUE "this is a test value"
 #define XATTR_TEST_VALUE_SIZE 20
@@ -49,6 +51,8 @@ 
 #define BLK      "setxattr02blk"
 #define SOCK     "setxattr02sock"
 
+static int tmpdir_fd = -1;
+
 struct test_case {
 	char *fname;
 	char *key;
@@ -120,39 +124,58 @@  static struct test_case tc[] = {
 
 static void verify_setxattr(unsigned int i)
 {
+	char *sysname;
+
 	/* some tests might require existing keys for each iteration */
 	if (tc[i].needskeyset) {
 		SAFE_SETXATTR(tc[i].fname, tc[i].key, tc[i].value, tc[i].size,
-				XATTR_CREATE);
+			XATTR_CREATE);
 	}
 
-	TEST(setxattr(tc[i].fname, tc[i].key, tc[i].value, tc[i].size,
-			tc[i].flags));
+	if (tst_variant) {
+		sysname = "setxattrat";
+
+		struct xattr_args args = {
+			.value = (uint64_t)tc[i].value,
+			.size = tc[i].size,
+			.flags = tc[i].flags,
+		};
+
+		int at_flags = tc[i].needskeyset ? 0 : AT_SYMLINK_NOFOLLOW;
+
+		TEST(setxattrat(tmpdir_fd, tc[i].fname, at_flags,
+			tc[i].key, &args, sizeof(args)));
+	} else {
+		sysname = "setxattr";
+
+		TEST(setxattr(tc[i].fname, tc[i].key, tc[i].value, tc[i].size,
+				tc[i].flags));
+	}
 
 	if (TST_RET == -1 && TST_ERR == EOPNOTSUPP)
-		tst_brk(TCONF, "setxattr(2) not supported");
+		tst_brk(TCONF, "%s(2) not supported", sysname);
 
 	/* success */
 
 	if (!tc[i].exp_err) {
 		if (TST_RET) {
 			tst_res(TFAIL | TTERRNO,
-				"setxattr(2) on %s failed with %li",
-				tc[i].fname + OFFSET, TST_RET);
+				"%s(2) on %s failed with %li",
+				sysname, tc[i].fname + OFFSET, TST_RET);
 			return;
 		}
 
 		/* this is needed for subsequent iterations */
 		SAFE_REMOVEXATTR(tc[i].fname, tc[i].key);
 
-		tst_res(TPASS, "setxattr(2) on %s passed",
-				tc[i].fname + OFFSET);
+		tst_res(TPASS, "%s(2) on %s passed",
+				sysname, tc[i].fname + OFFSET);
 		return;
 	}
 
 	if (TST_RET == 0) {
-		tst_res(TFAIL, "setxattr(2) on %s passed unexpectedly",
-				tc[i].fname + OFFSET);
+		tst_res(TFAIL, "%s(2) on %s passed unexpectedly",
+				sysname, tc[i].fname + OFFSET);
 		return;
 	}
 
@@ -160,8 +183,8 @@  static void verify_setxattr(unsigned int i)
 
 	if (tc[i].exp_err != TST_ERR) {
 		tst_res(TFAIL | TTERRNO,
-				"setxattr(2) on %s should have failed with %s",
-				tc[i].fname + OFFSET,
+				"%s(2) on %s should have failed with %s",
+				sysname, tc[i].fname + OFFSET,
 				tst_strerrno(tc[i].exp_err));
 		return;
 	}
@@ -170,8 +193,8 @@  static void verify_setxattr(unsigned int i)
 	if (tc[i].needskeyset)
 		SAFE_REMOVEXATTR(tc[i].fname, tc[i].key);
 
-	tst_res(TPASS | TTERRNO, "setxattr(2) on %s failed",
-			tc[i].fname + OFFSET);
+	tst_res(TPASS | TTERRNO, "%s(2) on %s failed",
+			sysname, tc[i].fname + OFFSET);
 }
 
 static void setup(void)
@@ -185,12 +208,30 @@  static void setup(void)
 	SAFE_MKNOD(CHR, S_IFCHR | 0777, dev);
 	SAFE_MKNOD(BLK, S_IFBLK | 0777, 0);
 	SAFE_MKNOD(SOCK, S_IFSOCK | 0777, 0);
+
+	tmpdir_fd = SAFE_OPEN(tst_tmpdir_path(), O_DIRECTORY);
+}
+
+static void cleanup(void)
+{
+	if (tmpdir_fd != -1)
+		SAFE_CLOSE(tmpdir_fd);
+
+	SAFE_UNLINK(FILENAME);
+	SAFE_RMDIR(DIRNAME);
+	SAFE_UNLINK(SYMLINK);
+	SAFE_UNLINK(FIFO);
+	SAFE_UNLINK(CHR);
+	SAFE_UNLINK(BLK);
+	SAFE_UNLINK(SOCK);
 }
 
 static struct tst_test test = {
 	.setup = setup,
+	.cleanup = cleanup,
 	.test = verify_setxattr,
 	.tcnt = ARRAY_SIZE(tc),
+	.test_variants = 2,
 	.needs_tmpdir = 1,
 	.needs_root = 1,
 };