[LTP] [PATCH V2] syscalls: clock_settime: Add test around y2038 vulnerability

[Arnd Bergmann]

On Thu, May 28, 2020 at 11:11 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 28-05-20, 10:27, Arnd Bergmann wrote:
> > On Thu, May 28, 2020 at 8:57 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > > +       ret = tv->clock_settime(CLOCK_REALTIME, tst_ts_get(&ts));
> > > +       if (ret == -1)
> > > +               tst_brk(TBROK | TERRNO, "clock_settime() failed");
>
> But I noticed that even this version may not be good enough, as I am
> still doing the same thing in setup(), i.e. setting time to just
> before y2038 to test if it is y2038 safe or not. I believe even that
> isn't fine ?

Good point. I see you have this check above that:

+       /* Check if the kernel is y2038 safe */
+       if (tv->type != TST_KERN_OLD_TIMESPEC)
+               return;
+

So that part should prevent it from doing something bad, but I now
see it's still not great because it also prevents the test case from running
on 64-bit architectures. If you change the condition to also check for
sizeof(__kernel_old_timespec) it should be ok.

> diff --git a/testcases/kernel/syscalls/clock_settime/clock_settime03.c b/testcases/kernel/syscalls/clock_settime/clock_settime03.c
> index 9e316378b1cc..876651a5d537 100644
> --- a/testcases/kernel/syscalls/clock_settime/clock_settime03.c
> +++ b/testcases/kernel/syscalls/clock_settime/clock_settime03.c
> @@ -72,6 +71,7 @@ static void run(void)
>                 .sigev_notify = SIGEV_SIGNAL,
>                 .sigev_signo = SIGABRT,
>         };
> +       struct tst_ts diff;
>         timer_t timer;
>         sigset_t set;
>         int sig, ret;
> @@ -105,7 +105,16 @@ static void run(void)
>         if (sigwait(&set, &sig) == -1)
>                 tst_brk(TBROK, "sigwait() failed");
>
> +       ret = tv->clock_gettime(CLOCK_REALTIME, tst_ts_get(&end));
> +       if (ret == -1)
> +               tst_brk(TBROK | TERRNO, "clock_gettime() failed");
> +
>         if (sig == SIGABRT) {
> +               diff = tst_ts_diff(end, ts);
> +
> +               if (tst_ts_get_sec(diff) != EXPIREDELTA)
> +                       tst_res(TINFO, "Test slept longer than it should have, expected:%d, actual:%lld",
> +                               EXPIREDELTA, tst_ts_get_sec(diff));
>                 tst_res(TPASS, "clock_settime(): Y2038 test passed");
>                 return;

Yes, I think that should print a warning when something goes wrong,
but it can be improved:

- don't say it slept longer when it could also have slept shorter, or not
  slept at all. It's probably enough to say that the expired time is not what
  was expected and leave the interpretation to a person

- comparing only the seconds means that you warn when the elapsed
  time was less than expected or more than 1000000000 nanoseconds
  longer than expected, but that is a fairly long and arbitrary interval.
  Maybe make it something like 50ms and use a constant for it, so it
  can be adjusted if necessary.

        Arnd