[LTP] [PATCH 0/5] syscalls: Remove incorrect usage of libc structures

[LTP] [PATCH 0/5] syscalls: Remove incorrect usage of libc structures

[Viresh Kumar]

Hi,

It is incorrect to use the libc structures (timespec, timeval, timex),
to the non-timex64 syscalls called via tst_syscall() as the kernel
expects the old structures in there and the libc structure definition
may change in the future.

Arnd: It was getting difficult to search for such instances and so I
searched it with following strings (search for files that use these
structures, as well as tst_syscall()) to catch the abuse, hope it covers
all cases.

git grep "struct timeval" `git grep -l tst_syscall`
git grep "struct timespec" `git grep -l tst_syscall`
git grep "struct timex" `git grep -l tst_syscall`

Viresh Kumar (5):
  tst_safe_clocks: Remove safe_clock_adjtime()
  syscalls: settimeofday: Use gettimeofday()
  syscalls: Don't use tst_syscall() unnecessarily
  syscalls: Don't pass struct timespec to tst_syscall()
  syscalls: Don't pass struct timeval to tst_syscall()

 include/tst_clocks.h                          |  8 ++++---
 include/tst_safe_clocks.h                     | 18 ---------------
 include/tst_timer.h                           |  6 +++++
 lib/parse_opts.c                              |  3 +--
 lib/tst_clocks.c                              |  6 ++---
 lib/tst_test.c                                | 16 ++++++-------
 lib/tst_timer.c                               | 23 ++++++++++++-------
 lib/tst_timer_test.c                          |  2 +-
 lib/tst_wallclock.c                           | 17 +++++++-------
 testcases/cve/cve-2016-7117.c                 |  2 +-
 .../syscalls/clock_adjtime/clock_adjtime.h    |  5 ----
 .../syscalls/gettimeofday/gettimeofday02.c    |  3 ++-
 .../syscalls/settimeofday/settimeofday01.c    |  6 ++---
 .../syscalls/settimeofday/settimeofday02.c    |  2 +-
 testcases/kernel/syscalls/stime/stime_var.h   |  3 ++-
 15 files changed, 57 insertions(+), 63 deletions(-)

-- 
2.25.0.rc1.19.g042ed3e048af

[LTP] [PATCH 1/5] tst_safe_clocks: Remove safe_clock_adjtime()

[Viresh Kumar]

safe_clock_adjtime() isn't used anymore, remove it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/tst_safe_clocks.h | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/include/tst_safe_clocks.h b/include/tst_safe_clocks.h
index 27e8bda45589..4cb5f41ed82f 100644
--- a/include/tst_safe_clocks.h
+++ b/include/tst_safe_clocks.h
@@ -55,21 +55,6 @@ static inline void safe_clock_settime(const char *file, const int lineno,
 	}
 }
 
-static inline int safe_clock_adjtime(const char *file, const int lineno,
-	clockid_t clk_id, struct timex *txc)
-{
-	int rval;
-
-	rval = tst_syscall(__NR_clock_adjtime, clk_id, txc);
-	if (rval < 0) {
-		tst_brk(TBROK | TERRNO,
-			"%s:%d clock_adjtime(%s) failed %i",
-			file, lineno, tst_clock_name(clk_id), rval);
-	}
-
-	return rval;
-}
-
 #define SAFE_CLOCK_GETRES(clk_id, res)\
 	safe_clock_getres(__FILE__, __LINE__, (clk_id), (res))
 
@@ -79,7 +64,4 @@ static inline int safe_clock_adjtime(const char *file, const int lineno,
 #define SAFE_CLOCK_SETTIME(clk_id, tp)\
 	safe_clock_settime(__FILE__, __LINE__, (clk_id), (tp))
 
-#define SAFE_CLOCK_ADJTIME(clk_id, txc)\
-	safe_clock_adjtime(__FILE__, __LINE__, (clk_id), (txc))
-
 #endif /* SAFE_CLOCKS_H__ */
-- 
2.25.0.rc1.19.g042ed3e048af

[LTP] [PATCH 2/5] syscalls: settimeofday: Use gettimeofday()

[Viresh Kumar]

Use gettimeofday() instead of calling it with tst_syscall().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 testcases/kernel/syscalls/settimeofday/settimeofday01.c | 6 +++---
 testcases/kernel/syscalls/settimeofday/settimeofday02.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/syscalls/settimeofday/settimeofday01.c b/testcases/kernel/syscalls/settimeofday/settimeofday01.c
index 368fdebc0c8e..c599a820fc97 100644
--- a/testcases/kernel/syscalls/settimeofday/settimeofday01.c
+++ b/testcases/kernel/syscalls/settimeofday/settimeofday01.c
@@ -23,7 +23,7 @@ static void verify_settimeofday(void)
 	suseconds_t delta;
 	struct timeval tv1, tv2;
 
-	if (tst_syscall(__NR_gettimeofday, &tv1, NULL) == -1)
+	if (gettimeofday(&tv1, NULL) == -1)
 		tst_brk(TBROK | TERRNO, "gettimeofday(&tv1, NULL) failed");
 
 	tv1.tv_sec += VAL_SEC;
@@ -37,7 +37,7 @@ static void verify_settimeofday(void)
 		return;
 	}
 
-	if (tst_syscall(__NR_gettimeofday, &tv2, NULL) == -1)
+	if (gettimeofday(&tv2, NULL) == -1)
 		tst_brk(TBROK | TERRNO, "gettimeofday(&tv2, NULL) failed");
 
 	if (tv2.tv_sec > tv1.tv_sec) {
@@ -58,7 +58,7 @@ static void verify_settimeofday(void)
 
 static void setup(void)
 {
-	if (tst_syscall(__NR_gettimeofday, &tv_saved, NULL) == -1)
+	if (gettimeofday(&tv_saved, NULL) == -1)
 		tst_brk(TBROK | TERRNO, "gettimeofday(&tv_saved, NULL) failed");
 }
 
diff --git a/testcases/kernel/syscalls/settimeofday/settimeofday02.c b/testcases/kernel/syscalls/settimeofday/settimeofday02.c
index 485a26b1d9c5..0d6862eb33b1 100644
--- a/testcases/kernel/syscalls/settimeofday/settimeofday02.c
+++ b/testcases/kernel/syscalls/settimeofday/settimeofday02.c
@@ -46,7 +46,7 @@ static void verify_settimeofday(unsigned int n)
 
 static void setup(void)
 {
-	if (tst_syscall(__NR_gettimeofday, &tv_saved, NULL) == -1)
+	if (gettimeofday(&tv_saved, NULL) == -1)
 		tst_brk(TBROK | TERRNO, "gettimeofday(&tv_saved, NULL) failed");
 }
 
-- 
2.25.0.rc1.19.g042ed3e048af

[LTP] [PATCH 2/5] syscalls: settimeofday: Use gettimeofday()

[Arnd Bergmann]

On Tue, May 19, 2020 at 10:51 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Use gettimeofday() instead of calling it with tst_syscall().
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

I think the change makes it work reliably, but it does change what you
are testing for: instead of testing the low-level system call interface,
this will now test the libc interface, which is implemented on top
of the vdso or clock_gettime().

I think all variants (vdso, syscall(__NR_gettimeofday), libc
gettimeofday, emulation with clock_gettime syscall/vdso/libc)
need to be tested. It's possible they all are, but that should
be clarified in the changelog text.

       Arnd

[LTP] [PATCH 2/5] syscalls: settimeofday: Use gettimeofday()

[Viresh Kumar]

On 19-05-20, 11:20, Arnd Bergmann wrote:
> On Tue, May 19, 2020 at 10:51 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Use gettimeofday() instead of calling it with tst_syscall().
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> 
> I think the change makes it work reliably, but it does change what you
> are testing for: instead of testing the low-level system call interface,
> this will now test the libc interface, which is implemented on top
> of the vdso or clock_gettime().

Actually the testcase was for settimeofday() and we were unnecessarily
calling gettimeofday with tst_syscall(). And so the testcase should
remain unaffected that way. Had this been a testcase for
gettimeofday(), I would have agreed with you.

> I think all variants (vdso, syscall(__NR_gettimeofday), libc
> gettimeofday, emulation with clock_gettime syscall/vdso/libc)
> need to be tested. It's possible they all are, but that should
> be clarified in the changelog text.

They aren't tested that way.

-- 
viresh

[LTP] [PATCH 2/5] syscalls: settimeofday: Use gettimeofday()

[Arnd Bergmann]

On Tue, May 19, 2020 at 11:25 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 19-05-20, 11:20, Arnd Bergmann wrote:
> > On Tue, May 19, 2020 at 10:51 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > Use gettimeofday() instead of calling it with tst_syscall().
> > >
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> >
> > I think the change makes it work reliably, but it does change what you
> > are testing for: instead of testing the low-level system call interface,
> > this will now test the libc interface, which is implemented on top
> > of the vdso or clock_gettime().
>
> Actually the testcase was for settimeofday() and we were unnecessarily
> calling gettimeofday with tst_syscall(). And so the testcase should
> remain unaffected that way. Had this been a testcase for
> gettimeofday(), I would have agreed with you.

Ok, makes sense. Just mention in the changelog text that this fixes
running against an updated libc with 64-bit time_t in that case.

       Arnd

[LTP] [PATCH 2/5] syscalls: settimeofday: Use gettimeofday()

[Cyril Hrubis]

Hi!
You can set the .restore_wallclock flag in the tst_test structure for
these two tests and remove the setup() and cleanup() as well.

Also in the settimeofday02 I would be inclined to just remove the clock
restoration code, since there is no way the tim will be changed unless
the kernel is buggy.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/5] syscalls: settimeofday: Use gettimeofday()

[Arnd Bergmann]

On Tue, May 19, 2020 at 2:15 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> You can set the .restore_wallclock flag in the tst_test structure for
> these two tests and remove the setup() and cleanup() as well.
>
> Also in the settimeofday02 I would be inclined to just remove the clock
> restoration code, since there is no way the tim will be changed unless
> the kernel is buggy.

Ah, I did not realize that LTP actually does try to set the clock and then
set it back. If it does that, it may be very interesting to test the behavior
across the y2038 overflow, e.g. set the time ot just after the 2038-01-19
expiration and ensure the kernel still behaves as expected, or set the
timer to just before the overflow and set a timer just after, and see that
the timer triggers.

      Arnd

[LTP] [PATCH 2/5] syscalls: settimeofday: Use gettimeofday()

[Viresh Kumar]

On 19-05-20, 14:16, Cyril Hrubis wrote:
> Hi!
> You can set the .restore_wallclock flag in the tst_test structure for
> these two tests and remove the setup() and cleanup() as well.

Done.

> Also in the settimeofday02 I would be inclined to just remove the clock
> restoration code, since there is no way the tim will be changed unless
> the kernel is buggy.

If I understand it correctly, you now suggested to get rid of the
.restore_wallclock() flag from 02.c ? I think it is better to keep it as the
kernel can be buggy and we must get back to a working state in such a case. So,
I didn't remove it :)

-- 
viresh

[LTP] [PATCH 2/5] syscalls: settimeofday: Use gettimeofday()

[Cyril Hrubis]

Hi!
> > Also in the settimeofday02 I would be inclined to just remove the clock
> > restoration code, since there is no way the tim will be changed unless
> > the kernel is buggy.
> 
> If I understand it correctly, you now suggested to get rid of the
> .restore_wallclock() flag from 02.c ? I think it is better to keep it as the
> kernel can be buggy and we must get back to a working state in such a case. So,
> I didn't remove it :)

If kernel is buggy, all bets are off and we do not guarantee to leave
the system in an usable state.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 3/5] syscalls: Don't use tst_syscall() unnecessarily

[Viresh Kumar]

These syscall are old enough and must have support in libc for everyone.
Don't use tst_syscall() for them unnecessarily.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 lib/parse_opts.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/parse_opts.c b/lib/parse_opts.c
index a9d50589a3f9..b3ab69c0a539 100644
--- a/lib/parse_opts.c
+++ b/lib/parse_opts.c
@@ -45,7 +45,6 @@
 #include "test.h"
 #include "ltp_priv.h"
 #include "usctest.h"
-#include "tst_clocks.h"
 
 #ifndef UNIT_TEST
 #define UNIT_TEST	0
@@ -472,7 +471,7 @@ static uint64_t get_current_time(void)
 {
 	struct timespec ts;
 
-	tst_clock_gettime(CLOCK_MONOTONIC, &ts);
+	clock_gettime(CLOCK_MONOTONIC, &ts);
 
 	return (((uint64_t) ts.tv_sec) * USECS_PER_SEC) + ts.tv_nsec / 1000;
 }
-- 
2.25.0.rc1.19.g042ed3e048af

[LTP] [PATCH 3/5] syscalls: Don't use tst_syscall() unnecessarily

[Arnd Bergmann]

On Tue, May 19, 2020 at 10:51 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> @@ -472,7 +471,7 @@ static uint64_t get_current_time(void)
>  {
>         struct timespec ts;
>
> -       tst_clock_gettime(CLOCK_MONOTONIC, &ts);
> +       clock_gettime(CLOCK_MONOTONIC, &ts);

Again, you are changing from the low-level syscall interface that is not
present on new 32-bit architectures which only have clock_gettime64
to a high-level interface.

This change correctly changes it to pass matching timespec variant, but
the changelog text does not mention that.

      Arnd

[LTP] [PATCH 3/5] syscalls: Don't use tst_syscall() unnecessarily

[Viresh Kumar]

On 19-05-20, 11:22, Arnd Bergmann wrote:
> On Tue, May 19, 2020 at 10:51 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> > @@ -472,7 +471,7 @@ static uint64_t get_current_time(void)
> >  {
> >         struct timespec ts;
> >
> > -       tst_clock_gettime(CLOCK_MONOTONIC, &ts);
> > +       clock_gettime(CLOCK_MONOTONIC, &ts);
> 
> Again, you are changing from the low-level syscall interface that is not
> present on new 32-bit architectures which only have clock_gettime64
> to a high-level interface.
> 
> This change correctly changes it to pass matching timespec variant, but
> the changelog text does not mention that.

Right, I can actually mention why this change was necessary.

-- 
viresh

[LTP] [PATCH 3/5] syscalls: Don't use tst_syscall() unnecessarily

[Cyril Hrubis]

Hi!
> These syscall are old enough and must have support in libc for everyone.
> Don't use tst_syscall() for them unnecessarily.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  lib/parse_opts.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/lib/parse_opts.c b/lib/parse_opts.c
> index a9d50589a3f9..b3ab69c0a539 100644
> --- a/lib/parse_opts.c
> +++ b/lib/parse_opts.c
> @@ -45,7 +45,6 @@
>  #include "test.h"
>  #include "ltp_priv.h"
>  #include "usctest.h"
> -#include "tst_clocks.h"
>  
>  #ifndef UNIT_TEST
>  #define UNIT_TEST	0
> @@ -472,7 +471,7 @@ static uint64_t get_current_time(void)
>  {
>  	struct timespec ts;
>  
> -	tst_clock_gettime(CLOCK_MONOTONIC, &ts);
> +	clock_gettime(CLOCK_MONOTONIC, &ts);

I guess that this will reintroduce LTP compilation failures on older
glibc, which was the primary reason we used the tst_clock_gettime()
instead of clock_gettime().

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 3/5] syscalls: Don't use tst_syscall() unnecessarily

[Arnd Bergmann]

On Tue, May 19, 2020 at 2:23 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> > These syscall are old enough and must have support in libc for everyone.
> > Don't use tst_syscall() for them unnecessarily.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  lib/parse_opts.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/lib/parse_opts.c b/lib/parse_opts.c
> > index a9d50589a3f9..b3ab69c0a539 100644
> > --- a/lib/parse_opts.c
> > +++ b/lib/parse_opts.c
> > @@ -45,7 +45,6 @@
> >  #include "test.h"
> >  #include "ltp_priv.h"
> >  #include "usctest.h"
> > -#include "tst_clocks.h"
> >
> >  #ifndef UNIT_TEST
> >  #define UNIT_TEST    0
> > @@ -472,7 +471,7 @@ static uint64_t get_current_time(void)
> >  {
> >       struct timespec ts;
> >
> > -     tst_clock_gettime(CLOCK_MONOTONIC, &ts);
> > +     clock_gettime(CLOCK_MONOTONIC, &ts);
>
> I guess that this will reintroduce LTP compilation failures on older
> glibc, which was the primary reason we used the tst_clock_gettime()
> instead of clock_gettime().

I see that clock_gettime was first added in glibc-2.1.3 back in 1999.
Can that actually run LTP any more? If it can and this is considered
important, I fear the tst_clock_gettime() call needs to be extended
to call the clock_gettime()/clock_gettime64()/gettimeofday() syscalls,
whichever is the first to work, and convert the formats from the
native kernel format to the glibc format.

         Arnd

[LTP] [PATCH 3/5] syscalls: Don't use tst_syscall() unnecessarily

[Petr Vorel]

Hi,

> > > -     tst_clock_gettime(CLOCK_MONOTONIC, &ts);
> > > +     clock_gettime(CLOCK_MONOTONIC, &ts);

> > I guess that this will reintroduce LTP compilation failures on older
> > glibc, which was the primary reason we used the tst_clock_gettime()
> > instead of clock_gettime().

> I see that clock_gettime was first added in glibc-2.1.3 back in 1999.
> Can that actually run LTP any more? If it can and this is considered
> important, I fear the tst_clock_gettime() call needs to be extended
> to call the clock_gettime()/clock_gettime64()/gettimeofday() syscalls,
> whichever is the first to work, and convert the formats from the
> native kernel format to the glibc format.
IMHO the older system we still test in Travis (but going to remove it soon) is
CentOS 6 (kernel 3.10, glibc 2.12, gcc 4.4.7). I suspect that it was needed this
system (e.g. system with old glibc and gcc; gcc required some fixes which
bothered me, but old glibc actually caught some bugs in fallback which we
wouldn't otherwise find). Or am I wrong?

We agreed (few LTP maintainers), that, at least for SUSE and Red Hat is ok to
drop support for distros 10+ years, because these systems are tested with some
older LTP release anyway.

>          Arnd

Kind regards,
Petr

[LTP] [PATCH 3/5] syscalls: Don't use tst_syscall() unnecessarily

[Cyril Hrubis]

Hi!
> > > -     tst_clock_gettime(CLOCK_MONOTONIC, &ts);
> > > +     clock_gettime(CLOCK_MONOTONIC, &ts);
> >
> > I guess that this will reintroduce LTP compilation failures on older
> > glibc, which was the primary reason we used the tst_clock_gettime()
> > instead of clock_gettime().
> 
> I see that clock_gettime was first added in glibc-2.1.3 back in 1999.
> Can that actually run LTP any more? If it can and this is considered
> important, I fear the tst_clock_gettime() call needs to be extended
> to call the clock_gettime()/clock_gettime64()/gettimeofday() syscalls,
> whichever is the first to work, and convert the formats from the
> native kernel format to the glibc format.

I guess that at the current time we do support distros that are at max
10 years old, mostly because enterprise support cycles are about 10
years in lenght.

The issue here is that glibc needed -lrt passed to linker couple of
years ago and we wanted to avoid the need of linking everything with
-lrt, as calling the raw syscall was just easier fix.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 3/5] syscalls: Don't use tst_syscall() unnecessarily

[Viresh Kumar]

On 19-05-20, 15:45, Cyril Hrubis wrote:
> Hi!
> > > > -     tst_clock_gettime(CLOCK_MONOTONIC, &ts);
> > > > +     clock_gettime(CLOCK_MONOTONIC, &ts);
> > >
> > > I guess that this will reintroduce LTP compilation failures on older
> > > glibc, which was the primary reason we used the tst_clock_gettime()
> > > instead of clock_gettime().
> > 
> > I see that clock_gettime was first added in glibc-2.1.3 back in 1999.
> > Can that actually run LTP any more? If it can and this is considered
> > important, I fear the tst_clock_gettime() call needs to be extended
> > to call the clock_gettime()/clock_gettime64()/gettimeofday() syscalls,
> > whichever is the first to work, and convert the formats from the
> > native kernel format to the glibc format.
> 
> I guess that at the current time we do support distros that are at max
> 10 years old, mostly because enterprise support cycles are about 10
> years in lenght.
> 
> The issue here is that glibc needed -lrt passed to linker couple of
> years ago and we wanted to avoid the need of linking everything with
> -lrt, as calling the raw syscall was just easier fix.

To conclude the discussion, is this patch okay or not ? The reason why I am
asking this is because this file still uses the old test framework and so can't
include tst_timer.h, and so doesn't have access to struct __kernel_old_timespec.

What do you suggest I do here ?

-- 
viresh

[LTP] [PATCH 3/5] syscalls: Don't use tst_syscall() unnecessarily

[Cyril Hrubis]

Hi!
> > I guess that at the current time we do support distros that are at max
> > 10 years old, mostly because enterprise support cycles are about 10
> > years in lenght.
> > 
> > The issue here is that glibc needed -lrt passed to linker couple of
> > years ago and we wanted to avoid the need of linking everything with
> > -lrt, as calling the raw syscall was just easier fix.
> 
> To conclude the discussion, is this patch okay or not ? The reason why I am
> asking this is because this file still uses the old test framework and so can't
> include tst_timer.h, and so doesn't have access to struct __kernel_old_timespec.
> 
> What do you suggest I do here ?

Well if it was only about __kernel_old_timespec we could easily just
declare it locally in the file and be done with it.

But I guess that newer 32bit architectures will have only the 64 bit
syscall present, I think that somebody pointed out that this is the case
for 32bit RiscV. If that's true we will have to have a fallback for that
case as well.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 3/5] syscalls: Don't use tst_syscall() unnecessarily

[Arnd Bergmann]

On Thu, May 21, 2020 at 4:20 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> > > I guess that at the current time we do support distros that are at max
> > > 10 years old, mostly because enterprise support cycles are about 10
> > > years in lenght.
> > >
> > > The issue here is that glibc needed -lrt passed to linker couple of
> > > years ago and we wanted to avoid the need of linking everything with
> > > -lrt, as calling the raw syscall was just easier fix.
> >
> > To conclude the discussion, is this patch okay or not ? The reason why I am
> > asking this is because this file still uses the old test framework and so can't
> > include tst_timer.h, and so doesn't have access to struct __kernel_old_timespec.
> >
> > What do you suggest I do here ?
>
> Well if it was only about __kernel_old_timespec we could easily just
> declare it locally in the file and be done with it.
>
> But I guess that newer 32bit architectures will have only the 64 bit
> syscall present, I think that somebody pointed out that this is the case
> for 32bit RiscV. If that's true we will have to have a fallback for that
> case as well.

Yes, that is correct. Also, the 32-bit time_t syscalls can be disabled
on newer kernels when you have a libc implementation that
uses the 64-bit calls exclusively.

I guess the fallback to to libc gettimeofday() isn't really needed
unless it needs to run on ancient kernels.

      Arnd

[LTP] [PATCH 4/5] syscalls: Don't pass struct timespec to tst_syscall()

[Viresh Kumar]

There are compatibility issues here as we are calling the direct
syscalls with the "struct timespec" (which is a libc definition). We
must use struct __kernel_old_timespec instead.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/tst_clocks.h          |  8 +++++---
 lib/tst_clocks.c              |  6 +++---
 lib/tst_test.c                | 16 ++++++++--------
 lib/tst_timer.c               | 23 +++++++++++++++--------
 lib/tst_timer_test.c          |  2 +-
 lib/tst_wallclock.c           | 17 +++++++++--------
 testcases/cve/cve-2016-7117.c |  2 +-
 7 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/include/tst_clocks.h b/include/tst_clocks.h
index 80030c6b0c68..bb149acd2f83 100644
--- a/include/tst_clocks.h
+++ b/include/tst_clocks.h
@@ -9,11 +9,13 @@
 #ifndef TST_CLOCKS__
 #define TST_CLOCKS__
 
-int tst_clock_getres(clockid_t clk_id, struct timespec *res);
+struct __kernel_old_timespec;
 
-int tst_clock_gettime(clockid_t clk_id, struct timespec *ts);
+int tst_clock_getres(clockid_t clk_id, struct __kernel_old_timespec *res);
 
-int tst_clock_settime(clockid_t clk_id, struct timespec *ts);
+int tst_clock_gettime(clockid_t clk_id, struct __kernel_old_timespec *ts);
+
+int tst_clock_settime(clockid_t clk_id, struct __kernel_old_timespec *ts);
 
 /*
  * Converts clock id to a readable name.
diff --git a/lib/tst_clocks.c b/lib/tst_clocks.c
index 2eaa73b11abe..8d8c2f7666fa 100644
--- a/lib/tst_clocks.c
+++ b/lib/tst_clocks.c
@@ -11,17 +11,17 @@
 #include "lapi/syscalls.h"
 #include "lapi/posix_clocks.h"
 
-int tst_clock_getres(clockid_t clk_id, struct timespec *res)
+int tst_clock_getres(clockid_t clk_id, struct __kernel_old_timespec *res)
 {
 	return tst_syscall(__NR_clock_getres, clk_id, res);
 }
 
-int tst_clock_gettime(clockid_t clk_id, struct timespec *ts)
+int tst_clock_gettime(clockid_t clk_id, struct __kernel_old_timespec *ts)
 {
 	return tst_syscall(__NR_clock_gettime, clk_id, ts);
 }
 
-int tst_clock_settime(clockid_t clk_id, struct timespec *ts)
+int tst_clock_settime(clockid_t clk_id, struct __kernel_old_timespec *ts)
 {
 	return tst_syscall(__NR_clock_settime, clk_id, ts);
 }
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 0e58060e0159..8f6b07072429 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -49,7 +49,7 @@ static float timeout_mul = -1;
 static pid_t main_pid, lib_pid;
 static int mntpoint_mounted;
 static int ovl_mounted;
-static struct timespec tst_start_time; /* valid only for test pid */
+static struct tst_ts tst_start_time = { .type = TST_KERN_OLD_TIMESPEC }; /* valid only for test pid */
 
 struct results {
 	int passed;
@@ -1081,12 +1081,12 @@ static void run_tests(void)
 
 static unsigned long long get_time_ms(void)
 {
-	struct timespec ts;
+	struct tst_ts ts = { .type = TST_KERN_OLD_TIMESPEC };
 
-	if (tst_clock_gettime(CLOCK_MONOTONIC, &ts))
+	if (tst_clock_gettime(CLOCK_MONOTONIC, &ts.ts.kern_old_ts))
 		tst_brk(TBROK | TERRNO, "tst_clock_gettime()");
 
-	return tst_timespec_to_ms(ts);
+	return tst_ts_to_ms(ts);
 }
 
 static void add_paths(void)
@@ -1108,7 +1108,7 @@ static void add_paths(void)
 
 static void heartbeat(void)
 {
-	if (tst_clock_gettime(CLOCK_MONOTONIC, &tst_start_time))
+	if (tst_clock_gettime(CLOCK_MONOTONIC, &tst_start_time.ts.kern_old_ts))
 		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 
 	kill(getppid(), SIGUSR1);
@@ -1190,13 +1190,13 @@ static void sigint_handler(int sig LTP_ATTRIBUTE_UNUSED)
 
 unsigned int tst_timeout_remaining(void)
 {
-	static struct timespec now;
+	struct tst_ts now = { .type = TST_KERN_OLD_TIMESPEC };
 	unsigned int elapsed;
 
-	if (tst_clock_gettime(CLOCK_MONOTONIC, &now))
+	if (tst_clock_gettime(CLOCK_MONOTONIC, &now.ts.kern_old_ts))
 		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 
-	elapsed = (tst_timespec_diff_ms(now, tst_start_time) + 500) / 1000;
+	elapsed = (tst_ts_diff_ms(now, tst_start_time) + 500) / 1000;
 	if (results->timeout > elapsed)
 		return results->timeout - elapsed;
 
diff --git a/lib/tst_timer.c b/lib/tst_timer.c
index 62d8f9080f38..97817770ec7e 100644
--- a/lib/tst_timer.c
+++ b/lib/tst_timer.c
@@ -12,12 +12,13 @@
 #include "tst_clocks.h"
 #include "lapi/posix_clocks.h"
 
-static struct timespec start_time, stop_time;
+static struct tst_ts start_time = { .type = TST_KERN_OLD_TIMESPEC };
+static struct tst_ts stop_time = { .type = TST_KERN_OLD_TIMESPEC };
 static clockid_t clock_id;
 
 void tst_timer_check(clockid_t clk_id)
 {
-	if (tst_clock_gettime(clk_id, &start_time)) {
+	if (tst_clock_gettime(clk_id, &start_time.ts.kern_old_ts)) {
 		if (errno == EINVAL) {
 			tst_brk(TCONF,
 			         "Clock id %s(%u) not supported by kernel",
@@ -33,27 +34,33 @@ void tst_timer_start(clockid_t clk_id)
 {
 	clock_id = clk_id;
 
-	if (tst_clock_gettime(clock_id, &start_time))
+	if (tst_clock_gettime(clock_id, &start_time.ts.kern_old_ts))
 		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 }
 
 int tst_timer_expired_ms(long long ms)
 {
-	struct timespec cur_time;
+	struct tst_ts cur_time = { .type = TST_KERN_OLD_TIMESPEC };
 
-	if (tst_clock_gettime(clock_id, &cur_time))
+	if (tst_clock_gettime(clock_id, &cur_time.ts.kern_old_ts))
 		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 
-	return tst_timespec_diff_ms(cur_time, start_time) >= ms;
+	return tst_ts_diff_ms(cur_time, start_time) >= ms;
 }
 
 void tst_timer_stop(void)
 {
-	if (tst_clock_gettime(clock_id, &stop_time))
+	if (tst_clock_gettime(clock_id, &stop_time.ts.kern_old_ts))
 		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 }
 
 struct timespec tst_timer_elapsed(void)
 {
-	return tst_timespec_diff(stop_time, start_time);
+	struct tst_ts ts;
+	struct timespec tspec;
+
+	ts = tst_ts_diff(stop_time, start_time);
+	tspec.tv_sec = ts.ts.kern_old_ts.tv_sec;
+	tspec.tv_nsec = ts.ts.kern_old_ts.tv_nsec;
+	return tspec;
 }
diff --git a/lib/tst_timer_test.c b/lib/tst_timer_test.c
index 196c51272d34..6aa5711643fe 100644
--- a/lib/tst_timer_test.c
+++ b/lib/tst_timer_test.c
@@ -341,7 +341,7 @@ static int set_latency(void)
 
 static void timer_setup(void)
 {
-	struct timespec t;
+	struct __kernel_old_timespec t;
 	int ret;
 
 	if (setup)
diff --git a/lib/tst_wallclock.c b/lib/tst_wallclock.c
index 282d6ada3008..a267792756ee 100644
--- a/lib/tst_wallclock.c
+++ b/lib/tst_wallclock.c
@@ -14,7 +14,8 @@
 #include "tst_wallclock.h"
 #include "lapi/posix_clocks.h"
 
-static struct timespec real_begin, mono_begin;
+static struct tst_ts real_begin = { .type = TST_KERN_OLD_TIMESPEC };
+static struct tst_ts mono_begin = { .type = TST_KERN_OLD_TIMESPEC };
 
 static int clock_saved;
 
@@ -22,10 +23,10 @@ void tst_wallclock_save(void)
 {
 	/* save initial monotonic time to restore it when needed */
 
-	if (tst_clock_gettime(CLOCK_REALTIME, &real_begin))
+	if (tst_clock_gettime(CLOCK_REALTIME, &real_begin.ts.kern_old_ts))
 		tst_brk(TBROK | TERRNO, "tst_clock_gettime() realtime failed");
 
-	if (tst_clock_gettime(CLOCK_MONOTONIC_RAW, &mono_begin)) {
+	if (tst_clock_gettime(CLOCK_MONOTONIC_RAW, &mono_begin.ts.kern_old_ts)) {
 		if (errno == EINVAL) {
 			tst_brk(TCONF | TERRNO,
 				"tst_clock_gettime() didn't support CLOCK_MONOTONIC_RAW");
@@ -39,22 +40,22 @@ void tst_wallclock_save(void)
 
 void tst_wallclock_restore(void)
 {
-	static struct timespec mono_end, elapsed, adjust;
+	struct tst_ts elapsed, adjust, mono_end = { .type = TST_KERN_OLD_TIMESPEC };
 
 	if (!clock_saved)
 		return;
 
 	clock_saved = 0;
 
-	if (tst_clock_gettime(CLOCK_MONOTONIC_RAW, &mono_end))
+	if (tst_clock_gettime(CLOCK_MONOTONIC_RAW, &mono_end.ts.kern_old_ts))
 		tst_brk(TBROK | TERRNO, "tst_clock_gettime() monotonic failed");
 
-	elapsed = tst_timespec_diff(mono_end, mono_begin);
+	elapsed = tst_ts_diff(mono_end, mono_begin);
 
-	adjust = tst_timespec_add(real_begin, elapsed);
+	adjust = tst_ts_add(real_begin, elapsed);
 
 	/* restore realtime clock based on monotonic delta */
 
-	if (tst_clock_settime(CLOCK_REALTIME, &adjust))
+	if (tst_clock_settime(CLOCK_REALTIME, &adjust.ts.kern_old_ts))
 		tst_brk(TBROK | TERRNO, "tst_clock_settime() realtime failed");
 }
diff --git a/testcases/cve/cve-2016-7117.c b/testcases/cve/cve-2016-7117.c
index dca002924728..cbbc1c182079 100644
--- a/testcases/cve/cve-2016-7117.c
+++ b/testcases/cve/cve-2016-7117.c
@@ -73,7 +73,7 @@ static struct mmsghdr msghdrs[2] = {
 	}
 };
 static char rbuf[sizeof(MSG)];
-static struct timespec timeout = { .tv_sec = RECV_TIMEOUT };
+static struct __kernel_old_timespec timeout = { .tv_sec = RECV_TIMEOUT };
 static struct tst_fzsync_pair fzsync_pair;
 static void *send_and_close(void *);
 
-- 
2.25.0.rc1.19.g042ed3e048af

[LTP] [PATCH 4/5] syscalls: Don't pass struct timespec to tst_syscall()

[Cyril Hrubis]

Hi!
> There are compatibility issues here as we are calling the direct
> syscalls with the "struct timespec" (which is a libc definition). We
> must use struct __kernel_old_timespec instead.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  include/tst_clocks.h          |  8 +++++---
>  lib/tst_clocks.c              |  6 +++---
>  lib/tst_test.c                | 16 ++++++++--------
>  lib/tst_timer.c               | 23 +++++++++++++++--------
>  lib/tst_timer_test.c          |  2 +-
>  lib/tst_wallclock.c           | 17 +++++++++--------
>  testcases/cve/cve-2016-7117.c |  2 +-
>  7 files changed, 42 insertions(+), 32 deletions(-)
> 
> diff --git a/include/tst_clocks.h b/include/tst_clocks.h
> index 80030c6b0c68..bb149acd2f83 100644
> --- a/include/tst_clocks.h
> +++ b/include/tst_clocks.h
> @@ -9,11 +9,13 @@
>  #ifndef TST_CLOCKS__
>  #define TST_CLOCKS__
>  
> -int tst_clock_getres(clockid_t clk_id, struct timespec *res);
> +struct __kernel_old_timespec;
>  
> -int tst_clock_gettime(clockid_t clk_id, struct timespec *ts);
> +int tst_clock_getres(clockid_t clk_id, struct __kernel_old_timespec *res);
>  
> -int tst_clock_settime(clockid_t clk_id, struct timespec *ts);
> +int tst_clock_gettime(clockid_t clk_id, struct __kernel_old_timespec *ts);
> +
> +int tst_clock_settime(clockid_t clk_id, struct __kernel_old_timespec *ts);
>  
>  /*
>   * Converts clock id to a readable name.
> diff --git a/lib/tst_clocks.c b/lib/tst_clocks.c
> index 2eaa73b11abe..8d8c2f7666fa 100644
> --- a/lib/tst_clocks.c
> +++ b/lib/tst_clocks.c
> @@ -11,17 +11,17 @@
>  #include "lapi/syscalls.h"
>  #include "lapi/posix_clocks.h"
>  
> -int tst_clock_getres(clockid_t clk_id, struct timespec *res)
> +int tst_clock_getres(clockid_t clk_id, struct __kernel_old_timespec *res)
>  {
>  	return tst_syscall(__NR_clock_getres, clk_id, res);
>  }
>  
> -int tst_clock_gettime(clockid_t clk_id, struct timespec *ts)
> +int tst_clock_gettime(clockid_t clk_id, struct __kernel_old_timespec *ts)
>  {
>  	return tst_syscall(__NR_clock_gettime, clk_id, ts);
>  }
>  
> -int tst_clock_settime(clockid_t clk_id, struct timespec *ts)
> +int tst_clock_settime(clockid_t clk_id, struct __kernel_old_timespec *ts)
>  {
>  	return tst_syscall(__NR_clock_settime, clk_id, ts);
>  }

These functions are supposed to be library-internal and were added so
that we do not have to link everything in LTP with -lrt, which is needed
for the clock_gettime() functions on older glibc.

So we can as well so that they take the tst_ts structure, then we are
also free to change the way the timestamp is acquired without the need
to change all the callers.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 4/5] syscalls: Don't pass struct timespec to tst_syscall()

[Viresh Kumar]

On 19-05-20, 14:21, Cyril Hrubis wrote:
> So we can as well so that they take the tst_ts structure, then we are
> also free to change the way the timestamp is acquired without the need
> to change all the callers.

I am not sure I understood it all. What do you mean by "also free to change the
way the timestamp is acquired"?

Also, even if these routines take a struct tst_ts, the callers will all need to
have the changes which are currently done, as they can't pass struct timespec
anymore. Over that, few of the callers (cve-2016-7117.c and tst_timer_test.c)
would be required to have more changes as right now they only needed to move
from timespec to __kernel_old_timespec and nothing else.

Another reason why I didn't pass tst_ts to them is because you suggested me
earlier, to not pass that to the tst_syscall() definitions I added earlier for
all the syscalls, where I was doing tst_get_ts() inside the tst_clock_gettime()
calls for example. And you asked me to let the caller do that and pass only the
actual timespec in a void *.

-- 
viresh

[LTP] [PATCH 4/5] syscalls: Don't pass struct timespec to tst_syscall()

[Arnd Bergmann]

On Wed, May 20, 2020 at 9:31 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 19-05-20, 14:21, Cyril Hrubis wrote:
> > So we can as well so that they take the tst_ts structure, then we are
> > also free to change the way the timestamp is acquired without the need
> > to change all the callers.
>
> I am not sure I understood it all. What do you mean by "also free to change the
> way the timestamp is acquired"?

The bug in the current implementation is that the tst_clock_gettime() takes
the libc type but the argument to the kernel that may expect a different
type.

Your patch solves the problem by using the kernel type consistently,
but the other way to solve it is to keep passing the glibc type and
instead make tst_clock_gettime() get a timestamp through the low
level kernel interface using the kernel type and then convert it, like

int tst_clock_gettime(clockid_t clk_id, struct timespec *ts)
 {
       int ret;

#ifdef(__NR_clock_gettime64)
        struct __kernel_timespec newts;
        ret = tst_syscall(__NR_clock_gettime64, clk_id, &newts);
        *ts = (struct timespec) { .tv_sec = newts.tv_sec, .tv_nsec =
newts.tv_nsec };
        if (ret != -ENOSYS)
              return ret;
#endif

#ifdef __NR_clock_gettime
        struct __kernel_old_timespec oldts;
        ret = tst_syscall(__NR_clock_gettime, clk_id, &oldts);
        *ts = (struct timespec) { .tv_sec = oldts.tv_sec, .tv_nsec =
oldts.tv_nsec };
        if (ret != -ENOSYS)
              return ret;
#endif

       /* fallback for prehistoric linux */
        struct timeval tv;
        ret = gettimeofday(&tv, NULL);
        *ts = (struct timespec) { .tv_sec = newts.tv_sec, .tv_usec =
newts.tv_nsec / 1000};

        return ret;
}

Or something like it that works reliably.

[LTP] [PATCH 4/5] syscalls: Don't pass struct timespec to tst_syscall()

[Viresh Kumar]

On 20-05-20, 10:47, Arnd Bergmann wrote:
> On Wed, May 20, 2020 at 9:31 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 19-05-20, 14:21, Cyril Hrubis wrote:
> > > So we can as well so that they take the tst_ts structure, then we are
> > > also free to change the way the timestamp is acquired without the need
> > > to change all the callers.
> >
> > I am not sure I understood it all. What do you mean by "also free to change the
> > way the timestamp is acquired"?
> 
> The bug in the current implementation is that the tst_clock_gettime() takes
> the libc type but the argument to the kernel that may expect a different
> type.
> 
> Your patch solves the problem by using the kernel type consistently,
> but the other way to solve it is to keep passing the glibc type and
> instead make tst_clock_gettime() get a timestamp through the low
> level kernel interface using the kernel type and then convert it, like

That can be one way of doing it, but Cyril wasn't suggesting this I believe. He
talked about passing struct tst_ts instead (which is a union of all timespec
types).

> int tst_clock_gettime(clockid_t clk_id, struct timespec *ts)
>  {
>        int ret;
> 
> #ifdef(__NR_clock_gettime64)
>         struct __kernel_timespec newts;
>         ret = tst_syscall(__NR_clock_gettime64, clk_id, &newts);
>         *ts = (struct timespec) { .tv_sec = newts.tv_sec, .tv_nsec =
> newts.tv_nsec };
>         if (ret != -ENOSYS)
>               return ret;
> #endif
> 
> #ifdef __NR_clock_gettime
>         struct __kernel_old_timespec oldts;
>         ret = tst_syscall(__NR_clock_gettime, clk_id, &oldts);
>         *ts = (struct timespec) { .tv_sec = oldts.tv_sec, .tv_nsec =
> oldts.tv_nsec };
>         if (ret != -ENOSYS)
>               return ret;
> #endif
> 
>        /* fallback for prehistoric linux */
>         struct timeval tv;
>         ret = gettimeofday(&tv, NULL);
>         *ts = (struct timespec) { .tv_sec = newts.tv_sec, .tv_usec =
> newts.tv_nsec / 1000};
> 
>         return ret;
> }

This is used only for the internal working of the library and so we may not need
to support all these timespec types TBH and make it complex.

-- 
viresh

[LTP] [PATCH 4/5] syscalls: Don't pass struct timespec to tst_syscall()

[Arnd Bergmann]

On Wed, May 20, 2020 at 11:05 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 20-05-20, 10:47, Arnd Bergmann wrote:
> > On Wed, May 20, 2020 at 9:31 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> > int tst_clock_gettime(clockid_t clk_id, struct timespec *ts)
> >  {
> >        int ret;
> >
> > #ifdef(__NR_clock_gettime64)
> >         struct __kernel_timespec newts;
> >         ret = tst_syscall(__NR_clock_gettime64, clk_id, &newts);
> >         *ts = (struct timespec) { .tv_sec = newts.tv_sec, .tv_nsec =
> > newts.tv_nsec };
> >         if (ret != -ENOSYS)
> >               return ret;
> > #endif
> >
> > #ifdef __NR_clock_gettime
> >         struct __kernel_old_timespec oldts;
> >         ret = tst_syscall(__NR_clock_gettime, clk_id, &oldts);
> >         *ts = (struct timespec) { .tv_sec = oldts.tv_sec, .tv_nsec =
> > oldts.tv_nsec };
> >         if (ret != -ENOSYS)
> >               return ret;
> > #endif
> >
> >        /* fallback for prehistoric linux */
> >         struct timeval tv;
> >         ret = gettimeofday(&tv, NULL);
> >         *ts = (struct timespec) { .tv_sec = newts.tv_sec, .tv_usec =
> > newts.tv_nsec / 1000};
> >
> >         return ret;
> > }
>
> This is used only for the internal working of the library and so we may not need
> to support all these timespec types TBH and make it complex.

Well, the point here is that you need the function to reliably return the
current time in the right format, and I don't think a simpler implementation
would be correct. In fact, I just realized that your patch version cannot work
on riscv32 since it only has __NR_clock_gettime64 but not __NR_clock_gettime.

         Arnd

[LTP] [PATCH 4/5] syscalls: Don't pass struct timespec to tst_syscall()

[Viresh Kumar]

On 20-05-20, 11:35, Arnd Bergmann wrote:
> On Wed, May 20, 2020 at 11:05 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 20-05-20, 10:47, Arnd Bergmann wrote:
> > > On Wed, May 20, 2020 at 9:31 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > > int tst_clock_gettime(clockid_t clk_id, struct timespec *ts)
> > >  {
> > >        int ret;
> > >
> > > #ifdef(__NR_clock_gettime64)
> > >         struct __kernel_timespec newts;
> > >         ret = tst_syscall(__NR_clock_gettime64, clk_id, &newts);
> > >         *ts = (struct timespec) { .tv_sec = newts.tv_sec, .tv_nsec =
> > > newts.tv_nsec };
> > >         if (ret != -ENOSYS)
> > >               return ret;
> > > #endif
> > >
> > > #ifdef __NR_clock_gettime
> > >         struct __kernel_old_timespec oldts;
> > >         ret = tst_syscall(__NR_clock_gettime, clk_id, &oldts);
> > >         *ts = (struct timespec) { .tv_sec = oldts.tv_sec, .tv_nsec =
> > > oldts.tv_nsec };
> > >         if (ret != -ENOSYS)
> > >               return ret;
> > > #endif
> > >
> > >        /* fallback for prehistoric linux */
> > >         struct timeval tv;
> > >         ret = gettimeofday(&tv, NULL);
> > >         *ts = (struct timespec) { .tv_sec = newts.tv_sec, .tv_usec =
> > > newts.tv_nsec / 1000};
> > >
> > >         return ret;
> > > }
> >
> > This is used only for the internal working of the library and so we may not need
> > to support all these timespec types TBH and make it complex.
> 
> Well, the point here is that you need the function to reliably return the
> current time in the right format, and I don't think a simpler implementation
> would be correct. In fact, I just realized that your patch version cannot work
> on riscv32 since it only has __NR_clock_gettime64 but not __NR_clock_gettime.

I am wondering that there would be lots of such issues across LTP, all existing
ones btw, and should we try to fix them or not ? No one ever ran it on Risk I
believe, otherwise they would have seen it.

-- 
viresh

[LTP] [PATCH 4/5] syscalls: Don't pass struct timespec to tst_syscall()

[Arnd Bergmann]

On Wed, May 20, 2020 at 11:39 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 20-05-20, 11:35, Arnd Bergmann wrote:
> > On Wed, May 20, 2020 at 11:05 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > On 20-05-20, 10:47, Arnd Bergmann wrote:
> > > > On Wed, May 20, 2020 at 9:31 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > > int tst_clock_gettime(clockid_t clk_id, struct timespec *ts)
> > > >  {
> > > >        int ret;
> > > >
> > > > #ifdef(__NR_clock_gettime64)
> > > >         struct __kernel_timespec newts;
> > > >         ret = tst_syscall(__NR_clock_gettime64, clk_id, &newts);
> > > >         *ts = (struct timespec) { .tv_sec = newts.tv_sec, .tv_nsec =
> > > > newts.tv_nsec };
> > > >         if (ret != -ENOSYS)
> > > >               return ret;
> > > > #endif
> > > >
> > > > #ifdef __NR_clock_gettime
> > > >         struct __kernel_old_timespec oldts;
> > > >         ret = tst_syscall(__NR_clock_gettime, clk_id, &oldts);
> > > >         *ts = (struct timespec) { .tv_sec = oldts.tv_sec, .tv_nsec =
> > > > oldts.tv_nsec };
> > > >         if (ret != -ENOSYS)
> > > >               return ret;
> > > > #endif
> > > >
> > > >        /* fallback for prehistoric linux */
> > > >         struct timeval tv;
> > > >         ret = gettimeofday(&tv, NULL);
> > > >         *ts = (struct timespec) { .tv_sec = newts.tv_sec, .tv_usec =
> > > > newts.tv_nsec / 1000};
> > > >
> > > >         return ret;
> > > > }
> > >
> > > This is used only for the internal working of the library and so we may not need
> > > to support all these timespec types TBH and make it complex.
> >
> > Well, the point here is that you need the function to reliably return the
> > current time in the right format, and I don't think a simpler implementation
> > would be correct. In fact, I just realized that your patch version cannot work
> > on riscv32 since it only has __NR_clock_gettime64 but not __NR_clock_gettime.
>
> I am wondering that there would be lots of such issues across LTP, all existing
> ones btw, and should we try to fix them or not ? No one ever ran it on Risk I
> believe, otherwise they would have seen it.

I tried running LTP last year on an arm kernel that I configured to behave the
same way as rv32 (disabling CONFIG_COMPAT_32BIT_TIME) and I did
run into these problems, but I don't think they were too hard to work around.

           Arnd

[LTP] [PATCH 5/5] syscalls: Don't pass struct timeval to tst_syscall()

[Viresh Kumar]

There are compatibility issues here as we are calling the direct
syscalls with the "struct timeval" (which is a libc definition). We
must use struct __kernel_old_timeval instead.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/tst_timer.h                                     | 6 ++++++
 testcases/kernel/syscalls/clock_adjtime/clock_adjtime.h | 5 -----
 testcases/kernel/syscalls/gettimeofday/gettimeofday02.c | 3 ++-
 testcases/kernel/syscalls/stime/stime_var.h             | 3 ++-
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/tst_timer.h b/include/tst_timer.h
index 071061f5b280..814b70797643 100644
--- a/include/tst_timer.h
+++ b/include/tst_timer.h
@@ -12,6 +12,7 @@
 #ifndef TST_TIMER
 #define TST_TIMER
 
+#include <asm/posix_types.h>
 #include <sched.h>
 #include <sys/time.h>
 #include <mqueue.h>
@@ -104,6 +105,11 @@ typedef long __kernel_long_t;
 
 typedef __kernel_long_t	__kernel_old_time_t;
 
+struct __kernel_old_timeval {
+	__kernel_old_time_t	tv_sec;		/* seconds */
+	__kernel_suseconds_t	tv_usec;	/* microseconds */
+};
+
 struct __kernel_old_timespec {
 	__kernel_old_time_t	tv_sec;		/* seconds */
 	__kernel_old_time_t	tv_nsec;	/* nanoseconds */
diff --git a/testcases/kernel/syscalls/clock_adjtime/clock_adjtime.h b/testcases/kernel/syscalls/clock_adjtime/clock_adjtime.h
index eb60f707f776..dbe0a561a3ab 100644
--- a/testcases/kernel/syscalls/clock_adjtime/clock_adjtime.h
+++ b/testcases/kernel/syscalls/clock_adjtime/clock_adjtime.h
@@ -18,11 +18,6 @@
 #include "lapi/timex.h"
 
 #ifndef __kernel_timex
-struct __kernel_old_timeval {
-	__kernel_old_time_t	tv_sec;		/* seconds */
-	__kernel_suseconds_t	tv_usec;	/* microseconds */
-};
-
 struct __kernel_old_timex {
 	unsigned int modes;	/* mode selector */
 	__kernel_long_t offset;	/* time offset (usec) */
diff --git a/testcases/kernel/syscalls/gettimeofday/gettimeofday02.c b/testcases/kernel/syscalls/gettimeofday/gettimeofday02.c
index b7687468d39d..b73bf129b116 100644
--- a/testcases/kernel/syscalls/gettimeofday/gettimeofday02.c
+++ b/testcases/kernel/syscalls/gettimeofday/gettimeofday02.c
@@ -21,6 +21,7 @@
 #include <errno.h>
 
 #include "tst_test.h"
+#include "tst_timer.h"
 #include "lapi/syscalls.h"
 
 static volatile sig_atomic_t done;
@@ -39,7 +40,7 @@ static void breakout(int sig)
 
 static void verify_gettimeofday(void)
 {
-	struct timeval tv1, tv2;
+	struct __kernel_old_timeval tv1, tv2;
 	unsigned long long cnt = 0;
 
 	done = 0;
diff --git a/testcases/kernel/syscalls/stime/stime_var.h b/testcases/kernel/syscalls/stime/stime_var.h
index b33c5704e94a..708b80573167 100644
--- a/testcases/kernel/syscalls/stime/stime_var.h
+++ b/testcases/kernel/syscalls/stime/stime_var.h
@@ -9,6 +9,7 @@
 
 #include <sys/time.h>
 #include "config.h"
+#include "tst_timer.h"
 #include "lapi/syscalls.h"
 
 #define TEST_VARIANTS 3
@@ -26,7 +27,7 @@ static int do_stime(time_t *ntime)
 	case 1:
 		return tst_syscall(__NR_stime, ntime);
 	case 2: {
-		struct timeval tv;
+		struct __kernel_old_timeval tv;
 
 		tv.tv_sec = *ntime;
 		tv.tv_usec = 0;
-- 
2.25.0.rc1.19.g042ed3e048af