| Message ID | 20230427002905.1354207-2-edliaw@google.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fcntl{34,36}: Fixes for Android arm64 | expand |
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
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__ */
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.
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.
> 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.
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__ */
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(-)