[V2,15/17] syscalls/semtimedop: Add support for semtimedop and its time64 version

This adds support for semtimedop() and its time64 variant to the
existing semop() syscall tests.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 testcases/kernel/syscalls/ipc/semop/semop.h   | 52 +++++++++++++++++++
 testcases/kernel/syscalls/ipc/semop/semop01.c | 11 +++-
 testcases/kernel/syscalls/ipc/semop/semop02.c | 12 ++++-
 testcases/kernel/syscalls/ipc/semop/semop03.c | 12 ++++-
 testcases/kernel/syscalls/ipc/semop/semop04.c | 12 ++++-
 testcases/kernel/syscalls/ipc/semop/semop05.c | 12 ++++-
 6 files changed, 106 insertions(+), 5 deletions(-)
 create mode 100644 testcases/kernel/syscalls/ipc/semop/semop.h

On Fri, May 8, 2020 at 6:24 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> +
> +static inline int sys_semtimedop(int semid, struct sembuf *sops, size_t nsops,
> +               void *timeout)
> +{
> +       return tst_syscall(__NR_semtimedop, semid, sops, nsops, timeout);
> +}
> +
> +static inline int sys_semtimedop_time64(int semid, struct sembuf *sops,
> +                                       size_t nsops, void *timeout)
> +{
> +       return tst_syscall(__NR_semtimedop_time64, semid, sops, nsops, timeout);
> +}
> +
> +struct test_variants {
> +       int (*semop)(int semid, struct sembuf *sops, size_t nsops);
> +       int (*semtimedop)(int semid, struct sembuf *sops, size_t nsops, void *timeout);
> +       enum tst_ts_type type;
> +       char *desc;
> +} variants[] = {
> +       { .semop = semop, .type = TST_LIBC_TIMESPEC, .desc = "semop: vDSO or syscall"},
> +#if defined(TST_ABI32)
> +       { .semtimedop = sys_semtimedop, .type = TST_LIBC_TIMESPEC, .desc = "semtimedop: syscall with libc spec"},
> +       { .semtimedop = sys_semtimedop, .type = TST_KERN_OLD_TIMESPEC, .desc = "semtimedop: syscall with kernel spec32"},
> +#endif
> +
> +#if defined(TST_ABI64)
> +       { .semtimedop = sys_semtimedop, .type = TST_KERN_TIMESPEC, .desc = "semtimedop: syscall with kernel spec64"},
> +#endif


It feels like this is more complicated than it need to be. The line

semtimedop = sys_semtimedop, .type = TST_KERN_OLD_TIMESPEC, .desc =
"semtimedop: syscall with kernel spec32"},

should apply to any kernel that has "__NR_semtimedop !=
__LTP__NR_INVALID_SYSCALL",
regardless of any other macros set, and then you don't need the separate line

{ .semtimedop = sys_semtimedop, .type = TST_KERN_TIMESPEC, .desc =
"semtimedop: syscall with kernel spec64"},

which is not what the ABI is meant to be anyway (sys_semtimedop takes
a __kernel_old_timespec,
not a __kernel_timespec).

Similarly, the line

 { .semop = semop, .type = TST_LIBC_TIMESPEC, .desc = "semop: vDSO or syscall"},

should apply to both 32 and 64 bit machines

     Arnd

On 08-05-20, 09:18, Arnd Bergmann wrote:
> On Fri, May 8, 2020 at 6:24 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> > +
> > +static inline int sys_semtimedop(int semid, struct sembuf *sops, size_t nsops,
> > +               void *timeout)
> > +{
> > +       return tst_syscall(__NR_semtimedop, semid, sops, nsops, timeout);
> > +}
> > +
> > +static inline int sys_semtimedop_time64(int semid, struct sembuf *sops,
> > +                                       size_t nsops, void *timeout)
> > +{
> > +       return tst_syscall(__NR_semtimedop_time64, semid, sops, nsops, timeout);
> > +}
> > +
> > +struct test_variants {
> > +       int (*semop)(int semid, struct sembuf *sops, size_t nsops);
> > +       int (*semtimedop)(int semid, struct sembuf *sops, size_t nsops, void *timeout);
> > +       enum tst_ts_type type;
> > +       char *desc;
> > +} variants[] = {
> > +       { .semop = semop, .type = TST_LIBC_TIMESPEC, .desc = "semop: vDSO or syscall"},
> > +#if defined(TST_ABI32)
> > +       { .semtimedop = sys_semtimedop, .type = TST_LIBC_TIMESPEC, .desc = "semtimedop: syscall with libc spec"},
> > +       { .semtimedop = sys_semtimedop, .type = TST_KERN_OLD_TIMESPEC, .desc = "semtimedop: syscall with kernel spec32"},
> > +#endif
> > +
> > +#if defined(TST_ABI64)
> > +       { .semtimedop = sys_semtimedop, .type = TST_KERN_TIMESPEC, .desc = "semtimedop: syscall with kernel spec64"},
> > +#endif
> 
> 
> It feels like this is more complicated than it need to be. The line
> 
> semtimedop = sys_semtimedop, .type = TST_KERN_OLD_TIMESPEC, .desc =
> "semtimedop: syscall with kernel spec32"},
> 
> should apply to any kernel that has "__NR_semtimedop !=
> __LTP__NR_INVALID_SYSCALL",
> regardless of any other macros set, and then you don't need the separate line
> 
> { .semtimedop = sys_semtimedop, .type = TST_KERN_TIMESPEC, .desc =
> "semtimedop: syscall with kernel spec64"},
 
> which is not what the ABI is meant to be anyway (sys_semtimedop takes
> a __kernel_old_timespec,
> not a __kernel_timespec).

There is some misunderstanding here, surely from my side. The sys_
helpers here are the direct syscalls called from userspace with help
of tst_syscall().

AFAIU, on 32 bit systems we need to call __NR_semtimedop with the 32
bit and 64 bit timespec (both), and on 64 bit systems which don't
implement __NR_semtimedop_time64, we need to call __NR_semtimedop with
the 64 bit timespec only.

What you are telling now is very different from that and so I don't
get it.

> Similarly, the line
> 
>  { .semop = semop, .type = TST_LIBC_TIMESPEC, .desc = "semop: vDSO or syscall"},
> 
> should apply to both 32 and 64 bit machines

Yes and so it is called without ifdef hackery. Isn't that correct ?

On Fri, May 8, 2020 at 10:57 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 08-05-20, 09:18, Arnd Bergmann wrote:
> > On Fri, May 8, 2020 at 6:24 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > > +
> > > +static inline int sys_semtimedop(int semid, struct sembuf *sops, size_t nsops,
> > > +               void *timeout)
> > > +{
> > > +       return tst_syscall(__NR_semtimedop, semid, sops, nsops, timeout);
> > > +}
> > > +
> > > +static inline int sys_semtimedop_time64(int semid, struct sembuf *sops,
> > > +                                       size_t nsops, void *timeout)
> > > +{
> > > +       return tst_syscall(__NR_semtimedop_time64, semid, sops, nsops, timeout);
> > > +}
> > > +
> > > +struct test_variants {
> > > +       int (*semop)(int semid, struct sembuf *sops, size_t nsops);
> > > +       int (*semtimedop)(int semid, struct sembuf *sops, size_t nsops, void *timeout);
> > > +       enum tst_ts_type type;
> > > +       char *desc;
> > > +} variants[] = {
> > > +       { .semop = semop, .type = TST_LIBC_TIMESPEC, .desc = "semop: vDSO or syscall"},
> > > +#if defined(TST_ABI32)
> > > +       { .semtimedop = sys_semtimedop, .type = TST_LIBC_TIMESPEC, .desc = "semtimedop: syscall with libc spec"},
> > > +       { .semtimedop = sys_semtimedop, .type = TST_KERN_OLD_TIMESPEC, .desc = "semtimedop: syscall with kernel spec32"},
> > > +#endif
> > > +
> > > +#if defined(TST_ABI64)
> > > +       { .semtimedop = sys_semtimedop, .type = TST_KERN_TIMESPEC, .desc = "semtimedop: syscall with kernel spec64"},
> > > +#endif
> >
> >
> > It feels like this is more complicated than it need to be. The line
> >
> > semtimedop = sys_semtimedop, .type = TST_KERN_OLD_TIMESPEC, .desc =
> > "semtimedop: syscall with kernel spec32"},
> >
> > should apply to any kernel that has "__NR_semtimedop !=
> > __LTP__NR_INVALID_SYSCALL",
> > regardless of any other macros set, and then you don't need the separate line
> >
> > { .semtimedop = sys_semtimedop, .type = TST_KERN_TIMESPEC, .desc =
> > "semtimedop: syscall with kernel spec64"},
>
> > which is not what the ABI is meant to be anyway (sys_semtimedop takes
> > a __kernel_old_timespec,
> > not a __kernel_timespec).
>
> There is some misunderstanding here, surely from my side. The sys_
> helpers here are the direct syscalls called from userspace with help
> of tst_syscall().
>
> AFAIU, on 32 bit systems we need to call __NR_semtimedop with the 32
> bit and 64 bit timespec (both), and on 64 bit systems which don't
> implement __NR_semtimedop_time64, we need to call __NR_semtimedop with
> the 64 bit timespec only.
>
> What you are telling now is very different from that and so I don't
> get it.

__NR_semtimedop can only be called with the 'old' timespec, which
may have either 32 or 64 members depending on the architecture.
On x32 it uses 64-bit members, and on riscv32 it does not exist at all.

I think you already have a correct __kernel_old_timespec definition,
so what I'd expect to see here is code that passes __kernel_old_timespec
into __NR_semtimedop whenever __NR_semtimedop is defined.

Passing the libc timespec into __kernel_old_timespec is a bug, as
the libc may be using either the old or the new (always 64-bit)
definition.

> >  { .semop = semop, .type = TST_LIBC_TIMESPEC, .desc = "semop: vDSO or syscall"},
> >
> > should apply to both 32 and 64 bit machines
>
> Yes and so it is called without ifdef hackery. Isn't that correct ?

My mistake, I confused the lines. What I meant is that there should
be an unconditional test of the libc 'semtimedop' with the libc 'timespec'
definition.

     Arnd

On Fri, May 8, 2020 at 12:28 PM Viresh Kumar <viresh.kumar@linaro.org>
wrote:

> This adds support for semtimedop() and its time64 variant to the
> existing semop() syscall tests.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  testcases/kernel/syscalls/ipc/semop/semop.h   | 52 +++++++++++++++++++
>  testcases/kernel/syscalls/ipc/semop/semop01.c | 11 +++-
>  testcases/kernel/syscalls/ipc/semop/semop02.c | 12 ++++-
>  testcases/kernel/syscalls/ipc/semop/semop03.c | 12 ++++-
>  testcases/kernel/syscalls/ipc/semop/semop04.c | 12 ++++-
>  testcases/kernel/syscalls/ipc/semop/semop05.c | 12 ++++-
>  6 files changed, 106 insertions(+), 5 deletions(-)
>  create mode 100644 testcases/kernel/syscalls/ipc/semop/semop.h
>
> diff --git a/testcases/kernel/syscalls/ipc/semop/semop.h
> b/testcases/kernel/syscalls/ipc/semop/semop.h
> new file mode 100644
> index 000000000000..8d1245b65ec0
> --- /dev/null
> +++ b/testcases/kernel/syscalls/ipc/semop/semop.h
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#ifndef SEMOP_VAR__
> +#define SEMOP_VAR__
> +
> +#include <sys/sem.h>
> +#include "tst_timer.h"
> +#include "lapi/abisize.h"
> +
> +static inline int sys_semtimedop(int semid, struct sembuf *sops, size_t
> nsops,
> +               void *timeout)
> +{
> +       return tst_syscall(__NR_semtimedop, semid, sops, nsops, timeout);
> +}
> +
> +static inline int sys_semtimedop_time64(int semid, struct sembuf *sops,
> +                                       size_t nsops, void *timeout)
> +{
> +       return tst_syscall(__NR_semtimedop_time64, semid, sops, nsops,
> timeout);
> +}
> +
> +struct test_variants {
> +       int (*semop)(int semid, struct sembuf *sops, size_t nsops);
> +       int (*semtimedop)(int semid, struct sembuf *sops, size_t nsops,
> void *timeout);
> +       enum tst_ts_type type;
> +       char *desc;
> +} variants[] = {
> +       { .semop = semop, .type = TST_LIBC_TIMESPEC, .desc = "semop: vDSO
> or syscall"},
> +#if defined(TST_ABI32)
> +       { .semtimedop = sys_semtimedop, .type = TST_LIBC_TIMESPEC, .desc =
> "semtimedop: syscall with libc spec"},
> +       { .semtimedop = sys_semtimedop, .type = TST_KERN_OLD_TIMESPEC,
> .desc = "semtimedop: syscall with kernel spec32"},
> +#endif
> +
> +#if defined(TST_ABI64)
> +       { .semtimedop = sys_semtimedop, .type = TST_KERN_TIMESPEC, .desc =
> "semtimedop: syscall with kernel spec64"},
> +#endif
> +
> +#if (__NR_semtimedop_time64 != __LTP__NR_INVALID_SYSCALL)
> +       { .semtimedop = sys_semtimedop_time64, .type = TST_KERN_TIMESPEC,
> .desc = "semtimedop: syscall time64 with kernel spec64"},
> +#endif
> +};
> +
> +static inline int call_semop(struct test_variants *tv, int semid,
> +               struct sembuf *sops, size_t nsops, struct tst_ts *timeout)
> +{
> +       if (tv->semop)
> +               return tv->semop(semid, sops, nsops);
> +
> +       return tv->semtimedop(semid, sops, nsops, tst_ts_get(timeout));
> +}
> +
> +#endif /* SEMOP_VAR__ */
> diff --git a/testcases/kernel/syscalls/ipc/semop/semop01.c
> b/testcases/kernel/syscalls/ipc/semop/semop01.c
> index bcb45fa69320..a4924376e32c 100644
> --- a/testcases/kernel/syscalls/ipc/semop/semop01.c
> +++ b/testcases/kernel/syscalls/ipc/semop/semop01.c
> @@ -41,6 +41,7 @@
>  #include "tst_test.h"
>  #include "libnewipc.h"
>  #include "lapi/semun.h"
> +#include "semop.h"
>
>  #define NSEMS  4               /* the number of primitive semaphores to
> test */
>
> @@ -52,11 +53,16 @@ static struct sembuf sops[PSEMS];
>
>  static void run(void)
>  {
> +       struct test_variants *tv = &variants[tst_variant];
>         union semun arr = { .val = 0 };
> +       struct tst_ts timeout;
>         int fail = 0;
>         int i;
>
> -       TEST(semop(sem_id, sops, NSEMS));
> +       timeout.type = tv->type;
> +       tst_ts_set_nsec(&timeout, 10000);
> +
> +       TEST(call_semop(tv, sem_id, sops, NSEMS, &timeout));
>
>         if (TST_RET == -1) {
>                 tst_res(TFAIL | TTERRNO, "semop() failed");
> @@ -94,6 +100,8 @@ static void setup(void)
>  {
>         int i;
>
> +       tst_res(TINFO, "Testing variant: %s", variants[tst_variant].desc);
> +
>         get_arr.array = malloc(sizeof(unsigned short int) * PSEMS);
>         if (get_arr.array == NULL)
>                 tst_brk(TBROK, "malloc failed");
> @@ -124,6 +132,7 @@ static void cleanup(void)
>
>  static struct tst_test test = {
>         .test_all = run,
> +       .test_variants = ARRAY_SIZE(variants),
>         .setup = setup,
>         .cleanup = cleanup,
>         .needs_tmpdir = 1,
> diff --git a/testcases/kernel/syscalls/ipc/semop/semop02.c
> b/testcases/kernel/syscalls/ipc/semop/semop02.c
> index f24d284776a4..a51906340400 100644
> --- a/testcases/kernel/syscalls/ipc/semop/semop02.c
> +++ b/testcases/kernel/syscalls/ipc/semop/semop02.c
> @@ -20,6 +20,7 @@
>  #include "tst_test.h"
>  #include "libnewipc.h"
>  #include "lapi/semun.h"
> +#include "semop.h"
>
>  static int sem_id_1 = -1;      /* a semaphore set with read & alter
> permissions */
>  static int sem_id_2 = -1;      /* a semaphore set without read & alter
> permissions */
> @@ -52,6 +53,8 @@ static void setup(void)
>         struct seminfo ipc_buf;
>         union semun arr;
>
> +       tst_res(TINFO, "Testing variant: %s", variants[tst_variant].desc);
> +
>         ltpuser = SAFE_GETPWNAM(nobody_uid);
>         SAFE_SETUID(ltpuser->pw_uid);
>
> @@ -84,7 +87,13 @@ static void setup(void)
>
>  static void run(unsigned int i)
>  {
> -       TEST(semop(*(tc[i].semid), tc[i].t_sbuf, tc[i].t_ops));
> +       struct test_variants *tv = &variants[tst_variant];
> +       struct tst_ts timeout;
> +
> +       timeout.type = tv->type;
> +       tst_ts_set_nsec(&timeout, 10000);
> +
> +       TEST(call_semop(tv, *(tc[i].semid), tc[i].t_sbuf, tc[i].t_ops,
> &timeout));
>
>         if (TST_RET != -1) {
>                 tst_res(TFAIL | TTERRNO, "call succeeded unexpectedly");
> @@ -117,6 +126,7 @@ static void cleanup(void)
>  static struct tst_test test = {
>         .test = run,
>         .tcnt = ARRAY_SIZE(tc),
> +       .test_variants = ARRAY_SIZE(variants),
>         .setup = setup,
>         .cleanup = cleanup,
>         .needs_tmpdir = 1,
> diff --git a/testcases/kernel/syscalls/ipc/semop/semop03.c
> b/testcases/kernel/syscalls/ipc/semop/semop03.c
> index 4f5f78eb6d8d..d36194ce1f41 100644
> --- a/testcases/kernel/syscalls/ipc/semop/semop03.c
> +++ b/testcases/kernel/syscalls/ipc/semop/semop03.c
> @@ -38,6 +38,7 @@
>  #include "tst_test.h"
>  #include "libnewipc.h"
>  #include "lapi/semun.h"
> +#include "semop.h"
>
>  static key_t semkey;
>  static int sem_id = -1;
> @@ -47,6 +48,12 @@ static int tc[] = { -1, PSEMS + 1 }; /* negative and
> too many "primitive" semas
>
>  static void run(unsigned int i)
>  {
> +       struct test_variants *tv = &variants[tst_variant];
> +       struct tst_ts timeout;
> +
> +       timeout.type = tv->type;
> +       tst_ts_set_nsec(&timeout, 10000);
> +
>         /* initialize two fields in the sembuf structure here */
>         s_buf.sem_op = 1;       /* add this value to struct sem.semval */
>         s_buf.sem_flg = SEM_UNDO;       /* undo when process exits */
> @@ -61,7 +68,7 @@ static void run(unsigned int i)
>          * use the TEST macro to make the call
>          */
>
> -       TEST(semop(sem_id, &s_buf, 1));
> +       TEST(call_semop(tv, sem_id, &s_buf, 1, &timeout));
>
>         if (TST_RET != -1) {
>                 tst_res(TFAIL | TTERRNO, "call succeeded unexpectedly");
> @@ -80,6 +87,8 @@ static void run(unsigned int i)
>
>  static void setup(void)
>  {
> +       tst_res(TINFO, "Testing variant: %s", variants[tst_variant].desc);
> +
>         /* get an IPC resource key */
>         semkey = GETIPCKEY();
>
> @@ -102,6 +111,7 @@ static void cleanup(void)
>  static struct tst_test test = {
>         .test = run,
>         .tcnt = ARRAY_SIZE(tc),
> +       .test_variants = ARRAY_SIZE(variants),
>         .setup = setup,
>         .cleanup = cleanup,
>         .needs_tmpdir = 1,
> diff --git a/testcases/kernel/syscalls/ipc/semop/semop04.c
> b/testcases/kernel/syscalls/ipc/semop/semop04.c
> index 0faf00a3585f..033065e7bbfc 100644
> --- a/testcases/kernel/syscalls/ipc/semop/semop04.c
> +++ b/testcases/kernel/syscalls/ipc/semop/semop04.c
> @@ -38,6 +38,7 @@
>  #include "tst_test.h"
>  #include "libnewipc.h"
>  #include "lapi/semun.h"
> +#include "semop.h"
>
>  static int sem_id = -1;
>  static int val; /* value for SETVAL */
> @@ -62,6 +63,12 @@ static struct test_case_t {
>
>  static void run(unsigned int i)
>  {
> +       struct test_variants *tv = &variants[tst_variant];
> +       struct tst_ts timeout;
> +
> +       timeout.type = tv->type;
> +       tst_ts_set_nsec(&timeout, 10000);
> +
>         /* initialize the s_buf buffer */
>         s_buf.sem_op = tc[i].op;
>         s_buf.sem_flg = tc[i].flg;
> @@ -72,7 +79,7 @@ static void run(unsigned int i)
>         if (semctl(sem_id, tc[i].num, SETVAL, tc[i].get_arr) == -1)
>                 tst_brk(TBROK | TERRNO, "semctl() failed");
>
> -       TEST(semop(sem_id, &s_buf, 1));
> +       TEST(call_semop(tv, sem_id, &s_buf, 1, &timeout));
>         if (TST_RET != -1) {
>                 tst_res(TFAIL, "call succeeded unexpectedly");
>                 return;
> @@ -86,6 +93,8 @@ static void run(unsigned int i)
>
>  static void setup(void)
>  {
> +       tst_res(TINFO, "Testing variant: %s", variants[tst_variant].desc);
> +
>         val = 1;
>
>         /* get an IPC resource key */
> @@ -114,6 +123,7 @@ static void cleanup(void)
>  static struct tst_test test = {
>         .test = run,
>         .tcnt = ARRAY_SIZE(tc),
> +       .test_variants = ARRAY_SIZE(variants),
>         .setup = setup,
>         .cleanup = cleanup,
>         .needs_tmpdir = 1,
> diff --git a/testcases/kernel/syscalls/ipc/semop/semop05.c
> b/testcases/kernel/syscalls/ipc/semop/semop05.c
> index 9e8e040b0b19..f892cd519ecf 100644
> --- a/testcases/kernel/syscalls/ipc/semop/semop05.c
> +++ b/testcases/kernel/syscalls/ipc/semop/semop05.c
> @@ -52,6 +52,7 @@
>  #include "tst_test.h"
>  #include "libnewipc.h"
>  #include "lapi/semun.h"
> +#include "semop.h"
>
>  static key_t semkey;
>  static int sem_id = -1;
> @@ -115,7 +116,13 @@ static inline int process_state_wait2(pid_t pid,
> const char state)
>
>  static void do_child(int i)
>  {
> -       TEST(semop(sem_id, &s_buf, 1));
> +       struct test_variants *tv = &variants[tst_variant];
> +       struct tst_ts timeout;
> +
> +       timeout.type = tv->type;
> +       tst_ts_set_nsec(&timeout, 10000);
> +
> +       TEST(call_semop(tv, sem_id, &s_buf, 1, &timeout));
>         if (TST_RET != -1) {
>                 tst_res(TFAIL, "call succeeded when error expected");
>                 exit(-1);
> @@ -137,6 +144,8 @@ static void sighandler(int sig)
>
>  static void setup(void)
>  {
> +       tst_res(TINFO, "Testing variant: %s", variants[tst_variant].desc);
> +
>         SAFE_SIGNAL(SIGHUP, sighandler);
>
>         /* get an IPC resource key */
> @@ -238,6 +247,7 @@ static void do_child_uclinux(void)
>  static struct tst_test test = {
>         .test = run,
>         .tcnt = ARRAY_SIZE(tc),
> +       .test_variants = ARRAY_SIZE(variants),
>

RHEL8.2(x84_64) failed as:

# ./semop05
tst_test.c:1246: INFO: Timeout per run is 0h 05m 00s
semop05.c:147: INFO: Testing variant: semop: vDSO or syscall
semop05.c:132: PASS: expected failure: EIDRM (43)
semop05.c:132: PASS: expected failure: EIDRM (43)
semop05.c:132: PASS: expected failure: EINTR (4)
semop05.c:132: PASS: expected failure: EINTR (4)
tst_test.c:1246: INFO: Timeout per run is 0h 05m 00s
semop05.c:147: INFO: Testing variant: semtimedop: syscall with kernel spec64
semop05.c:134: FAIL: unexpected failure: EAGAIN/EWOULDBLOCK (11)
Test timeouted, sending SIGKILL!
tst_test.c:1286: INFO: If you are running on slow machine, try exporting
LTP_TIMEOUT_MUL > 1
tst_test.c:1287: BROK: Test killed! (timeout?)


RHEL8.2(s390x) failed as:

tst_test.c:1246: INFO: Timeout per run is 0h 05m 00s
semop05.c:147: INFO: Testing variant: semop: vDSO or syscall
semop05.c:132: PASS: expected failure: EIDRM (43)
semop05.c:132: PASS: expected failure: EIDRM (43)
semop05.c:132: PASS: expected failure: EINTR (4)
semop05.c:132: PASS: expected failure: EINTR (4)
tst_test.c:1246: INFO: Timeout per run is 0h 05m 00s
semop05.c:147: INFO: Testing variant: semtimedop: syscall with kernel spec64
semop.h:13: CONF: syscall(392) __NR_semtimedop not supported
Test timeouted, sending SIGKILL!
tst_test.c:1286: INFO: If you are running on slow machine, try exporting
LTP_TIMEOUT_MUL > 1
tst_test.c:1287: BROK: Test killed! (timeout?)



>         .setup = setup,
>         .cleanup = cleanup,
>         .needs_tmpdir = 1,
> --
> 2.25.0.rc1.19.g042ed3e048af
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

On 08-05-20, 11:24, Arnd Bergmann wrote:
> __NR_semtimedop can only be called with the 'old' timespec, which
> may have either 32 or 64 members depending on the architecture.
> On x32 it uses 64-bit members, and on riscv32 it does not exist at all.
> 
> I think you already have a correct __kernel_old_timespec definition,
> so what I'd expect to see here is code that passes __kernel_old_timespec
> into __NR_semtimedop whenever __NR_semtimedop is defined.

Ahh, so on ARM64 and x86_64, fields will be 8 bytes long even with the old
timespec. I missed that and so the confusion.

On 08-05-20, 09:18, Arnd Bergmann wrote:
> It feels like this is more complicated than it need to be. The line
> 
> semtimedop = sys_semtimedop, .type = TST_KERN_OLD_TIMESPEC, .desc =
> "semtimedop: syscall with kernel spec32"},
> 
> should apply to any kernel that has "__NR_semtimedop !=
> __LTP__NR_INVALID_SYSCALL",
> regardless of any other macros set, and then you don't need the separate line
> 
> { .semtimedop = sys_semtimedop, .type = TST_KERN_TIMESPEC, .desc =
> "semtimedop: syscall with kernel spec64"},
> 
> which is not what the ABI is meant to be anyway (sys_semtimedop takes
> a __kernel_old_timespec,
> not a __kernel_timespec).
> 
> Similarly, the line
> 
>  { .semop = semop, .type = TST_LIBC_TIMESPEC, .desc = "semop: vDSO or syscall"},
> 
> should apply to both 32 and 64 bit machines

This is how I modified this and all other time64 tests, I hope this looks fine now ?

diff --git a/testcases/kernel/syscalls/ipc/semop/semop.h b/testcases/kernel/syscalls/ipc/semop/semop.h
index 8d1245b65ec0..8647fa686bd3 100644
--- a/testcases/kernel/syscalls/ipc/semop/semop.h
+++ b/testcases/kernel/syscalls/ipc/semop/semop.h
@@ -26,17 +26,13 @@ struct test_variants {
        char *desc;
 } variants[] = {
        { .semop = semop, .type = TST_LIBC_TIMESPEC, .desc = "semop: vDSO or syscall"},
-#if defined(TST_ABI32)
-       { .semtimedop = sys_semtimedop, .type = TST_LIBC_TIMESPEC, .desc = "semtimedop: syscall with libc spec"},
-       { .semtimedop = sys_semtimedop, .type = TST_KERN_OLD_TIMESPEC, .desc = "semtimedop: syscall with kernel spec32"},
-#endif
 
-#if defined(TST_ABI64)
-       { .semtimedop = sys_semtimedop, .type = TST_KERN_TIMESPEC, .desc = "semtimedop: syscall with kernel spec64"},
+#if (__NR_semtimedop != __LTP__NR_INVALID_SYSCALL)
+       { .semtimedop = sys_semtimedop, .type = TST_KERN_OLD_TIMESPEC, .desc = "semtimedop: syscall with old kernel spec"},
 #endif
 
 #if (__NR_semtimedop_time64 != __LTP__NR_INVALID_SYSCALL)
-       { .semtimedop = sys_semtimedop_time64, .type = TST_KERN_TIMESPEC, .desc = "semtimedop: syscall time64 with kernel spec64"},
+       { .semtimedop = sys_semtimedop_time64, .type = TST_KERN_TIMESPEC, .desc = "semtimedop: syscall time64 with kernel spec"},
 #endif
 };

On Mon, May 11, 2020 at 8:49 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 08-05-20, 09:18, Arnd Bergmann wrote:
> >
> > Similarly, the line
> >
> >  { .semop = semop, .type = TST_LIBC_TIMESPEC, .desc = "semop: vDSO or syscall"},
> >
> > should apply to both 32 and 64 bit machines
>
> This is how I modified this and all other time64 tests, I hope this looks fine now ?
>
> diff --git a/testcases/kernel/syscalls/ipc/semop/semop.h b/testcases/kernel/syscalls/ipc/semop/semop.h
> index 8d1245b65ec0..8647fa686bd3 100644
> --- a/testcases/kernel/syscalls/ipc/semop/semop.h
> +++ b/testcases/kernel/syscalls/ipc/semop/semop.h
> @@ -26,17 +26,13 @@ struct test_variants {
>         char *desc;
>  } variants[] = {
>         { .semop = semop, .type = TST_LIBC_TIMESPEC, .desc = "semop: vDSO or syscall"},
> -#if defined(TST_ABI32)
> -       { .semtimedop = sys_semtimedop, .type = TST_LIBC_TIMESPEC, .desc = "semtimedop: syscall with libc spec"},
> -       { .semtimedop = sys_semtimedop, .type = TST_KERN_OLD_TIMESPEC, .desc = "semtimedop: syscall with kernel spec32"},
> -#endif
>
> -#if defined(TST_ABI64)
> -       { .semtimedop = sys_semtimedop, .type = TST_KERN_TIMESPEC, .desc = "semtimedop: syscall with kernel spec64"},
> +#if (__NR_semtimedop != __LTP__NR_INVALID_SYSCALL)
> +       { .semtimedop = sys_semtimedop, .type = TST_KERN_OLD_TIMESPEC, .desc = "semtimedop: syscall with old kernel spec"},
>  #endif
>
>  #if (__NR_semtimedop_time64 != __LTP__NR_INVALID_SYSCALL)
> -       { .semtimedop = sys_semtimedop_time64, .type = TST_KERN_TIMESPEC, .desc = "semtimedop: syscall time64 with kernel spec64"},
> +       { .semtimedop = sys_semtimedop_time64, .type = TST_KERN_TIMESPEC, .desc = "semtimedop: syscall time64 with kernel spec"},
>  #endif
>  };

Yes, much better. Please do it like this for all the tests.

It looks like you are still missing a test for the libc semtimedop function with
TST_LIBC_TIMESPEC, but perhaps this is in a different place?

There is one more complication regarding semtimedop, and that is how some
architectures never had this as a separate syscall but instead require calling
sys_ipc(SEMTIMEDOP, ...). All architectures that have sys_ipc() let you use
both SEMOP and SEMTIMEDOP with __kernel_old_timespec. Some
architectures define more than one of __NR_ipc, __NR_semtimedop and
__NR_semop.

As of linux-5.1, all 64-bit architectures at least have __NR_semtimedop, and
all 32-bit architectures at least have __NR_semtimedop64, plus whatever
other variants they supported before then.

      Arnd

On 11-05-20, 09:20, Arnd Bergmann wrote:
> It looks like you are still missing a test for the libc semtimedop function with
> TST_LIBC_TIMESPEC, but perhaps this is in a different place?

The libc test is missing for few of the syscalls, as it was already missing for
them because the headers may not have the definition.

And so I didn't add them separately.

On Mon, May 11, 2020 at 9:33 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 11-05-20, 09:20, Arnd Bergmann wrote:
> > It looks like you are still missing a test for the libc semtimedop function with
> > TST_LIBC_TIMESPEC, but perhaps this is in a different place?
>
> The libc test is missing for few of the syscalls, as it was already missing for
> them because the headers may not have the definition.
>
> And so I didn't add them separately.

Ok, I see. This one just looked curious because the you add tests for the kernel
syscall (semtimedop) in a file that checks an older libc syscall (semop).

The libc semop is implemented in terms of the kernel's semtimedop for
architectures that don't have their own semop by passing a NULL timeout
argument. We should still check that five variants of it (sys_semop,
sys_semtimedop, sys_semtimedop64, ipc(SEMTIMEDOP) and ipc(SEMOP))
correctly implement the semop semantics, but I would also note that
testing the differences of the time types is only meaningful when you
actually pass a non-NULL timeout.

      Arnd

On 09-05-20, 12:21, Li Wang wrote:
> RHEL8.2(x84_64) failed as:
> 
> # ./semop05
> tst_test.c:1246: INFO: Timeout per run is 0h 05m 00s
> semop05.c:147: INFO: Testing variant: semop: vDSO or syscall
> semop05.c:132: PASS: expected failure: EIDRM (43)
> semop05.c:132: PASS: expected failure: EIDRM (43)
> semop05.c:132: PASS: expected failure: EINTR (4)
> semop05.c:132: PASS: expected failure: EINTR (4)
> tst_test.c:1246: INFO: Timeout per run is 0h 05m 00s
> semop05.c:147: INFO: Testing variant: semtimedop: syscall with kernel spec64
> semop05.c:134: FAIL: unexpected failure: EAGAIN/EWOULDBLOCK (11)
> Test timeouted, sending SIGKILL!
> tst_test.c:1286: INFO: If you are running on slow machine, try exporting
> LTP_TIMEOUT_MUL > 1
> tst_test.c:1287: BROK: Test killed! (timeout?)
> 
> 
> RHEL8.2(s390x) failed as:
> 
> tst_test.c:1246: INFO: Timeout per run is 0h 05m 00s
> semop05.c:147: INFO: Testing variant: semop: vDSO or syscall
> semop05.c:132: PASS: expected failure: EIDRM (43)
> semop05.c:132: PASS: expected failure: EIDRM (43)
> semop05.c:132: PASS: expected failure: EINTR (4)
> semop05.c:132: PASS: expected failure: EINTR (4)
> tst_test.c:1246: INFO: Timeout per run is 0h 05m 00s
> semop05.c:147: INFO: Testing variant: semtimedop: syscall with kernel spec64
> semop.h:13: CONF: syscall(392) __NR_semtimedop not supported
> Test timeouted, sending SIGKILL!
> tst_test.c:1286: INFO: If you are running on slow machine, try exporting
> LTP_TIMEOUT_MUL > 1
> tst_test.c:1287: BROK: Test killed! (timeout?)

Some part of the timeout was left uninitialized and so was causing
some issues. Can you please try with this delta:

diff --git a/testcases/kernel/syscalls/ipc/semop/semop05.c b/testcases/kernel/syscalls/ipc/semop/semop05.c
index f892cd519ecf..0e18f77ba1f5 100644
--- a/testcases/kernel/syscalls/ipc/semop/semop05.c
+++ b/testcases/kernel/syscalls/ipc/semop/semop05.c
@@ -120,6 +120,7 @@ static void do_child(int i)
        struct tst_ts timeout;
 
        timeout.type = tv->type;
+       tst_ts_set_sec(&timeout, 1);
        tst_ts_set_nsec(&timeout, 10000);
 
        TEST(call_semop(tv, sem_id, &s_buf, 1, &timeout));

On 11-05-20, 09:58, Arnd Bergmann wrote:
> We should still check that five variants of it (sys_semop,
> sys_semtimedop, sys_semtimedop64, ipc(SEMTIMEDOP) and ipc(SEMOP))
> correctly implement the semop semantics

How do you suggest I add tests for ipc() types? Should they be always
tested? Or they should be tested only if the normal sys_* variant
isn't implemented by the kernel/arch ?

On Mon, May 11, 2020 at 6:43 PM Viresh Kumar <viresh.kumar@linaro.org>
wrote:

> On 09-05-20, 12:21, Li Wang wrote:
> > RHEL8.2(x84_64) failed as:
> >
> > # ./semop05
> > tst_test.c:1246: INFO: Timeout per run is 0h 05m 00s
> > semop05.c:147: INFO: Testing variant: semop: vDSO or syscall
> > semop05.c:132: PASS: expected failure: EIDRM (43)
> > semop05.c:132: PASS: expected failure: EIDRM (43)
> > semop05.c:132: PASS: expected failure: EINTR (4)
> > semop05.c:132: PASS: expected failure: EINTR (4)
> > tst_test.c:1246: INFO: Timeout per run is 0h 05m 00s
> > semop05.c:147: INFO: Testing variant: semtimedop: syscall with kernel
> spec64
> > semop05.c:134: FAIL: unexpected failure: EAGAIN/EWOULDBLOCK (11)
> > Test timeouted, sending SIGKILL!
> > tst_test.c:1286: INFO: If you are running on slow machine, try exporting
> > LTP_TIMEOUT_MUL > 1
> > tst_test.c:1287: BROK: Test killed! (timeout?)
> >
> >
> > RHEL8.2(s390x) failed as:
> >
> > tst_test.c:1246: INFO: Timeout per run is 0h 05m 00s
> > semop05.c:147: INFO: Testing variant: semop: vDSO or syscall
> > semop05.c:132: PASS: expected failure: EIDRM (43)
> > semop05.c:132: PASS: expected failure: EIDRM (43)
> > semop05.c:132: PASS: expected failure: EINTR (4)
> > semop05.c:132: PASS: expected failure: EINTR (4)
> > tst_test.c:1246: INFO: Timeout per run is 0h 05m 00s
> > semop05.c:147: INFO: Testing variant: semtimedop: syscall with kernel
> spec64
> > semop.h:13: CONF: syscall(392) __NR_semtimedop not supported
> > Test timeouted, sending SIGKILL!
> > tst_test.c:1286: INFO: If you are running on slow machine, try exporting
> > LTP_TIMEOUT_MUL > 1
> > tst_test.c:1287: BROK: Test killed! (timeout?)
>
> Some part of the timeout was left uninitialized and so was causing
> some issues. Can you please try with this delta:
>

This delta works(on x86_64) for me. But the s390x issue looks like the same
as the futex TCONF in another email.

On 11-05-20, 20:29, Li Wang wrote:
> This delta works(on x86_64) for me. But the s390x issue looks like the same
> as the futex TCONF in another email.

Ah, this should fix it then:

diff --git a/testcases/kernel/syscalls/ipc/semop/semop.h b/testcases/kernel/syscalls/ipc/semop/semop.h
index 7180f6e35185..584d12c68e0d 100644
--- a/testcases/kernel/syscalls/ipc/semop/semop.h
+++ b/testcases/kernel/syscalls/ipc/semop/semop.h
@@ -44,4 +44,12 @@ static inline int call_semop(struct test_variants *tv, int semid,
 	return tv->semtimedop(semid, sops, nsops, tst_ts_get(timeout));
 }
 
+static inline void semop_supported_by_kernel(struct test_variants *tv)
+{
+       /* Check if the syscall is implemented on the platform */
+       TEST(call_semop(tv, 0, NULL, 0, NULL));
+       if (TST_RET == -1 && TST_ERR == ENOSYS)
+               tst_brk(TCONF, "Test not supported on kernel/platform");
+}
+
 #endif /* SEMOP_VAR__ */
diff --git a/testcases/kernel/syscalls/ipc/semop/semop01.c b/testcases/kernel/syscalls/ipc/semop/semop01.c
index cc8725bdbed3..7947183c837b 100644
--- a/testcases/kernel/syscalls/ipc/semop/semop01.c
+++ b/testcases/kernel/syscalls/ipc/semop/semop01.c
@@ -99,9 +99,11 @@ static void run(void)
 
 static void setup(void)
 {
+	struct test_variants *tv = &variants[tst_variant];
 	int i;
 
-	tst_res(TINFO, "Testing variant: %s", variants[tst_variant].desc);
+	tst_res(TINFO, "Testing variant: %s", tv->desc);
+	semop_supported_by_kernel(tv);
 
 	get_arr.array = malloc(sizeof(unsigned short int) * PSEMS);
 	if (get_arr.array == NULL)
diff --git a/testcases/kernel/syscalls/ipc/semop/semop02.c b/testcases/kernel/syscalls/ipc/semop/semop02.c
index 5af1909480b5..4ffb9dd72c1c 100644
--- a/testcases/kernel/syscalls/ipc/semop/semop02.c
+++ b/testcases/kernel/syscalls/ipc/semop/semop02.c
@@ -47,13 +47,15 @@ static struct test_case_t {
 
 static void setup(void)
 {
+	struct test_variants *tv = &variants[tst_variant];
 	char nobody_uid[] = "nobody";
 	struct passwd *ltpuser;
 	key_t semkey2;
 	struct seminfo ipc_buf;
 	union semun arr;
 
-	tst_res(TINFO, "Testing variant: %s", variants[tst_variant].desc);
+	tst_res(TINFO, "Testing variant: %s", tv->desc);
+	semop_supported_by_kernel(tv);
 
 	ltpuser = SAFE_GETPWNAM(nobody_uid);
 	SAFE_SETUID(ltpuser->pw_uid);
diff --git a/testcases/kernel/syscalls/ipc/semop/semop03.c b/testcases/kernel/syscalls/ipc/semop/semop03.c
index e877f989edaa..40a6b095c57d 100644
--- a/testcases/kernel/syscalls/ipc/semop/semop03.c
+++ b/testcases/kernel/syscalls/ipc/semop/semop03.c
@@ -88,7 +88,10 @@ static void run(unsigned int i)
 
 static void setup(void)
 {
-	tst_res(TINFO, "Testing variant: %s", variants[tst_variant].desc);
+	struct test_variants *tv = &variants[tst_variant];
+
+	tst_res(TINFO, "Testing variant: %s", tv->desc);
+	semop_supported_by_kernel(tv);
 
 	/* get an IPC resource key */
 	semkey = GETIPCKEY();
diff --git a/testcases/kernel/syscalls/ipc/semop/semop04.c b/testcases/kernel/syscalls/ipc/semop/semop04.c
index 226a64603943..5e4bfeb625e2 100644
--- a/testcases/kernel/syscalls/ipc/semop/semop04.c
+++ b/testcases/kernel/syscalls/ipc/semop/semop04.c
@@ -94,7 +94,10 @@ static void run(unsigned int i)
 
 static void setup(void)
 {
-	tst_res(TINFO, "Testing variant: %s", variants[tst_variant].desc);
+	struct test_variants *tv = &variants[tst_variant];
+
+	tst_res(TINFO, "Testing variant: %s", tv->desc);
+	semop_supported_by_kernel(tv);
 
 	val = 1;
 
diff --git a/testcases/kernel/syscalls/ipc/semop/semop05.c b/testcases/kernel/syscalls/ipc/semop/semop05.c
index 0e18f77ba1f5..770646bc564c 100644
--- a/testcases/kernel/syscalls/ipc/semop/semop05.c
+++ b/testcases/kernel/syscalls/ipc/semop/semop05.c
@@ -145,7 +145,10 @@ static void sighandler(int sig)
 
 static void setup(void)
 {
-	tst_res(TINFO, "Testing variant: %s", variants[tst_variant].desc);
+	struct test_variants *tv = &variants[tst_variant];
+
+	tst_res(TINFO, "Testing variant: %s", tv->desc);
+	semop_supported_by_kernel(tv);
 
 	SAFE_SIGNAL(SIGHUP, sighandler);