[LTP] [PATCH v1 0/2] fcntl{34,36}: Fixes for Android arm64

[LTP] [PATCH v1 0/2] fcntl{34,36}: Fixes for Android arm64

[Edward Liaw via ltp]

Fixes: 7643115aaacb ("fcntl{34,36}: Always use 64-bit flock struct to avoid EINVAL")

In Richard's commit I think that he had meant to write that the flock64
type (not fcntl64 type) had been removed from some libcs.  I believe
that was the reason why he added the my_flock64 type.

On Android arm64, this test was breaking for two reasons:

1. The my_flock64 type definition did not match the expected type when
compiled for 64 bits.
2. The test was mixing fcntl and the flock64 struct when compiled for 32
bits.

Both seem to be ok on x86_64 but not on arm64.  To fix it, I gated the
compat function on TST_ABI64 instead of tst_kernel_bits.

Also, I'm not sure that the my_flock64 struct is needed with these
patches.  I think that l64 can just have the flock64 type.

Edward Liaw (2):
  fcntl{34,36}: Only use fcntl64 with 32bit abi
  fcntl{34,36}: Use arch dependent types for my_flock64

 .../kernel/syscalls/fcntl/fcntl_common.h      | 24 ++++++++++++-------
 1 file changed, 15 insertions(+), 9 deletions(-)

--
2.40.1.495.gc816e09b53d-goog


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

[LTP] [PATCH v1 1/2] fcntl{34, 36}: Only use fcntl64 with 32bit abi

[Edward Liaw via ltp]

Fixes: 7643115aaacb ("fcntl{34,36}: Always use 64-bit flock struct to avoid EINVAL")

On Android arm64, tst_kernel_bits is disregarding the abi, so compiling
with the 32bit abi is calling the fcntl syscall instead of fcntl64.  The
fcntl syscall is not compatible with the flock64 struct being passed
(this doesn't seem to be the case with x86_64, only with arm64).

This changes it to only use the fcntl64 compat syscall with the 32bit
abi.

Signed-off-by: Edward Liaw <edliaw@google.com>
---
 testcases/kernel/syscalls/fcntl/fcntl_common.h | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/testcases/kernel/syscalls/fcntl/fcntl_common.h b/testcases/kernel/syscalls/fcntl/fcntl_common.h
index 5c130a784..485a31367 100644
--- a/testcases/kernel/syscalls/fcntl/fcntl_common.h
+++ b/testcases/kernel/syscalls/fcntl/fcntl_common.h
@@ -10,6 +10,11 @@
 #include "lapi/abisize.h"
 #include "lapi/fcntl.h"
 
+#if defined(TST_ABI64)
+#define FCNTL_COMPAT(fd, cmd, flock) \
+	SAFE_FCNTL(fd, cmd, flock)
+
+#else
 struct my_flock64 {
 	int16_t l_type;
 	int16_t l_whence;
@@ -43,8 +48,8 @@ static inline int fcntl_compat(const char *file, const int line, const char *cmd
 		.l_len = lck->l_len,
 		.l_pid = lck->l_pid,
 	};
-	const int sysno = tst_kernel_bits() > 32 ? __NR_fcntl : __NR_fcntl64;
-	const int ret = tst_syscall(sysno, fd, cmd, &l64);
+
+	const int ret = tst_syscall(__NR_fcntl64, fd, cmd, &l64);
 
 	lck->l_type = l64.l_type;
 	lck->l_whence = l64.l_whence;
@@ -57,7 +62,7 @@ static inline int fcntl_compat(const char *file, const int line, const char *cmd
 
 	tst_brk_(file, line, TBROK | TERRNO,
 		 "%s(%d, %s, { %d, %d, %"PRId64", %"PRId64", %d })",
-		 tst_kernel_bits() > 32 ? "fcntl" : "fcntl64",
+		 "fcntl64",
 		 fd,
 		 cmd_name,
 		 l64.l_type, l64.l_whence, l64.l_start, l64.l_len, l64.l_pid);
@@ -67,5 +72,6 @@ static inline int fcntl_compat(const char *file, const int line, const char *cmd
 
 #define FCNTL_COMPAT(fd, cmd, flock) \
 	fcntl_compat(__FILE__, __LINE__, #cmd, fd, cmd, flock)
+#endif
 
 #endif /* FCNTL_COMMON_H__ */
-- 
2.40.1.495.gc816e09b53d-goog


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

Re: [LTP] [PATCH v1 1/2] fcntl{34, 36}: Only use fcntl64 with 32bit abi

[Richard Palethorpe]

Hello,

Edward Liaw via ltp <ltp@lists.linux.it> writes:

> Fixes: 7643115aaacb ("fcntl{34,36}: Always use 64-bit flock struct to avoid EINVAL")
>
> On Android arm64, tst_kernel_bits is disregarding the abi, so compiling
> with the 32bit abi is calling the fcntl syscall instead of fcntl64.
> The

IIRC that's the idea because fcntl64 doesn't exist on a 64bit
kernel.

So if you compile the test with -m32/32bit ABI on x86_64 64bit kernel
then you will get ENOSYS? If not then I suppose it is fine.

> fcntl syscall is not compatible with the flock64 struct being passed
> (this doesn't seem to be the case with x86_64, only with arm64).
>
> This changes it to only use the fcntl64 compat syscall with the 32bit
> abi.
>
> Signed-off-by: Edward Liaw <edliaw@google.com>
> ---
>  testcases/kernel/syscalls/fcntl/fcntl_common.h | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/fcntl/fcntl_common.h b/testcases/kernel/syscalls/fcntl/fcntl_common.h
> index 5c130a784..485a31367 100644
> --- a/testcases/kernel/syscalls/fcntl/fcntl_common.h
> +++ b/testcases/kernel/syscalls/fcntl/fcntl_common.h
> @@ -10,6 +10,11 @@
>  #include "lapi/abisize.h"
>  #include "lapi/fcntl.h"
>  
> +#if defined(TST_ABI64)
> +#define FCNTL_COMPAT(fd, cmd, flock) \
> +	SAFE_FCNTL(fd, cmd, flock)
> +
> +#else
>  struct my_flock64 {
>  	int16_t l_type;
>  	int16_t l_whence;
> @@ -43,8 +48,8 @@ static inline int fcntl_compat(const char *file, const int line, const char *cmd
>  		.l_len = lck->l_len,
>  		.l_pid = lck->l_pid,
>  	};
> -	const int sysno = tst_kernel_bits() > 32 ? __NR_fcntl : __NR_fcntl64;
> -	const int ret = tst_syscall(sysno, fd, cmd, &l64);
> +
> +	const int ret = tst_syscall(__NR_fcntl64, fd, cmd, &l64);
>  
>  	lck->l_type = l64.l_type;
>  	lck->l_whence = l64.l_whence;
> @@ -57,7 +62,7 @@ static inline int fcntl_compat(const char *file, const int line, const char *cmd
>  
>  	tst_brk_(file, line, TBROK | TERRNO,
>  		 "%s(%d, %s, { %d, %d, %"PRId64", %"PRId64", %d })",
> -		 tst_kernel_bits() > 32 ? "fcntl" : "fcntl64",
> +		 "fcntl64",
>  		 fd,
>  		 cmd_name,
>  		 l64.l_type, l64.l_whence, l64.l_start, l64.l_len, l64.l_pid);
> @@ -67,5 +72,6 @@ static inline int fcntl_compat(const char *file, const int line, const char *cmd
>  
>  #define FCNTL_COMPAT(fd, cmd, flock) \
>  	fcntl_compat(__FILE__, __LINE__, #cmd, fd, cmd, flock)
> +#endif
>  
>  #endif /* FCNTL_COMMON_H__ */
> -- 
> 2.40.1.495.gc816e09b53d-goog


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH v1 1/2] fcntl{34, 36}: Only use fcntl64 with 32bit abi

[Edward Liaw via ltp]

On Thu, Apr 27, 2023 at 2:47 AM Richard Palethorpe <rpalethorpe@suse.de> wrote:
>
> Hello,
>
> Edward Liaw via ltp <ltp@lists.linux.it> writes:
>
> > Fixes: 7643115aaacb ("fcntl{34,36}: Always use 64-bit flock struct to avoid EINVAL")
> >
> > On Android arm64, tst_kernel_bits is disregarding the abi, so compiling
> > with the 32bit abi is calling the fcntl syscall instead of fcntl64.
> > The
>
> IIRC that's the idea because fcntl64 doesn't exist on a 64bit
> kernel.
>
> So if you compile the test with -m32/32bit ABI on x86_64 64bit kernel
> then you will get ENOSYS? If not then I suppose it is fine.

I did not get ENOSYS.  I checked with strace and my x86_64 bit kernel
does appear to have fcntl64 when compiled with -m32.  I'm not sure if
that's always the case, though.  I checked with the other fcntl tests
and they also appear to call fcntl64 with the 32 bit abi.

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

Re: [LTP] [PATCH v1 1/2] fcntl{34, 36}: Only use fcntl64 with 32bit abi

[Petr Vorel]

Hi Edward,

> Fixes: 7643115aaacb ("fcntl{34,36}: Always use 64-bit flock struct to avoid EINVAL")

> On Android arm64, tst_kernel_bits is disregarding the abi, so compiling
What exactly do you mean by "disregarding the abi"? Why is aarch64 different?

> with the 32bit abi is calling the fcntl syscall instead of fcntl64.  The
> fcntl syscall is not compatible with the flock64 struct being passed
> (this doesn't seem to be the case with x86_64, only with arm64).

> This changes it to only use the fcntl64 compat syscall with the 32bit
> abi.

> Signed-off-by: Edward Liaw <edliaw@google.com>
> ---
>  testcases/kernel/syscalls/fcntl/fcntl_common.h | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)

> diff --git a/testcases/kernel/syscalls/fcntl/fcntl_common.h b/testcases/kernel/syscalls/fcntl/fcntl_common.h
> index 5c130a784..485a31367 100644
> --- a/testcases/kernel/syscalls/fcntl/fcntl_common.h
> +++ b/testcases/kernel/syscalls/fcntl/fcntl_common.h
> @@ -10,6 +10,11 @@
>  #include "lapi/abisize.h"
>  #include "lapi/fcntl.h"

> +#if defined(TST_ABI64)
> +#define FCNTL_COMPAT(fd, cmd, flock) \
> +	SAFE_FCNTL(fd, cmd, flock)
> +
> +#else
>  struct my_flock64 {
>  	int16_t l_type;
>  	int16_t l_whence;
> @@ -43,8 +48,8 @@ static inline int fcntl_compat(const char *file, const int line, const char *cmd
>  		.l_len = lck->l_len,
>  		.l_pid = lck->l_pid,
>  	};
> -	const int sysno = tst_kernel_bits() > 32 ? __NR_fcntl : __NR_fcntl64;
> -	const int ret = tst_syscall(sysno, fd, cmd, &l64);
> +
> +	const int ret = tst_syscall(__NR_fcntl64, fd, cmd, &l64);

>  	lck->l_type = l64.l_type;
>  	lck->l_whence = l64.l_whence;
> @@ -57,7 +62,7 @@ static inline int fcntl_compat(const char *file, const int line, const char *cmd

>  	tst_brk_(file, line, TBROK | TERRNO,
>  		 "%s(%d, %s, { %d, %d, %"PRId64", %"PRId64", %d })",
> -		 tst_kernel_bits() > 32 ? "fcntl" : "fcntl64",
> +		 "fcntl64",

Once we removed tst_kernel_bits(), there is no need to pass "fcntl64" as %s,
thus it should be:

		 "fcntl64(%d, %s, { %d, %d, %"PRId64", %"PRId64", %d })",
		 fd, cmd_name, l64.l_type, l64.l_whence, l64.l_start, l64.l_len,
		 l64.l_pid);

Otherwise LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

>  		 fd,
>  		 cmd_name,
>  		 l64.l_type, l64.l_whence, l64.l_start, l64.l_len, l64.l_pid);
> @@ -67,5 +72,6 @@ static inline int fcntl_compat(const char *file, const int line, const char *cmd

>  #define FCNTL_COMPAT(fd, cmd, flock) \
>  	fcntl_compat(__FILE__, __LINE__, #cmd, fd, cmd, flock)
> +#endif

>  #endif /* FCNTL_COMMON_H__ */

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

Re: [LTP] [PATCH v1 1/2] fcntl{34, 36}: Only use fcntl64 with 32bit abi

[Edward Liaw via ltp]

On Thu, Apr 27, 2023 at 2:35 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Edward,
>
> > Fixes: 7643115aaacb ("fcntl{34,36}: Always use 64-bit flock struct to avoid EINVAL")
>
> > On Android arm64, tst_kernel_bits is disregarding the abi, so compiling
> What exactly do you mean by "disregarding the abi"? Why is aarch64 different?

In x86/entry/syscalls/syscall_32.tbl, a 64bit kernel uses
compat_sys_fcntl64, which is flock64 compatible; whereas in
arm/tools/syscall.tbl it uses sys_fcntl, which is not flock64
compatible.

> Once we removed tst_kernel_bits(), there is no need to pass "fcntl64" as %s,
> thus it should be:
>
>                  "fcntl64(%d, %s, { %d, %d, %"PRId64", %"PRId64", %d })",
>                  fd, cmd_name, l64.l_type, l64.l_whence, l64.l_start, l64.l_len,
>                  l64.l_pid);
>
> Otherwise LGTM.

Sounds good, I will send a v2.

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

Re: [LTP] [PATCH v1 1/2] fcntl{34, 36}: Only use fcntl64 with 32bit abi

[Petr Vorel]

> On Thu, Apr 27, 2023 at 2:35 AM Petr Vorel <pvorel@suse.cz> wrote:

> > Hi Edward,

> > > Fixes: 7643115aaacb ("fcntl{34,36}: Always use 64-bit flock struct to avoid EINVAL")

> > > On Android arm64, tst_kernel_bits is disregarding the abi, so compiling
> > What exactly do you mean by "disregarding the abi"? Why is aarch64 different?

> In x86/entry/syscalls/syscall_32.tbl, a 64bit kernel uses
> compat_sys_fcntl64, which is flock64 compatible; whereas in
> arm/tools/syscall.tbl it uses sys_fcntl, which is not flock64
> compatible.
Thanks for info! Also thanks you updated the description in v2.

Kind regards,
Petr

> > Once we removed tst_kernel_bits(), there is no need to pass "fcntl64" as %s,
> > thus it should be:

> >                  "fcntl64(%d, %s, { %d, %d, %"PRId64", %"PRId64", %d })",
> >                  fd, cmd_name, l64.l_type, l64.l_whence, l64.l_start, l64.l_len,
> >                  l64.l_pid);

> > Otherwise LGTM.

> Sounds good, I will send a v2.

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

[LTP] [PATCH v1 2/2] fcntl{34, 36}: Use arch dependent types for my_flock64

[Edward Liaw via ltp]

On Android arm64, fcntl is not accepting the my_flock64 struct being
passed (fails with EINVAL).  This modifies the struct type to match the
flock64 definition in the kernel.

Signed-off-by: Edward Liaw <edliaw@google.com>
---
 testcases/kernel/syscalls/fcntl/fcntl_common.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/testcases/kernel/syscalls/fcntl/fcntl_common.h b/testcases/kernel/syscalls/fcntl/fcntl_common.h
index 485a31367..78ffcdc2d 100644
--- a/testcases/kernel/syscalls/fcntl/fcntl_common.h
+++ b/testcases/kernel/syscalls/fcntl/fcntl_common.h
@@ -16,13 +16,13 @@
 
 #else
 struct my_flock64 {
-	int16_t l_type;
-	int16_t l_whence;
-	int64_t l_start;
-	int64_t l_len;
-	int32_t l_pid;
+	short l_type;
+	short l_whence;
+	off64_t l_start;
+	off64_t l_len;
+	pid_t l_pid;
 #if defined(__sparc__)
-	int16_t padding;
+	short padding;
 #endif
 };
 
-- 
2.40.1.495.gc816e09b53d-goog


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