[1/2] linux: clock_settime: Remove check for nanoseconds validity

  The check if passed nanoseconds via struct __timespec64's *tp pointer is
also performed in the Linux kernel. Remove it from glibc to avoid
duplication.
---
 sysdeps/unix/sysv/linux/clock_settime.c | 7 -------
 1 file changed, 7 deletions(-)
  

On Fri, Nov 8, 2019 at 7:34 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> The check if passed nanoseconds via struct __timespec64's *tp pointer is
> also performed in the Linux kernel. Remove it from glibc to avoid
> duplication.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  sysdeps/unix/sysv/linux/clock_settime.c | 7 -------
>  1 file changed, 7 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/sysv/linux/clock_settime.c
> index bda113809b..6706dbb31f 100644
> --- a/sysdeps/unix/sysv/linux/clock_settime.c
> +++ b/sysdeps/unix/sysv/linux/clock_settime.c
> @@ -25,13 +25,6 @@
>  int
>  __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
>  {
> -  /* Make sure the time cvalue is OK.  */
> -  if (! valid_nanoseconds (tp->tv_nsec))
> -    {
> -      __set_errno (EINVAL);
> -      return -1;
> -    }
> -
>  #ifdef __ASSUME_TIME64_SYSCALLS
>  # ifndef __NR_clock_settime64
>  #  define __NR_clock_settime64 __NR_clock_settime
> --
> 2.20.1
>
  

Dear Joseph, Paul,

> The check if passed nanoseconds via struct __timespec64's *tp pointer
> is also performed in the Linux kernel. Remove it from glibc to avoid
> duplication.
> ---
>  sysdeps/unix/sysv/linux/clock_settime.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/clock_settime.c
> b/sysdeps/unix/sysv/linux/clock_settime.c index
> bda113809b..6706dbb31f 100644 ---
> a/sysdeps/unix/sysv/linux/clock_settime.c +++
> b/sysdeps/unix/sysv/linux/clock_settime.c @@ -25,13 +25,6 @@
>  int
>  __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
>  {
> -  /* Make sure the time cvalue is OK.  */
> -  if (! valid_nanoseconds (tp->tv_nsec))
> -    {
> -      __set_errno (EINVAL);
> -      return -1;
> -    }
> -

I'm just wondering if this patch is OK, as with other patches, which 
convert time to use 64 bit syscalls we do rely on Linux kernel to
check the nanoseconds (and return proper error).

>  #ifdef __ASSUME_TIME64_SYSCALLS
>  # ifndef __NR_clock_settime64
>  #  define __NR_clock_settime64 __NR_clock_settime




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

Dear Joseph, Paul,

> Dear Joseph, Paul,
> 
> > The check if passed nanoseconds via struct __timespec64's *tp
> > pointer is also performed in the Linux kernel. Remove it from glibc
> > to avoid duplication.
> > ---
> >  sysdeps/unix/sysv/linux/clock_settime.c | 7 -------
> >  1 file changed, 7 deletions(-)
> > 
> > diff --git a/sysdeps/unix/sysv/linux/clock_settime.c
> > b/sysdeps/unix/sysv/linux/clock_settime.c index
> > bda113809b..6706dbb31f 100644 ---
> > a/sysdeps/unix/sysv/linux/clock_settime.c +++
> > b/sysdeps/unix/sysv/linux/clock_settime.c @@ -25,13 +25,6 @@
> >  int
> >  __clock_settime64 (clockid_t clock_id, const struct __timespec64
> > *tp) {
> > -  /* Make sure the time cvalue is OK.  */
> > -  if (! valid_nanoseconds (tp->tv_nsec))
> > -    {
> > -      __set_errno (EINVAL);
> > -      return -1;
> > -    }
> > -  
> 
> I'm just wondering if this patch is OK, as with other patches, which 
> convert time to use 64 bit syscalls we do rely on Linux kernel to
> check the nanoseconds (and return proper error).

Are there any comments regarding this patch?

> 
> >  #ifdef __ASSUME_TIME64_SYSCALLS
> >  # ifndef __NR_clock_settime64
> >  #  define __NR_clock_settime64 __NR_clock_settime  
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma@denx.de




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

On 11/27/19 9:04 AM, Lukasz Majewski wrote:

>> a/sysdeps/unix/sysv/linux/clock_settime.c +++
>> b/sysdeps/unix/sysv/linux/clock_settime.c @@ -25,13 +25,6 @@
>>   int
>>   __clock_settime64 (clockid_t clock_id, const struct __timespec64
>> *tp) {
>> -  /* Make sure the time cvalue is OK.  */
>> -  if (! valid_nanoseconds (tp->tv_nsec))
>> -    {
>> -      __set_errno (EINVAL);
>> -      return -1;
>> -    }
>> -
> I'm just wondering if this patch is OK, as with other patches, which
> convert time to use 64 bit syscalls we do rely on Linux kernel to
> check the nanoseconds (and return proper error).

Suppose tp->tv_sec == 1 && tp->tv_nsec == -1 && !defined 
__ASSUME_TIME64_SYSCALLS && !defined __NR_clock_settime64. Then the 
current code will fail with errno == EINVAL, but with the proposed patch 
the code will succeed and set the time to 1 second after the Epoch.

Code should always check for valid nanoseconds before calling 
valid_timespec64_to_timespec with possibly-invalid input. In this 
function, the check can be done at about the same time as the 
in_time_t_range check; that'd be better than what the current code does.