diff mbox series

time: Skip overflow itimer tests on 32-bit systems

Message ID 20210607131851.4015661-1-shorne@gmail.com
State Superseded
Delegated to: Adhemerval Zanella Netto
Headers show
Series time: Skip overflow itimer tests on 32-bit systems | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Stafford Horne June 7, 2021, 1:18 p.m. UTC
On the port of OpenRISC I am working on and it appears the rv32 port
we have sets __TIMESIZE == 64 && __WORDSIZE == 32.  This causes the
size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit
causing truncation.

The truncations are unavoidable on these systems so skip the
testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64.
---

I am open to other suggestions, this seemed the most correct to me.

Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: Alistair Francis <alistair.francis@wdc.com>

 time/tst-itimer.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Adhemerval Zanella June 9, 2021, 1:50 p.m. UTC | #1
On 07/06/2021 10:18, Stafford Horne wrote:
> On the port of OpenRISC I am working on and it appears the rv32 port
> we have sets __TIMESIZE == 64 && __WORDSIZE == 32.  This causes the
> size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit
> causing truncation.
> 
> The truncations are unavoidable on these systems so skip the
> testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64.

Sigh, I was hoping that we won't need to handle this situation (glibc 
support only 64-bit time_t, but kernel still providing some 32-bit 
syscall).

> ---
> 
> I am open to other suggestions, this seemed the most correct to me.
> 
> Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> Cc: Alistair Francis <alistair.francis@wdc.com>
> 
>  time/tst-itimer.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/time/tst-itimer.c b/time/tst-itimer.c
> index 929c2b74c7..0c99d46d7e 100644
> --- a/time/tst-itimer.c
> +++ b/time/tst-itimer.c
> @@ -89,6 +89,7 @@ do_test (void)
>        TEST_COMPARE (it.it_interval.tv_sec, it_old.it_interval.tv_sec);
>        TEST_COMPARE (it.it_interval.tv_usec, it_old.it_interval.tv_usec);
>  
> +#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64
>        if (sizeof (time_t) == 4)
>  	continue;
>  
> @@ -146,6 +147,7 @@ do_test (void)
>  	  TEST_COMPARE (setitimer (timers[i], &it, NULL), -1);
>  	  TEST_COMPARE (errno, EOVERFLOW);
>  	}
> +#endif
>    }
>  
>    {
> 

Instead of disabling, I think it would be better to use
__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 instead of __time_t sizeof
(so we can still tests the EOVERFLOW):

diff --git a/time/tst-itimer.c b/time/tst-itimer.c
index 929c2b74c7..bd7d7afe83 100644
--- a/time/tst-itimer.c
+++ b/time/tst-itimer.c
@@ -100,7 +100,7 @@ do_test (void)

       /* Linux does not provide 64 bit time_t support for getitimer and
         setitimer on architectures with 32 bit time_t support.  */
-      if (sizeof (__time_t) == 8)
+      if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
        { 
          TEST_COMPARE (setitimer (timers[i], &it, NULL), 0);
          TEST_COMPARE (setitimer (timers[i], &(struct itimerval) { 0 },
@@ -131,7 +131,7 @@ do_test (void)
       it.it_interval.tv_usec = 20;
       it.it_value.tv_sec = 30;
       it.it_value.tv_usec = 40;
-      if (sizeof (__time_t) == 8)
+      if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
        { 
          TEST_COMPARE (setitimer (timers[i], &it, NULL), 0);
Stafford Horne June 9, 2021, 9:38 p.m. UTC | #2
On Wed, Jun 09, 2021 at 10:50:23AM -0300, Adhemerval Zanella wrote:
> 
> 
> On 07/06/2021 10:18, Stafford Horne wrote:
> > On the port of OpenRISC I am working on and it appears the rv32 port
> > we have sets __TIMESIZE == 64 && __WORDSIZE == 32.  This causes the
> > size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit
> > causing truncation.
> > 
> > The truncations are unavoidable on these systems so skip the
> > testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64.
> 
> Sigh, I was hoping that we won't need to handle this situation (glibc 
> support only 64-bit time_t, but kernel still providing some 32-bit 
> syscall).
> 
> > ---
> > 
> > I am open to other suggestions, this seemed the most correct to me.
> > 
> > Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> > Cc: Alistair Francis <alistair.francis@wdc.com>
> > 
> >  time/tst-itimer.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/time/tst-itimer.c b/time/tst-itimer.c
> > index 929c2b74c7..0c99d46d7e 100644
> > --- a/time/tst-itimer.c
> > +++ b/time/tst-itimer.c
> > @@ -89,6 +89,7 @@ do_test (void)
> >        TEST_COMPARE (it.it_interval.tv_sec, it_old.it_interval.tv_sec);
> >        TEST_COMPARE (it.it_interval.tv_usec, it_old.it_interval.tv_usec);
> >  
> > +#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64
> >        if (sizeof (time_t) == 4)
> >  	continue;
> >  
> > @@ -146,6 +147,7 @@ do_test (void)
> >  	  TEST_COMPARE (setitimer (timers[i], &it, NULL), -1);
> >  	  TEST_COMPARE (errno, EOVERFLOW);
> >  	}
> > +#endif
> >    }
> >  
> >    {
> > 
> 
> Instead of disabling, I think it would be better to use
> __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 instead of __time_t sizeof
> (so we can still tests the EOVERFLOW):
> 
> diff --git a/time/tst-itimer.c b/time/tst-itimer.c
> index 929c2b74c7..bd7d7afe83 100644
> --- a/time/tst-itimer.c
> +++ b/time/tst-itimer.c
> @@ -100,7 +100,7 @@ do_test (void)
> 
>        /* Linux does not provide 64 bit time_t support for getitimer and
>          setitimer on architectures with 32 bit time_t support.  */
> -      if (sizeof (__time_t) == 8)
> +      if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
>         { 
>           TEST_COMPARE (setitimer (timers[i], &it, NULL), 0);
>           TEST_COMPARE (setitimer (timers[i], &(struct itimerval) { 0 },
> @@ -131,7 +131,7 @@ do_test (void)
>        it.it_interval.tv_usec = 20;
>        it.it_value.tv_sec = 30;
>        it.it_value.tv_usec = 40;
> -      if (sizeof (__time_t) == 8)
> +      if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
>         { 
>           TEST_COMPARE (setitimer (timers[i], &it, NULL), 0);

This looks good to me, I can update to this, test and resend the patch when I
get some time.  Probably later tonight.

-Strafford
Stafford Horne June 12, 2021, 9:19 a.m. UTC | #3
On Thu, Jun 10, 2021 at 06:38:04AM +0900, Stafford Horne wrote:
> On Wed, Jun 09, 2021 at 10:50:23AM -0300, Adhemerval Zanella wrote:
> > 
> > 
> > On 07/06/2021 10:18, Stafford Horne wrote:
> > > On the port of OpenRISC I am working on and it appears the rv32 port
> > > we have sets __TIMESIZE == 64 && __WORDSIZE == 32.  This causes the
> > > size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit
> > > causing truncation.
> > > 
> > > The truncations are unavoidable on these systems so skip the
> > > testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64.
> > 
> > Sigh, I was hoping that we won't need to handle this situation (glibc 
> > support only 64-bit time_t, but kernel still providing some 32-bit 
> > syscall).
> > 
> > > ---
> > > 
> > > I am open to other suggestions, this seemed the most correct to me.
> > > 
> > > Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> > > Cc: Alistair Francis <alistair.francis@wdc.com>
> > > 
> > >  time/tst-itimer.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/time/tst-itimer.c b/time/tst-itimer.c
> > > index 929c2b74c7..0c99d46d7e 100644
> > > --- a/time/tst-itimer.c
> > > +++ b/time/tst-itimer.c
> > > @@ -89,6 +89,7 @@ do_test (void)
> > >        TEST_COMPARE (it.it_interval.tv_sec, it_old.it_interval.tv_sec);
> > >        TEST_COMPARE (it.it_interval.tv_usec, it_old.it_interval.tv_usec);
> > >  
> > > +#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64
> > >        if (sizeof (time_t) == 4)
> > >  	continue;
> > >  
> > > @@ -146,6 +147,7 @@ do_test (void)
> > >  	  TEST_COMPARE (setitimer (timers[i], &it, NULL), -1);
> > >  	  TEST_COMPARE (errno, EOVERFLOW);
> > >  	}
> > > +#endif
> > >    }
> > >  
> > >    {
> > > 
> > 
> > Instead of disabling, I think it would be better to use
> > __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 instead of __time_t sizeof
> > (so we can still tests the EOVERFLOW):
> > 
> > diff --git a/time/tst-itimer.c b/time/tst-itimer.c
> > index 929c2b74c7..bd7d7afe83 100644
> > --- a/time/tst-itimer.c
> > +++ b/time/tst-itimer.c
> > @@ -100,7 +100,7 @@ do_test (void)
> > 
> >        /* Linux does not provide 64 bit time_t support for getitimer and
> >          setitimer on architectures with 32 bit time_t support.  */
> > -      if (sizeof (__time_t) == 8)
> > +      if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
> >         { 
> >           TEST_COMPARE (setitimer (timers[i], &it, NULL), 0);
> >           TEST_COMPARE (setitimer (timers[i], &(struct itimerval) { 0 },
> > @@ -131,7 +131,7 @@ do_test (void)
> >        it.it_interval.tv_usec = 20;
> >        it.it_value.tv_sec = 30;
> >        it.it_value.tv_usec = 40;
> > -      if (sizeof (__time_t) == 8)
> > +      if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
> >         { 
> >           TEST_COMPARE (setitimer (timers[i], &it, NULL), 0);
> 
> This looks good to me, I can update to this, test and resend the patch when I
> get some time.  Probably later tonight.

I tested this and it exposes an issue in the linux setitimer wrapper.  On my
platform I get EINVAL instead of EOVERFLOW.

    FAIL: time/tst-itimer
    original exit status 1
    tst-itimer.c:125: numeric comparison failure
       left: 22 (0x16); from: errno
      right: 75 (0x4b); from: EOVERFLOW
    tst-itimer.c:147: numeric comparison failure
       left: 22 (0x16); from: errno
      right: 75 (0x4b); from: EOVERFLOW


It seems this is because sysdeps/unix/sysv/linux/setitimer.c, checks that
the incoming value is in the range of time_t.  The problem is that
that we need to fit the value in __int32_t not time_t.  When testing the time_t
range check does not detect the overflow and setitimer ends up passing a -1 to
the kernel causing EINVAL.

I can fix that, as per the patch below, but It will take me some time to audit
other places this might be an issue.



  if (! in_time_t_range (new_value->it_interval.tv_sec)
      || ! in_time_t_range (new_value->it_value.tv_sec))
    {
      __set_errno (EOVERFLOW);
      return -1;
    }
  new_value_32.it_interval
    = valid_timeval64_to_timeval32 (new_value->it_interval);
  new_value_32.it_value
    = valid_timeval64_to_timeval32 (new_value->it_value);




The below patch works for me, but there is probably a better thing to do then
create a new functrion.



diff --git a/include/time.h b/include/time.h
index 4372bfbd96..377a4a45ea 100644
--- a/include/time.h
+++ b/include/time.h
@@ -342,6 +342,14 @@ in_time_t_range (__time64_t t)
   return s == t;
 }
 
+/* Check whether T fits in a timeval32 (__int32_t).  */
+static inline bool
+in_timeval32_range (__time64_t t)
+{
+  __int32_t s = t;
+  return s == t;
+}
+
 /* Convert a known valid struct timeval into a struct __timespec64.  */
 static inline struct __timespec64
 valid_timeval_to_timespec64 (const struct timeval tv)
diff --git a/sysdeps/unix/sysv/linux/setitimer.c
b/sysdeps/unix/sysv/linux/setitimer.c
index 083a25cf35..bada30ba02 100644
--- a/sysdeps/unix/sysv/linux/setitimer.c
+++ b/sysdeps/unix/sysv/linux/setitimer.c
@@ -32,8 +32,8 @@ __setitimer64 (__itimer_which_t which,
 #else
   struct __itimerval32 new_value_32;
 
-  if (! in_time_t_range (new_value->it_interval.tv_sec)
-      || ! in_time_t_range (new_value->it_value.tv_sec))
+  if (! in_timeval32_range (new_value->it_interval.tv_sec)
+      || ! in_timeval32_range (new_value->it_value.tv_sec))
     {


-Stafford
Adhemerval Zanella July 6, 2021, 7:58 p.m. UTC | #4
On 12/06/2021 06:19, Stafford Horne wrote:
> On Thu, Jun 10, 2021 at 06:38:04AM +0900, Stafford Horne wrote:
>> On Wed, Jun 09, 2021 at 10:50:23AM -0300, Adhemerval Zanella wrote:
>>>
>>>
>>> On 07/06/2021 10:18, Stafford Horne wrote:
>>>> On the port of OpenRISC I am working on and it appears the rv32 port
>>>> we have sets __TIMESIZE == 64 && __WORDSIZE == 32.  This causes the
>>>> size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit
>>>> causing truncation.
>>>>
>>>> The truncations are unavoidable on these systems so skip the
>>>> testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64.
>>>
>>> Sigh, I was hoping that we won't need to handle this situation (glibc 
>>> support only 64-bit time_t, but kernel still providing some 32-bit 
>>> syscall).
>>>
>>>> ---
>>>>
>>>> I am open to other suggestions, this seemed the most correct to me.
>>>>
>>>> Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>>> Cc: Alistair Francis <alistair.francis@wdc.com>
>>>>
>>>>  time/tst-itimer.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/time/tst-itimer.c b/time/tst-itimer.c
>>>> index 929c2b74c7..0c99d46d7e 100644
>>>> --- a/time/tst-itimer.c
>>>> +++ b/time/tst-itimer.c
>>>> @@ -89,6 +89,7 @@ do_test (void)
>>>>        TEST_COMPARE (it.it_interval.tv_sec, it_old.it_interval.tv_sec);
>>>>        TEST_COMPARE (it.it_interval.tv_usec, it_old.it_interval.tv_usec);
>>>>  
>>>> +#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64
>>>>        if (sizeof (time_t) == 4)
>>>>  	continue;
>>>>  
>>>> @@ -146,6 +147,7 @@ do_test (void)
>>>>  	  TEST_COMPARE (setitimer (timers[i], &it, NULL), -1);
>>>>  	  TEST_COMPARE (errno, EOVERFLOW);
>>>>  	}
>>>> +#endif
>>>>    }
>>>>  
>>>>    {
>>>>
>>>
>>> Instead of disabling, I think it would be better to use
>>> __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 instead of __time_t sizeof
>>> (so we can still tests the EOVERFLOW):
>>>
>>> diff --git a/time/tst-itimer.c b/time/tst-itimer.c
>>> index 929c2b74c7..bd7d7afe83 100644
>>> --- a/time/tst-itimer.c
>>> +++ b/time/tst-itimer.c
>>> @@ -100,7 +100,7 @@ do_test (void)
>>>
>>>        /* Linux does not provide 64 bit time_t support for getitimer and
>>>          setitimer on architectures with 32 bit time_t support.  */
>>> -      if (sizeof (__time_t) == 8)
>>> +      if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
>>>         { 
>>>           TEST_COMPARE (setitimer (timers[i], &it, NULL), 0);
>>>           TEST_COMPARE (setitimer (timers[i], &(struct itimerval) { 0 },
>>> @@ -131,7 +131,7 @@ do_test (void)
>>>        it.it_interval.tv_usec = 20;
>>>        it.it_value.tv_sec = 30;
>>>        it.it_value.tv_usec = 40;
>>> -      if (sizeof (__time_t) == 8)
>>> +      if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
>>>         { 
>>>           TEST_COMPARE (setitimer (timers[i], &it, NULL), 0);
>>
>> This looks good to me, I can update to this, test and resend the patch when I
>> get some time.  Probably later tonight.
> 
> I tested this and it exposes an issue in the linux setitimer wrapper.  On my
> platform I get EINVAL instead of EOVERFLOW.
> 
>     FAIL: time/tst-itimer
>     original exit status 1
>     tst-itimer.c:125: numeric comparison failure
>        left: 22 (0x16); from: errno
>       right: 75 (0x4b); from: EOVERFLOW
>     tst-itimer.c:147: numeric comparison failure
>        left: 22 (0x16); from: errno
>       right: 75 (0x4b); from: EOVERFLOW
> 
> 
> It seems this is because sysdeps/unix/sysv/linux/setitimer.c, checks that
> the incoming value is in the range of time_t.  The problem is that
> that we need to fit the value in __int32_t not time_t.  When testing the time_t
> range check does not detect the overflow and setitimer ends up passing a -1 to
> the kernel causing EINVAL.
> 
> I can fix that, as per the patch below, but It will take me some time to audit
> other places this might be an issue.
> 
> 
> 
>   if (! in_time_t_range (new_value->it_interval.tv_sec)
>       || ! in_time_t_range (new_value->it_value.tv_sec))
>     {
>       __set_errno (EOVERFLOW);
>       return -1;
>     }
>   new_value_32.it_interval
>     = valid_timeval64_to_timeval32 (new_value->it_interval);
>   new_value_32.it_value
>     = valid_timeval64_to_timeval32 (new_value->it_value);
> 
> 
> 

Sigh... it seems that openrisc will the only *one* architecture with
64-bit time_t in userland which uses legacy 32-bit kernel ABI. 

> 
> The below patch works for me, but there is probably a better thing to do then
> create a new functrion.
> 
> 
> 
> diff --git a/include/time.h b/include/time.h
> index 4372bfbd96..377a4a45ea 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -342,6 +342,14 @@ in_time_t_range (__time64_t t)
>    return s == t;
>  }
>  
> +/* Check whether T fits in a timeval32 (__int32_t).  */
> +static inline bool
> +in_timeval32_range (__time64_t t)
> +{
> +  __int32_t s = t;
> +  return s == t;
> +}
> +

The name is confusing, it is mixing timeval from 'struct timeval' and
time_t.  And there is no need to use __int32_t, we need to use it only
on installed headers to avoid namespace pollution. 

I fact I think it would be better to just change 'in_time_t_range' to
use int32_t internally instead of time_t; I am pretty sure that all
usages assume that sizeof(time_t) == 32.
Stafford Horne July 7, 2021, 9:11 p.m. UTC | #5
On Tue, Jul 06, 2021 at 04:58:58PM -0300, Adhemerval Zanella wrote:
> 
...
> 
> Sigh... it seems that openrisc will the only *one* architecture with
> 64-bit time_t in userland which uses legacy 32-bit kernel ABI. 


In order to adhere to the "don't break userspace" rule I don't think I can
change that, I would like to.  Is there any documented migration path you know
of?

> > 
> > The below patch works for me, but there is probably a better thing to do then
> > create a new functrion.
> > 
> > 
> > 
> > diff --git a/include/time.h b/include/time.h
> > index 4372bfbd96..377a4a45ea 100644
> > --- a/include/time.h
> > +++ b/include/time.h
> > @@ -342,6 +342,14 @@ in_time_t_range (__time64_t t)
> >    return s == t;
> >  }
> >  
> > +/* Check whether T fits in a timeval32 (__int32_t).  */
> > +static inline bool
> > +in_timeval32_range (__time64_t t)
> > +{
> > +  __int32_t s = t;
> > +  return s == t;
> > +}
> > +
> 
> The name is confusing, it is mixing timeval from 'struct timeval' and
> time_t.  And there is no need to use __int32_t, we need to use it only
> on installed headers to avoid namespace pollution. 

Right, I did think about that, but just wanted to get something to show the
change, I couldn't think of a better name.

> I fact I think it would be better to just change 'in_time_t_range' to
> use int32_t internally instead of time_t; I am pretty sure that all
> usages assume that sizeof(time_t) == 32.

Yeah, I like that better too.  I will respin the patch and post again.

-Stafford
diff mbox series

Patch

diff --git a/time/tst-itimer.c b/time/tst-itimer.c
index 929c2b74c7..0c99d46d7e 100644
--- a/time/tst-itimer.c
+++ b/time/tst-itimer.c
@@ -89,6 +89,7 @@  do_test (void)
       TEST_COMPARE (it.it_interval.tv_sec, it_old.it_interval.tv_sec);
       TEST_COMPARE (it.it_interval.tv_usec, it_old.it_interval.tv_usec);
 
+#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64
       if (sizeof (time_t) == 4)
 	continue;
 
@@ -146,6 +147,7 @@  do_test (void)
 	  TEST_COMPARE (setitimer (timers[i], &it, NULL), -1);
 	  TEST_COMPARE (errno, EOVERFLOW);
 	}
+#endif
   }
 
   {