| Message ID | 20230110141616.1449-2-rpalethorpe@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] syscall: System call numbers are word sized | expand |
Hi Richie, [ Cc Khem Raj ] > Recently we switched to using struct fcntl with _FILE_OFFSET_BITS > instead of the transitional type fcntl64 which has been removed from > some libcs. Do you mean b0320456cd ("testcases: Fix largefile support") ? Because this commit really broke both 32 bit emulation + (obviously) LTP on native 32bit. Please add before merge: Fixes: b0320456c ("testcases: Fix largefile support") (although this needs also previous fix) > This broke testing with 32-bit executables on a 64bit kernel when > FILE_OFFSET_BITS was not set to 64. Because the fcntl64 syscall does > not exist on 64 bit kernels. > The reason we are making the syscall directly is because of old > glibc's which don't do any conversion. > So we now do a conversion unconditionally and call fcntl64 if the > kernel is 32-bit. +1. > When we no longer support old glibcs we can drop this compat function > altogether. I wonder which glibc these are. And how about musl? Anyway, thanks! Reviewed-by: Petr Vorel <pvorel@suse.cz> Tested-by: Petr Vorel <pvorel@suse.cz> Few unimportant notes below. ... > +++ b/testcases/kernel/syscalls/fcntl/fcntl_common.h > @@ -1,38 +1,71 @@ > #ifndef FCNTL_COMMON_H__ > #define FCNTL_COMMON_H__ > +#include <inttypes.h> > + > +#include "tst_test.h" > +#include "tst_kernel.h" > + > #include "lapi/syscalls.h" > #include "lapi/abisize.h" > +#include "lapi/fcntl.h" > + > +struct my_flock64 { > + int16_t l_type; > + int16_t l_whence; > + int64_t l_start; > + int64_t l_len; > + int32_t l_pid; > +#if defined(__sparc__) nit: #ifdef __sparc__ Well, we still support 32bit sparc (you did support for atomic in include/tst_atomic.h), but IMHO it's dead (I might ask if John Paul Adrian Glaubitz knows about anybody using LTP for testing on sparc). But as this is in kernel struct, there is no harm to keep it for LTP as well. > + int16_t padding; > +#endif > +}; ... > +static inline int fcntl_compat(const char *file, const int line, const char *cmd_name, > + int fd, int cmd, struct flock *lck) > { > - int ret = tst_syscall(__NR_fcntl64, fd, cmd, lck); > - if (ret == -1) > - tst_brk(TBROK|TERRNO, "fcntl64"); > + struct my_flock64 l64 = { > + .l_type = lck->l_type, > + .l_whence = lck->l_whence, > + .l_start = lck->l_start, > + .l_len = lck->l_len, > + .l_pid = lck->l_pid, > + }; > + const long sysno = tst_kernel_bits() > 32 ? __NR_fcntl : __NR_fcntl64; > + const int ret = tst_syscall(sysno, fd, cmd, &l64); > + > + lck->l_type = l64.l_type; > + lck->l_whence = l64.l_whence; > + lck->l_start = l64.l_start; > + lck->l_len = l64.l_len; > + lck->l_pid = l64.l_pid; > + > + if (ret != -1) > + return ret; > + > + tst_brk_(file, line, TBROK | TERRNO, > + "%s(%d, %s, { %d, %d, %"PRId64", %"PRId64", %d })", > + tst_kernel_bits() > 32 ? "fcntl" : "fcntl64", nit: maybe cache tst_kernel_bits() to variable? Kind regards, Petr > + fd, > + cmd_name, > + l64.l_type, l64.l_whence, l64.l_start, l64.l_len, l64.l_pid); > + > return ret; > } > -#endif > + > +#define FCNTL_COMPAT(fd, cmd, flock) \ > + fcntl_compat(__FILE__, __LINE__, #cmd, fd, cmd, flock) > #endif /* FCNTL_COMMON_H__ */
Hello, Petr Vorel <pvorel@suse.cz> writes: > Hi Richie, > > [ Cc Khem Raj ] > >> Recently we switched to using struct fcntl with _FILE_OFFSET_BITS >> instead of the transitional type fcntl64 which has been removed from >> some libcs. > > Do you mean b0320456cd ("testcases: Fix largefile support") ? > Because this commit really broke both 32 bit emulation > + (obviously) LTP on native 32bit. > > Please add before merge: > > Fixes: b0320456c ("testcases: Fix largefile support") I suppose, thanks. I'm not sure how much fixes tags help in LTP? In kernel they are used in automatic backporting and such. > > (although this needs also previous fix) > >> This broke testing with 32-bit executables on a 64bit kernel when >> FILE_OFFSET_BITS was not set to 64. Because the fcntl64 syscall does >> not exist on 64 bit kernels. > >> The reason we are making the syscall directly is because of old >> glibc's which don't do any conversion. > >> So we now do a conversion unconditionally and call fcntl64 if the >> kernel is 32-bit. > +1. > >> When we no longer support old glibcs we can drop this compat function >> altogether. > I wonder which glibc these are. And how about musl? I find it difficult to tell honestly. Pre 2.28 perhaps which is just what "git describe --contains" suggests. Muslc appears to always use 64-bit fcntl. > > Anyway, thanks! > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > Tested-by: Petr Vorel <pvorel@suse.cz> Will merge, thanks! > > Few unimportant notes below. > ... > >> +++ b/testcases/kernel/syscalls/fcntl/fcntl_common.h >> @@ -1,38 +1,71 @@ >> #ifndef FCNTL_COMMON_H__ >> #define FCNTL_COMMON_H__ > >> +#include <inttypes.h> >> + >> +#include "tst_test.h" >> +#include "tst_kernel.h" >> + >> #include "lapi/syscalls.h" >> #include "lapi/abisize.h" >> +#include "lapi/fcntl.h" >> + >> +struct my_flock64 { >> + int16_t l_type; >> + int16_t l_whence; >> + int64_t l_start; >> + int64_t l_len; >> + int32_t l_pid; >> +#if defined(__sparc__) > nit: #ifdef __sparc__ > > Well, we still support 32bit sparc (you did support for atomic in > include/tst_atomic.h), but IMHO it's dead (I might ask if John Paul Adrian > Glaubitz knows about anybody using LTP for testing on sparc). But as this is in > kernel struct, there is no harm to keep it for LTP as well. Yes, my main thinking was that it is easy to include and I'm not sure if __sparc__ also gets set on sparc64 or whatever is actually in use. > >> + int16_t padding; >> +#endif >> +}; > > ... > >> +static inline int fcntl_compat(const char *file, const int line, const char *cmd_name, >> + int fd, int cmd, struct flock *lck) >> { >> - int ret = tst_syscall(__NR_fcntl64, fd, cmd, lck); >> - if (ret == -1) >> - tst_brk(TBROK|TERRNO, "fcntl64"); >> + struct my_flock64 l64 = { >> + .l_type = lck->l_type, >> + .l_whence = lck->l_whence, >> + .l_start = lck->l_start, >> + .l_len = lck->l_len, >> + .l_pid = lck->l_pid, >> + }; >> + const long sysno = tst_kernel_bits() > 32 ? __NR_fcntl : __NR_fcntl64; >> + const int ret = tst_syscall(sysno, fd, cmd, &l64); >> + >> + lck->l_type = l64.l_type; >> + lck->l_whence = l64.l_whence; >> + lck->l_start = l64.l_start; >> + lck->l_len = l64.l_len; >> + lck->l_pid = l64.l_pid; >> + >> + if (ret != -1) >> + return ret; >> + >> + tst_brk_(file, line, TBROK | TERRNO, >> + "%s(%d, %s, { %d, %d, %"PRId64", %"PRId64", %d })", >> + tst_kernel_bits() > 32 ? "fcntl" : "fcntl64", > nit: maybe cache tst_kernel_bits() to variable? The function already does it. > > Kind regards, > Petr > >> + fd, >> + cmd_name, >> + l64.l_type, l64.l_whence, l64.l_start, l64.l_len, l64.l_pid); >> + >> return ret; >> } >> -#endif >> + >> +#define FCNTL_COMPAT(fd, cmd, flock) \ >> + fcntl_compat(__FILE__, __LINE__, #cmd, fd, cmd, flock) > >> #endif /* FCNTL_COMMON_H__ */
diff --git a/testcases/kernel/syscalls/fcntl/fcntl34.c b/testcases/kernel/syscalls/fcntl/fcntl34.c index 8d5e22b72..45e693fe6 100644 --- a/testcases/kernel/syscalls/fcntl/fcntl34.c +++ b/testcases/kernel/syscalls/fcntl/fcntl34.c @@ -10,10 +10,8 @@ #include <pthread.h> #include <sched.h> -#include "lapi/fcntl.h" -#include "tst_safe_pthread.h" -#include "tst_test.h" #include "fcntl_common.h" +#include "tst_safe_pthread.h" static int thread_cnt; static const int max_thread_cnt = 32; @@ -62,13 +60,13 @@ void *thread_fn_01(void *arg) for (i = 0; i < writes_num; ++i) { lck.l_type = F_WRLCK; - my_fcntl(fd, F_OFD_SETLKW, &lck); + FCNTL_COMPAT(fd, F_OFD_SETLKW, &lck); SAFE_LSEEK(fd, 0, SEEK_END); SAFE_WRITE(SAFE_WRITE_ALL, fd, buf, write_size); lck.l_type = F_UNLCK; - my_fcntl(fd, F_OFD_SETLKW, &lck); + FCNTL_COMPAT(fd, F_OFD_SETLKW, &lck); sched_yield(); } diff --git a/testcases/kernel/syscalls/fcntl/fcntl36.c b/testcases/kernel/syscalls/fcntl/fcntl36.c index 067eaee64..e84b7ed0c 100644 --- a/testcases/kernel/syscalls/fcntl/fcntl36.c +++ b/testcases/kernel/syscalls/fcntl/fcntl36.c @@ -84,13 +84,13 @@ static void *fn_ofd_w(void *arg) memset(buf, wt, pa->length); lck.l_type = F_WRLCK; - my_fcntl(fd, F_OFD_SETLKW, &lck); + FCNTL_COMPAT(fd, F_OFD_SETLKW, &lck); SAFE_LSEEK(fd, pa->offset, SEEK_SET); SAFE_WRITE(SAFE_WRITE_ALL, fd, buf, pa->length); lck.l_type = F_UNLCK; - my_fcntl(fd, F_OFD_SETLKW, &lck); + FCNTL_COMPAT(fd, F_OFD_SETLKW, &lck); wt++; if (wt >= 255) @@ -163,7 +163,7 @@ static void *fn_ofd_r(void *arg) memset(buf, 0, pa->length); lck.l_type = F_RDLCK; - my_fcntl(fd, F_OFD_SETLKW, &lck); + FCNTL_COMPAT(fd, F_OFD_SETLKW, &lck); /* rlock acquired */ SAFE_LSEEK(fd, pa->offset, SEEK_SET); @@ -194,7 +194,7 @@ static void *fn_ofd_r(void *arg) } lck.l_type = F_UNLCK; - my_fcntl(fd, F_OFD_SETLK, &lck); + FCNTL_COMPAT(fd, F_OFD_SETLK, &lck); sched_yield(); } diff --git a/testcases/kernel/syscalls/fcntl/fcntl_common.h b/testcases/kernel/syscalls/fcntl/fcntl_common.h index abb82c96f..734b57af2 100644 --- a/testcases/kernel/syscalls/fcntl/fcntl_common.h +++ b/testcases/kernel/syscalls/fcntl/fcntl_common.h @@ -1,38 +1,71 @@ #ifndef FCNTL_COMMON_H__ #define FCNTL_COMMON_H__ +#include <inttypes.h> + +#include "tst_test.h" +#include "tst_kernel.h" + #include "lapi/syscalls.h" #include "lapi/abisize.h" +#include "lapi/fcntl.h" + +struct my_flock64 { + int16_t l_type; + int16_t l_whence; + int64_t l_start; + int64_t l_len; + int32_t l_pid; +#if defined(__sparc__) + int16_t padding; +#endif +}; /* - * glibc commit: - * 06ab719d30b0 ("Fix Linux fcntl OFD locks for non-LFS architectures (BZ#20251)") - * changed behavior of arg parameter for OFD commands. It is no - * longer passing arg directly to syscall, but expects it to be - * 'struct flock'. + * F_OFD_* commands always require flock64 struct. Older GLibc (pre 2.29) would + * pass the flock sturct directly to the kernel even if it had 32-bit + * offsets. * - * On 64-bit or _FILE_OFFSET_BITS == 64 we can use fcntl() and - * struct flock64 with any glibc version. struct flock and flock64 - * should be identical. + * Also, if and only if, we are on 32-bit kernel we need to use the + * fcntl64 compat syscall. * - * On 32-bit, older glibc would pass arg directly, recent one treats - * it as 'struct flock' and converts it to 'struct flock64'. - * So, to support both version, on 32-bit we use fcntl64 syscall - * directly with struct flock64. + * See: + * glibc: 06ab719d30 Fix Linux fcntl OFD locks for non-LFS architectures (BZ#20251) + * kernel: fs/fcntl.c */ -#if defined(TST_ABI64) || _FILE_OFFSET_BITS == 64 -static int my_fcntl(int fd, int cmd, void *lck) -{ - return SAFE_FCNTL(fd, cmd, lck); -} -#else -static int my_fcntl(int fd, int cmd, void *lck) +static inline int fcntl_compat(const char *file, const int line, const char *cmd_name, + int fd, int cmd, struct flock *lck) { - int ret = tst_syscall(__NR_fcntl64, fd, cmd, lck); - if (ret == -1) - tst_brk(TBROK|TERRNO, "fcntl64"); + struct my_flock64 l64 = { + .l_type = lck->l_type, + .l_whence = lck->l_whence, + .l_start = lck->l_start, + .l_len = lck->l_len, + .l_pid = lck->l_pid, + }; + const long sysno = tst_kernel_bits() > 32 ? __NR_fcntl : __NR_fcntl64; + const int ret = tst_syscall(sysno, fd, cmd, &l64); + + lck->l_type = l64.l_type; + lck->l_whence = l64.l_whence; + lck->l_start = l64.l_start; + lck->l_len = l64.l_len; + lck->l_pid = l64.l_pid; + + if (ret != -1) + return ret; + + tst_brk_(file, line, TBROK | TERRNO, + "%s(%d, %s, { %d, %d, %"PRId64", %"PRId64", %d })", + tst_kernel_bits() > 32 ? "fcntl" : "fcntl64", + fd, + cmd_name, + l64.l_type, l64.l_whence, l64.l_start, l64.l_len, l64.l_pid); + return ret; } -#endif + +#define FCNTL_COMPAT(fd, cmd, flock) \ + fcntl_compat(__FILE__, __LINE__, #cmd, fd, cmd, flock) #endif /* FCNTL_COMMON_H__ */
Recently we switched to using struct fcntl with _FILE_OFFSET_BITS instead of the transitional type fcntl64 which has been removed from some libcs. This broke testing with 32-bit executables on a 64bit kernel when FILE_OFFSET_BITS was not set to 64. Because the fcntl64 syscall does not exist on 64 bit kernels. The reason we are making the syscall directly is because of old glibc's which don't do any conversion. So we now do a conversion unconditionally and call fcntl64 if the kernel is 32-bit. When we no longer support old glibcs we can drop this compat function altogether. Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> --- testcases/kernel/syscalls/fcntl/fcntl34.c | 8 +- testcases/kernel/syscalls/fcntl/fcntl36.c | 8 +- .../kernel/syscalls/fcntl/fcntl_common.h | 79 +++++++++++++------ 3 files changed, 63 insertions(+), 32 deletions(-)