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

Message ID 20191108153344.10949-1-lukma@denx.de
State New
Headers show
Series
  • [1/2] linux: clock_settime: Remove check for nanoseconds validity
Related show

Commit Message

Lukasz Majewski Nov. 8, 2019, 3:33 p.m. UTC
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(-)

Comments

Alistair Francis Nov. 8, 2019, 4:20 p.m. UTC | #1
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
>
Lukasz Majewski Nov. 11, 2019, 3:45 p.m. UTC | #2
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
Lukasz Majewski Nov. 27, 2019, 5:04 p.m. UTC | #3
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
Paul Eggert Nov. 27, 2019, 7:27 p.m. UTC | #4
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.

Patch
diff mbox series

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