syscalls/setitimer: Pass the kernel-defined struct __kernel_old_itimerval to sys_setitimer().

The setitimer01 tests fail on RV32 due to incompatible parameter
definitions. 'value' and 'ovalue' are defined by glibc as itimerval
(64-bit time_t on RV32), while the kernel expects __kernel_old_itimerval
(32-bit time_t on RV32) for the setitimer syscall[1]. This discrepancy
leads to incorrect 'ovalue' and test failures. Thus, the kernel-defined
__kernel_old_itimerval should be used.

[1] https://sourceware.org/git/?p=glibc.git;a=commit;h=a51e03588937ad804a9f583ea3d0fc0a4d088c33
Signed-off-by: Hui Min Mina Chou <minachou@andestech.com>
---
 configure.ac                                      |  3 ++-
 include/tst_timer.h                               | 12 ++++++++++++
 testcases/kernel/syscalls/setitimer/setitimer01.c |  8 ++------
 testcases/kernel/syscalls/setitimer/setitimer02.c | 12 ++++--------
 4 files changed, 20 insertions(+), 15 deletions(-)

Hi all,

> +++ b/configure.ac
> @@ -219,7 +219,8 @@ AC_CHECK_TYPES([struct xt_entry_match, struct xt_entry_target],,,[
>  ])

>  AC_CHECK_TYPES([struct __kernel_old_timeval, struct __kernel_old_timespec, struct __kernel_timespec,
> -                struct __kernel_old_itimerspec, struct __kernel_itimerspec],,,[#include <sys/socket.h>])
> +                struct __kernel_old_itimerspec, struct __kernel_itimerspec,
> +                struct __kernel_old_itimerval],,,[#include <sys/socket.h>])

@Hui following is unrelated to your patchset (we can sort it later, it should
not block your effort).

@Cyril the original code prior this patchset in 203ee275c ("Fix struct
__kernel_old_timeval redefinition on 64bit sparc") did not include
<linux/time_types.h> for some reason IMHO fallbacks were always used.
I wonder why and whether we still don't want to use <linux/time_types.h>.

Then Fabrice's fix in 12986b755 ("include/tst_timer.h: avoid redefinition of
kernel structures") add autotools check just for uncommon toolchain (sh4 from
Texas Instruments). It's somehow hidden (due missing comment it looks like we
mostly get the definitions from header, but obviously not when we include
<sys/socket.h>.

>  AC_CHECK_TYPES([struct futex_waitv],,,[#include <linux/futex.h>])
>  AC_CHECK_TYPES([struct mount_attr],,,[
> diff --git a/include/tst_timer.h b/include/tst_timer.h
> index 703f03294eae..6fb9400206b8 100644
> --- a/include/tst_timer.h
> +++ b/include/tst_timer.h
> @@ -135,6 +135,13 @@ struct __kernel_itimerspec {
>  	struct __kernel_timespec it_value;       /* timer expiration */
>  };
>  #endif
> +
> +#ifndef HAVE_STRUCT___KERNEL_OLD_ITIMERVAL
> +struct __kernel_old_itimerval {
> +	struct __kernel_old_timeval it_interval;	/* timer interval */
> +	struct __kernel_old_timeval it_value;		/* current value */
> +};
> +#endif
>  #endif

>  enum tst_ts_type {
> @@ -370,6 +377,11 @@ static inline int sys_timerfd_settime64(int fd, int flags, void *its,
>  	return tst_syscall(__NR_timerfd_settime64, fd, flags, its, old_its);
>  }

> +static inline int sys_setitimer(int which, void *new_value, void *old_value)
> +{
> +	return tst_syscall(__NR_setitimer, which, new_value, old_value);
> +}

+1 adding function to the common place.

IMHO we slightly prefer to add C functions to C file (e.g. lib/tst_timer.c,
there are other functions) + adding signature to tst_timer.h.

With that:
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

> +
>  /*
>   * Returns tst_ts seconds.
>   */
> diff --git a/testcases/kernel/syscalls/setitimer/setitimer01.c b/testcases/kernel/syscalls/setitimer/setitimer01.c
> index d12abe904f1c..94ee51c6a667 100644
> --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
> +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
> @@ -20,9 +20,10 @@
>  #include "tst_test.h"
>  #include "lapi/syscalls.h"
>  #include "tst_safe_clocks.h"
> +#include "tst_timer.h"

>  static struct timeval tv;
> -static struct itimerval *value, *ovalue;
> +static struct __kernel_old_itimerval *value, *ovalue;
>  static volatile unsigned long sigcnt;
>  static long time_step;
>  static long time_sec;
> @@ -38,11 +39,6 @@ static struct tcase {
>  	{ITIMER_PROF,    "ITIMER_PROF",    SIGPROF},
>  };

> -static int sys_setitimer(int which, void *new_value, void *old_value)
> -{
> -	return tst_syscall(__NR_setitimer, which, new_value, old_value);
> -}
> -
>  static void sig_routine(int signo LTP_ATTRIBUTE_UNUSED)
>  {
>  	sigcnt++;
> diff --git a/testcases/kernel/syscalls/setitimer/setitimer02.c b/testcases/kernel/syscalls/setitimer/setitimer02.c
> index b012d71fa7bd..c545f6288bbb 100644
> --- a/testcases/kernel/syscalls/setitimer/setitimer02.c
> +++ b/testcases/kernel/syscalls/setitimer/setitimer02.c
> @@ -19,13 +19,9 @@
>  #include <stdlib.h>
>  #include "tst_test.h"
>  #include "lapi/syscalls.h"
> +#include "tst_timer.h"

> -static struct itimerval *value, *ovalue;
> -
> -static int sys_setitimer(int which, void *new_value, void *old_value)
> -{
> -	return tst_syscall(__NR_setitimer, which, new_value, old_value);
> -}
> +static struct __kernel_old_itimerval *value, *ovalue;

>  static void verify_setitimer(unsigned int i)
>  {
> @@ -55,8 +51,8 @@ static struct tst_test test = {
>  	.test = verify_setitimer,
>  	.setup = setup,
>  	.bufs = (struct tst_buffers[]) {
> -		{&value,  .size = sizeof(struct itimerval)},
> -		{&ovalue, .size = sizeof(struct itimerval)},
> +		{&value,  .size = sizeof(struct __kernel_old_itimerval)},
> +		{&ovalue, .size = sizeof(struct __kernel_old_itimerval)},
>  		{}
>  	}
>  };

Hi!
> @Cyril the original code prior this patchset in 203ee275c ("Fix struct
> __kernel_old_timeval redefinition on 64bit sparc") did not include
> <linux/time_types.h> for some reason IMHO fallbacks were always used.
> I wonder why and whether we still don't want to use <linux/time_types.h>.

I suppose that this is broken on some old distro, try to run that
through CI and if that passes we can do so.

> Then Fabrice's fix in 12986b755 ("include/tst_timer.h: avoid redefinition of
> kernel structures") add autotools check just for uncommon toolchain (sh4 from
> Texas Instruments). It's somehow hidden (due missing comment it looks like we
> mostly get the definitions from header, but obviously not when we include
> <sys/socket.h>.

I guess that it depends on architecture/libc/kernel headers and it's a
big mess...

> >  AC_CHECK_TYPES([struct futex_waitv],,,[#include <linux/futex.h>])
> >  AC_CHECK_TYPES([struct mount_attr],,,[
> > diff --git a/include/tst_timer.h b/include/tst_timer.h
> > index 703f03294eae..6fb9400206b8 100644
> > --- a/include/tst_timer.h
> > +++ b/include/tst_timer.h
> > @@ -135,6 +135,13 @@ struct __kernel_itimerspec {
> >  	struct __kernel_timespec it_value;       /* timer expiration */
> >  };
> >  #endif
> > +
> > +#ifndef HAVE_STRUCT___KERNEL_OLD_ITIMERVAL
> > +struct __kernel_old_itimerval {
> > +	struct __kernel_old_timeval it_interval;	/* timer interval */
> > +	struct __kernel_old_timeval it_value;		/* current value */
> > +};
> > +#endif
> >  #endif


I've been staring at the kernel and libc code for a while and it seems
that there is not itimerval64 syscall and interval timers are limited to
32bit on 32bit architectures. In reality I suppose that it does not
matter since nobody is going to use intervals that actually need 64bit
amount of seconds anyways.

So libc takes 64bit itimer, converts it to 32bit and kernel does
the oposite conversion.

Also we should really add tests for the libc wrapper as well, since that
is actually more likely to get broken by the double conversion on 32bit
arch, but that should be done in an subsequent patches.

> >  enum tst_ts_type {
> > @@ -370,6 +377,11 @@ static inline int sys_timerfd_settime64(int fd, int flags, void *its,
> >  	return tst_syscall(__NR_timerfd_settime64, fd, flags, its, old_its);
> >  }
> 
> > +static inline int sys_setitimer(int which, void *new_value, void *old_value)
> > +{
> > +	return tst_syscall(__NR_setitimer, which, new_value, old_value);
> > +}
> C
> +1 adding function to the common place.
> 
> IMHO we slightly prefer to add C functions to C file (e.g. lib/tst_timer.c,
> there are other functions) + adding signature to tst_timer.h.

I would say that there is no point to do that for a single line fuctions
like this and actually I guess that this would break the line numbers
and filenames for the tst_sycall() so it's better this way.

So for the patch as it is:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

Hi all,

thanks, patchset merged.

> Hi!
> > @Cyril the original code prior this patchset in 203ee275c ("Fix struct
> > __kernel_old_timeval redefinition on 64bit sparc") did not include
> > <linux/time_types.h> for some reason IMHO fallbacks were always used.
> > I wonder why and whether we still don't want to use <linux/time_types.h>.

> I suppose that this is broken on some old distro, try to run that
> through CI and if that passes we can do so.

Right. Maybe this could then replace <sys/socket.h>, but I would have to verify
it running also Buildroot CI.

> > Then Fabrice's fix in 12986b755 ("include/tst_timer.h: avoid redefinition of
> > kernel structures") add autotools check just for uncommon toolchain (sh4 from
> > Texas Instruments). It's somehow hidden (due missing comment it looks like we
> > mostly get the definitions from header, but obviously not when we include
> > <sys/socket.h>.

> I guess that it depends on architecture/libc/kernel headers and it's a
> big mess...

Yes, uncommon toolchain.

> > >  AC_CHECK_TYPES([struct futex_waitv],,,[#include <linux/futex.h>])
> > >  AC_CHECK_TYPES([struct mount_attr],,,[
> > > diff --git a/include/tst_timer.h b/include/tst_timer.h
> > > index 703f03294eae..6fb9400206b8 100644
> > > --- a/include/tst_timer.h
> > > +++ b/include/tst_timer.h
> > > @@ -135,6 +135,13 @@ struct __kernel_itimerspec {
> > >  	struct __kernel_timespec it_value;       /* timer expiration */
> > >  };
> > >  #endif
> > > +
> > > +#ifndef HAVE_STRUCT___KERNEL_OLD_ITIMERVAL
> > > +struct __kernel_old_itimerval {
> > > +	struct __kernel_old_timeval it_interval;	/* timer interval */
> > > +	struct __kernel_old_timeval it_value;		/* current value */
> > > +};
> > > +#endif
> > >  #endif


> I've been staring at the kernel and libc code for a while and it seems
> that there is not itimerval64 syscall and interval timers are limited to
> 32bit on 32bit architectures. In reality I suppose that it does not
> matter since nobody is going to use intervals that actually need 64bit
> amount of seconds anyways.

> So libc takes 64bit itimer, converts it to 32bit and kernel does
> the oposite conversion.

Thanks for investigation.

> Also we should really add tests for the libc wrapper as well, since that
> is actually more likely to get broken by the double conversion on 32bit
> arch, but that should be done in an subsequent patches.

+1. Adding .test_variants with {g,s}etitimer() for {g,s}etitimer0[12].c should
be trivial. Or did you mean to have test variants also for other functions in
include/tst_timer.h (e.g. timerfd_{g,s}ettime()) ?

BTW we already test {g,s}etitimer() libc wrapper in getitimer01.c, therefore we
already have libc wrapper at least somehow covered (the other two test raw
syscall).

Once we agree what to do I'll either post a patch or add a ticket for it (or
feel free to add a ticket yourself).

> > >  enum tst_ts_type {
> > > @@ -370,6 +377,11 @@ static inline int sys_timerfd_settime64(int fd, int flags, void *its,
> > >  	return tst_syscall(__NR_timerfd_settime64, fd, flags, its, old_its);
> > >  }

> > > +static inline int sys_setitimer(int which, void *new_value, void *old_value)
> > > +{
> > > +	return tst_syscall(__NR_setitimer, which, new_value, old_value);
> > > +}
> > C
> > +1 adding function to the common place.

> > IMHO we slightly prefer to add C functions to C file (e.g. lib/tst_timer.c,
> > there are other functions) + adding signature to tst_timer.h.

> I would say that there is no point to do that for a single line fuctions
> like this and actually I guess that this would break the line numbers
> and filenames for the tst_sycall() so it's better this way.

Ah, I was wrong. There are also many other single line functions in
include/tst_timer.h.

Kind regards,
Petr

> So for the patch as it is:

> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

Hi Cyril,

> Hi all,

...
> > Also we should really add tests for the libc wrapper as well, since that
> > is actually more likely to get broken by the double conversion on 32bit
> > arch, but that should be done in an subsequent patches.

> +1. Adding .test_variants with {g,s}etitimer() for {g,s}etitimer0[12].c should
> be trivial. Or did you mean to have test variants also for other functions in
> include/tst_timer.h (e.g. timerfd_{g,s}ettime()) ?

timerfd_{g,s}ettime() (and other functions) test raw syscall with variants for
64bit and the old 32bit. These tests (in testcases/kernel/syscalls/timerfd) are
already quite complicated. I'm not sure if it's a good idea to add also
timerfd_{g,s}ettime() wrappers. But they should be covered as well, right?

> BTW we already test {g,s}etitimer() libc wrapper in getitimer01.c, therefore we
> already have libc wrapper at least somehow covered (the other two test raw
> syscall).

And we of course test {g,s}etitimer() libc wrappers in open posix testsuite.

Kind regards,
Petr

> Once we agree what to do I'll either post a patch or add a ticket for it (or
> feel free to add a ticket yourself).
...