[LTP] [PATCH] syscalls/file_attr01: Allow EOPNOTSUPP when attributes is NULL

[LTP] [PATCH] syscalls/file_attr01: Allow EOPNOTSUPP when attributes is NULL

[Wake Liu via ltp]

The syscalls file_getattr and file_setattr may return EOPNOTSUPP
instead of EFAULT when the ufattr argument is NULL, specifically on
filesystems that do not support extended attributes (e.g., tmpfs when
CONFIG_TMPFS_XATTR is disabled).

In the kernel, the fileattr_get/set operations are checked before the
pointer dereference. If the filesystem does not define these operations,
the kernel returns -ENOIOCTLCMD (translated to EOPNOTSUPP in userspace)
before checking the validity of the user-provided pointer.

This is a valid behavior for kernel configurations where extended
attributes are not enabled for certain filesystems. This leads to
test failures on such systems:
TFAIL: File attributes is NULL expected EFAULT: EOPNOTSUPP (95)

This patch updates the test case to support multiple expected errnos
using TST_EXP_FAIL_ARR, allowing both EFAULT and EOPNOTSUPP for the NULL
attributes test case.

Signed-off-by: Wake Liu <wakel@google.com>
---
 .../kernel/syscalls/file_attr/file_attr01.c   | 35 ++++++++++++-------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/testcases/kernel/syscalls/file_attr/file_attr01.c b/testcases/kernel/syscalls/file_attr/file_attr01.c
index c9c9288a1..02aea66a6 100644
--- a/testcases/kernel/syscalls/file_attr/file_attr01.c
+++ b/testcases/kernel/syscalls/file_attr/file_attr01.c
@@ -42,7 +42,8 @@ static struct tcase {
 	struct file_attr **ufattr;
 	size_t *usize;
 	int at_flags;
-	int exp_errno;
+	int exp_errs[2];
+	int exp_errs_cnt;
 	char *msg;
 } tcases[] = {
 	{
@@ -50,7 +51,8 @@ static struct tcase {
 		.filename = &valid_filename,
 		.ufattr = &valid_file_attr,
 		.usize = &valid_usize,
-		.exp_errno = EBADF,
+		.exp_errs = {EBADF},
+		.exp_errs_cnt = 1,
 		.msg = "Invalid file descriptor",
 	},
 	{
@@ -58,7 +60,8 @@ static struct tcase {
 		.filename = &invalid_filename,
 		.ufattr = &valid_file_attr,
 		.usize = &valid_usize,
-		.exp_errno = ENOENT,
+		.exp_errs = {ENOENT},
+		.exp_errs_cnt = 1,
 		.msg = "File doesn't exist",
 	},
 	{
@@ -66,7 +69,8 @@ static struct tcase {
 		.filename = &null_ptr,
 		.ufattr = &valid_file_attr,
 		.usize = &valid_usize,
-		.exp_errno = EFAULT,
+		.exp_errs = {EFAULT},
+		.exp_errs_cnt = 1,
 		.msg = "Filename is NULL",
 	},
 	{
@@ -74,7 +78,8 @@ static struct tcase {
 		.filename = &valid_filename,
 		.ufattr = (struct file_attr **)(&null_ptr),
 		.usize = &valid_usize,
-		.exp_errno = EFAULT,
+		.exp_errs = {EFAULT, EOPNOTSUPP},
+		.exp_errs_cnt = 2,
 		.msg = "File attributes is NULL",
 	},
 	{
@@ -82,7 +87,8 @@ static struct tcase {
 		.filename = &valid_filename,
 		.ufattr = &valid_file_attr,
 		.usize = &zero,
-		.exp_errno = EINVAL,
+		.exp_errs = {EINVAL},
+		.exp_errs_cnt = 1,
 		.msg = "File attributes size is zero",
 	},
 	{
@@ -90,7 +96,8 @@ static struct tcase {
 		.filename = &valid_filename,
 		.ufattr = &valid_file_attr,
 		.usize = &small_usize,
-		.exp_errno = EINVAL,
+		.exp_errs = {EINVAL},
+		.exp_errs_cnt = 1,
 		.msg = "File attributes size is too small",
 	},
 	{
@@ -98,7 +105,8 @@ static struct tcase {
 		.filename = &valid_filename,
 		.ufattr = &valid_file_attr,
 		.usize = &big_usize,
-		.exp_errno = E2BIG,
+		.exp_errs = {E2BIG},
+		.exp_errs_cnt = 1,
 		.msg = "File attributes size is too big",
 	},
 	{
@@ -107,7 +115,8 @@ static struct tcase {
 		.ufattr = &valid_file_attr,
 		.usize = &valid_usize,
 		.at_flags = -1,
-		.exp_errno = EINVAL,
+		.exp_errs = {EINVAL},
+		.exp_errs_cnt = 1,
 		.msg = "Invalid AT flags",
 	},
 };
@@ -117,18 +126,18 @@ static void run(unsigned int i)
 	struct tcase *tc = &tcases[i];
 
 	if (tst_variant) {
-		TST_EXP_FAIL(file_getattr(
+		TST_EXP_FAIL_ARR(file_getattr(
 			*tc->dfd, *tc->filename,
 			*tc->ufattr, *tc->usize,
 			tc->at_flags),
-			tc->exp_errno,
+			tc->exp_errs, tc->exp_errs_cnt,
 			"%s", tc->msg);
 	} else {
-		TST_EXP_FAIL(file_setattr(
+		TST_EXP_FAIL_ARR(file_setattr(
 			*tc->dfd, *tc->filename,
 			*tc->ufattr, *tc->usize,
 			tc->at_flags),
-			tc->exp_errno,
+			tc->exp_errs, tc->exp_errs_cnt,
 			"%s", tc->msg);
 	}
 }
-- 
2.53.0.983.g0bb29b3bc5-goog


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

Re: [LTP] [PATCH] syscalls/file_attr01: Allow EOPNOTSUPP when attributes is NULL

[Andrea Cervesato via ltp]

Hi Wake,

> The syscalls file_getattr and file_setattr may return EOPNOTSUPP
> instead of EFAULT when the ufattr argument is NULL, specifically on
> filesystems that do not support extended attributes (e.g., tmpfs when
> CONFIG_TMPFS_XATTR is disabled).

Patch is technically correct but EOPNOTSUPP should not be tested int this way,
since we are not checking the underlying kernel configuration. For instance,
we might have xattr support, but getting EOPNOTSUPP and have a TPASS. This
would be a kernel bug hidden by the test structure.

In other circumstances (if xattr was our testing goal) we might have used
.needs_kconfig, but in this case we should use a different approach:

- we verify at runtime that our kernel supports xattr via tst_kconfig_check()
- if kernel doesn't support xattr, we expect EOPNOTSUPP. Otherwise, EFAULT

In this way we make sure that syscalls are raising the proper error according
to the kernel configuration.

WDYT?

--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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

Re: [LTP] [PATCH] syscalls/file_attr01: Allow EOPNOTSUPP when attributes is NULL

[Wake Liu via ltp]

> Hi Wake,
>
> > The syscalls file_getattr and file_setattr may return EOPNOTSUPP
> > instead of EFAULT when the ufattr argument is NULL, specifically on
> > filesystems that do not support extended attributes (e.g., tmpfs when
> > CONFIG_TMPFS_XATTR is disabled).
>
> Patch is technically correct but EOPNOTSUPP should not be tested int this way,
> since we are not checking the underlying kernel configuration. For instance,
> we might have xattr support, but getting EOPNOTSUPP and have a TPASS. This
> would be a kernel bug hidden by the test structure.
>
> In other circumstances (if xattr was our testing goal) we might have used
> .needs_kconfig, but in this case we should use a different approach:
>
> - we verify at runtime that our kernel supports xattr via tst_kconfig_check()
> - if kernel doesn't support xattr, we expect EOPNOTSUPP. Otherwise, EFAULT
>
> In this way we make sure that syscalls are raising the proper error according
> to the kernel configuration.
>
> WDYT?
>
> --
> Andrea Cervesato
> SUSE QE Automation Engineer Linux
> andrea.cervesato@suse.com

Hi Andrea,

Thanks for the quick review and the detailed feedback.

I agree with your assessment that allowing EOPNOTSUPP unconditionally
could potentially mask a kernel bug if xattr support is present but
returns the wrong error. That's definitely something we should avoid.

Your suggested approach using tst_kconfig_check() at runtime to verify
kernel xattr support is much more robust and is the correct way to
handle this. It ensures the syscalls raise the proper error based on
the configuration.

I will revise the patch (v2) to implement this logic:

*   Check xattr support via tst_kconfig_check().
*   If xattr is not supported, expect EOPNOTSUPP.
*   Otherwise, expect EFAULT.

I'll send it out shortly.

Thanks again,


-- 
Best Regards,
Wake Liu

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

[LTP] [PATCH v2] syscalls/file_attr01: Dynamically expect EOPNOTSUPP on tmpfs without xattr

[Wake Liu via ltp]

The syscalls file_getattr and file_setattr return EOPNOTSUPP instead of
EFAULT when the ufattr argument is NULL on tmpfs without xattr support
(CONFIG_TMPFS_XATTR=n). This is because the kernel checks for the
filesystem operation support before dereferencing the user pointer.

This patch adds a runtime check for CONFIG_TMPFS_XATTR when testing
on tmpfs, ensuring the correct errno is expected based on the kernel
configuration. This prevents hiding potential kernel bugs where
EOPNOTSUPP might be returned even when xattr is supported.

Changes in v2:
- Replaced the TST_EXP_FAIL_ARR approach with a dynamic check in run().
- Included tst_kconfig.h and used tst_kconfig_check() to verify
  CONFIG_TMPFS_XATTR status.
- Expected EOPNOTSUPP only on tmpfs when xattr support is missing,
  otherwise default to EFAULT.

Signed-off-by: Wake Liu <wakel@google.com>
---
 testcases/kernel/syscalls/file_attr/file_attr01.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/syscalls/file_attr/file_attr01.c b/testcases/kernel/syscalls/file_attr/file_attr01.c
index c9c9288a1..43e0f0503 100644
--- a/testcases/kernel/syscalls/file_attr/file_attr01.c
+++ b/testcases/kernel/syscalls/file_attr/file_attr01.c
@@ -18,6 +18,7 @@
 
 #include <string.h>
 #include "tst_test.h"
+#include "tst_kconfig.h"
 #include "lapi/fs.h"
 #include "lapi/fcntl.h"
 
@@ -115,20 +116,28 @@ static struct tcase {
 static void run(unsigned int i)
 {
 	struct tcase *tc = &tcases[i];
+	int exp_errno = tc->exp_errno;
+
+	if (tc->ufattr == (struct file_attr **)(&null_ptr)) {
+		const char *const kconfig[] = {"CONFIG_TMPFS_XATTR=y", NULL};
+
+		if (!strcmp(tst_device->fs_type, "tmpfs") && tst_kconfig_check(kconfig))
+			exp_errno = EOPNOTSUPP;
+	}
 
 	if (tst_variant) {
 		TST_EXP_FAIL(file_getattr(
 			*tc->dfd, *tc->filename,
 			*tc->ufattr, *tc->usize,
 			tc->at_flags),
-			tc->exp_errno,
+			exp_errno,
 			"%s", tc->msg);
 	} else {
 		TST_EXP_FAIL(file_setattr(
 			*tc->dfd, *tc->filename,
 			*tc->ufattr, *tc->usize,
 			tc->at_flags),
-			tc->exp_errno,
+			exp_errno,
 			"%s", tc->msg);
 	}
 }
-- 
2.53.0.983.g0bb29b3bc5-goog


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

Re: [LTP] [PATCH v2] syscalls/file_attr01: Dynamically expect EOPNOTSUPP on tmpfs without xattr

[Andrea Cervesato via ltp]

LGTM

Reviewed-by: Andrea Cervesato <andrea.cervesato@suse.com>

--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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