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

[Arnd Bergmann]

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