diff mbox series

Fix clock_nanosleep when interrupted by a signal

Message ID 20191111163454.22535-1-adhemerval.zanella@linaro.org
State Accepted
Commit 42b926d303e571d5f9a4e97ffdb8e05d1eabae66
Headers show
Series Fix clock_nanosleep when interrupted by a signal | expand

Commit Message

Adhemerval Zanella Netto Nov. 11, 2019, 4:34 p.m. UTC 
This patch fixes the time64 support (added by 2e44b10b42d) where it
misses the remaining argument updated if __NR_clock_nanosleep
returns EINTR.

Checked on i686-linux-gnu on 4.15 kernel (no time64 support) and
on 5.3 kernel (with time64 support).

Change-Id: Ie2d3ffdcf52a9a4f1e49466fd6abece31a7c7e69
---
 sysdeps/unix/sysv/linux/clock_nanosleep.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

-- 
2.17.1

Comments

Alistair Francis Nov. 11, 2019, 5:30 p.m. UTC | #1 
On Mon, Nov 11, 2019 at 8:35 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>

> This patch fixes the time64 support (added by 2e44b10b42d) where it

> misses the remaining argument updated if __NR_clock_nanosleep

> returns EINTR.

>

> Checked on i686-linux-gnu on 4.15 kernel (no time64 support) and

> on 5.3 kernel (with time64 support).

>

> Change-Id: Ie2d3ffdcf52a9a4f1e49466fd6abece31a7c7e69


Thanks for the fix!

> ---

>  sysdeps/unix/sysv/linux/clock_nanosleep.c | 16 +++++++---------

>  1 file changed, 7 insertions(+), 9 deletions(-)

>

> diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c

> index 60c61ee277..fc47c58ee7 100644

> --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c

> +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c

> @@ -52,13 +52,10 @@ __clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec

>    r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id,

>                                 flags, req, rem);

>

> -  if (r == 0 || errno != ENOSYS)

> -    {

> -      return (INTERNAL_SYSCALL_ERROR_P (r, err)

> -              ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);

> -    }

> +  if (r != -ENOSYS)

> +    return (INTERNAL_SYSCALL_ERROR_P (r, err)

> +            ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);

>  # endif /* __NR_clock_nanosleep_time64 */


Ok

> -  struct timespec ts32, tr32;

>

>    if (! in_time_t_range (req->tv_sec))

>      {

> @@ -66,11 +63,12 @@ __clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec

>        return -1;

>      }

>

> -  ts32 = valid_timespec64_to_timespec (*req);

> +  struct timespec tr32;

> +  struct timespec ts32 = valid_timespec64_to_timespec (*req);

>    r =  INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags,

>                                  &ts32, &tr32);

>

> -  if (r == 0 && rem != NULL)

> +  if (r == -EINTR && rem != NULL && (flags & TIMER_ABSTIME) == 0)

>      *rem = valid_timespec_to_timespec64 (tr32);


Don't we also need to set this if the call succeeded?

>  #endif /* __ASSUME_TIME64_SYSCALLS */

>

> @@ -89,7 +87,7 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,

>    treq64 = valid_timespec_to_timespec64 (*req);

>    r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64);

>

> -  if (r == 0 && rem != NULL)

> +  if (r == EINTR && rem != NULL && (flags & TIMER_ABSTIME) == 0)

>      *rem = valid_timespec64_to_timespec (trem64);


Same with this one.

Alistair

>

>    return r;

> --

> 2.17.1

>
Adhemerval Zanella Netto Nov. 11, 2019, 6:07 p.m. UTC | #2 
On 11/11/2019 14:30, Alistair Francis wrote:
> On Mon, Nov 11, 2019 at 8:35 AM Adhemerval Zanella

> <adhemerval.zanella@linaro.org> wrote:

>>

>> This patch fixes the time64 support (added by 2e44b10b42d) where it

>> misses the remaining argument updated if __NR_clock_nanosleep

>> returns EINTR.

>>

>> Checked on i686-linux-gnu on 4.15 kernel (no time64 support) and

>> on 5.3 kernel (with time64 support).

>>

>> Change-Id: Ie2d3ffdcf52a9a4f1e49466fd6abece31a7c7e69

> 

> Thanks for the fix!

> 

>> ---

>>  sysdeps/unix/sysv/linux/clock_nanosleep.c | 16 +++++++---------

>>  1 file changed, 7 insertions(+), 9 deletions(-)

>>

>> diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c

>> index 60c61ee277..fc47c58ee7 100644

>> --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c

>> +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c

>> @@ -52,13 +52,10 @@ __clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec

>>    r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id,

>>                                 flags, req, rem);

>>

>> -  if (r == 0 || errno != ENOSYS)

>> -    {

>> -      return (INTERNAL_SYSCALL_ERROR_P (r, err)

>> -              ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);

>> -    }

>> +  if (r != -ENOSYS)

>> +    return (INTERNAL_SYSCALL_ERROR_P (r, err)

>> +            ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);

>>  # endif /* __NR_clock_nanosleep_time64 */

> 

> Ok

> 

>> -  struct timespec ts32, tr32;

>>

>>    if (! in_time_t_range (req->tv_sec))

>>      {

>> @@ -66,11 +63,12 @@ __clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec

>>        return -1;

>>      }

>>

>> -  ts32 = valid_timespec64_to_timespec (*req);

>> +  struct timespec tr32;

>> +  struct timespec ts32 = valid_timespec64_to_timespec (*req);

>>    r =  INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags,

>>                                  &ts32, &tr32);

>>

>> -  if (r == 0 && rem != NULL)

>> +  if (r == -EINTR && rem != NULL && (flags & TIMER_ABSTIME) == 0)

>>      *rem = valid_timespec_to_timespec64 (tr32);

> 

> Don't we also need to set this if the call succeeded?


If the syscall succeeds it is implicit that sleep with input timeout argument
has happened and there is no remaining time.  And if I am reading the kernel
code correctly, this is what the syscall does:

*kernel/time/posix-timers.c

1208 SYSCALL_DEFINE4(clock_nanosleep, const clockid_t, which_clock, int, flags,
1209                 const struct __kernel_timespec __user *, rqtp,
1210                 struct __kernel_timespec __user *, rmtp)
1211 {
[...]
1227         current->restart_block.nanosleep.type = rmtp ? TT_NATIVE : TT_NONE;
1228         current->restart_block.nanosleep.rmtp = rmtp;
1229 
1230         return kc->nsleep(which_clock, flags, &t);
1231 }
[...]

The RMTP output timespec is set on the current taks restart_block.

1197 /*
1198  * nanosleep for monotonic and realtime clocks
1199  */
1200 static int common_nsleep(const clockid_t which_clock, int flags,
1201                          const struct timespec64 *rqtp)
1202 {
1203         return hrtimer_nanosleep(rqtp, flags & TIMER_ABSTIME ?
1204                                  HRTIMER_MODE_ABS : HRTIMER_MODE_REL,
1205                                  which_clock);
1206 }
[...]

* kernel/time/hrtimer.c

1862 static int __sched do_nanosleep(struct hrtimer_sleeper *t, enum hrtimer_mode mode)           
1863 {  
1864         struct restart_block *restart;                                                       
1865    
1866         do {
1867                 set_current_state(TASK_INTERRUPTIBLE);
1868                 hrtimer_sleeper_start_expires(t, mode);                                      
1869            
1870                 if (likely(t->task))
1871                         freezable_schedule();                                                
1872            
1873                 hrtimer_cancel(&t->timer);                                                   
1874                 mode = HRTIMER_MODE_ABS;                                                     
1875    
1876         } while (t->task && !signal_pending(current));                                       
1877    
1878         __set_current_state(TASK_RUNNING);                                                   
1879    
1880         if (!t->task)
1881                 return 0;                                                                    
1882    
1883         restart = &current->restart_block;
1884         if (restart->nanosleep.type != TT_NONE) {
1885                 ktime_t rem = hrtimer_expires_remaining(&t->timer);                          
1886                 struct timespec64 rmt;                                                       
1887            
1888                 if (rem <= 0)
1889                         return 0;
1890                 rmt = ktime_to_timespec64(rem);                                              
1891            
1892                 return nanosleep_copyout(restart, &rmt);                                     
1893         }
1894         return -ERESTART_RESTARTBLOCK;                                                       
1895 }  
 
So the remaining timeout will be set iff the syscall has been interrupted
(rem > 0) and then nanosleep_copyout will copy out to userspace the remaining
time value.

So to mimic kernel behaviour we should update it only if it has been
interrupted.

> 

>>  #endif /* __ASSUME_TIME64_SYSCALLS */

>>

>> @@ -89,7 +87,7 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,

>>    treq64 = valid_timespec_to_timespec64 (*req);

>>    r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64);

>>

>> -  if (r == 0 && rem != NULL)

>> +  if (r == EINTR && rem != NULL && (flags & TIMER_ABSTIME) == 0)

>>      *rem = valid_timespec64_to_timespec (trem64);

> 

> Same with this one.

> 

> Alistair

> 

>>

>>    return r;

>> --

>> 2.17.1

>>
Alistair Francis Nov. 11, 2019, 6:51 p.m. UTC | #3 
On Mon, Nov 11, 2019 at 10:08 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>

>

>

> On 11/11/2019 14:30, Alistair Francis wrote:

> > On Mon, Nov 11, 2019 at 8:35 AM Adhemerval Zanella

> > <adhemerval.zanella@linaro.org> wrote:

> >>

> >> This patch fixes the time64 support (added by 2e44b10b42d) where it

> >> misses the remaining argument updated if __NR_clock_nanosleep

> >> returns EINTR.

> >>

> >> Checked on i686-linux-gnu on 4.15 kernel (no time64 support) and

> >> on 5.3 kernel (with time64 support).

> >>

> >> Change-Id: Ie2d3ffdcf52a9a4f1e49466fd6abece31a7c7e69

> >

> > Thanks for the fix!

> >

> >> ---

> >>  sysdeps/unix/sysv/linux/clock_nanosleep.c | 16 +++++++---------

> >>  1 file changed, 7 insertions(+), 9 deletions(-)

> >>

> >> diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c

> >> index 60c61ee277..fc47c58ee7 100644

> >> --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c

> >> +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c

> >> @@ -52,13 +52,10 @@ __clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec

> >>    r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id,

> >>                                 flags, req, rem);

> >>

> >> -  if (r == 0 || errno != ENOSYS)

> >> -    {

> >> -      return (INTERNAL_SYSCALL_ERROR_P (r, err)

> >> -              ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);

> >> -    }

> >> +  if (r != -ENOSYS)

> >> +    return (INTERNAL_SYSCALL_ERROR_P (r, err)

> >> +            ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);

> >>  # endif /* __NR_clock_nanosleep_time64 */

> >

> > Ok

> >

> >> -  struct timespec ts32, tr32;

> >>

> >>    if (! in_time_t_range (req->tv_sec))

> >>      {

> >> @@ -66,11 +63,12 @@ __clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec

> >>        return -1;

> >>      }

> >>

> >> -  ts32 = valid_timespec64_to_timespec (*req);

> >> +  struct timespec tr32;

> >> +  struct timespec ts32 = valid_timespec64_to_timespec (*req);

> >>    r =  INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags,

> >>                                  &ts32, &tr32);

> >>

> >> -  if (r == 0 && rem != NULL)

> >> +  if (r == -EINTR && rem != NULL && (flags & TIMER_ABSTIME) == 0)

> >>      *rem = valid_timespec_to_timespec64 (tr32);

> >

> > Don't we also need to set this if the call succeeded?

>

> If the syscall succeeds it is implicit that sleep with input timeout argument

> has happened and there is no remaining time.  And if I am reading the kernel

> code correctly, this is what the syscall does:

>

> *kernel/time/posix-timers.c

>

> 1208 SYSCALL_DEFINE4(clock_nanosleep, const clockid_t, which_clock, int, flags,

> 1209                 const struct __kernel_timespec __user *, rqtp,

> 1210                 struct __kernel_timespec __user *, rmtp)

> 1211 {

> [...]

> 1227         current->restart_block.nanosleep.type = rmtp ? TT_NATIVE : TT_NONE;

> 1228         current->restart_block.nanosleep.rmtp = rmtp;

> 1229

> 1230         return kc->nsleep(which_clock, flags, &t);

> 1231 }

> [...]

>

> The RMTP output timespec is set on the current taks restart_block.

>

> 1197 /*

> 1198  * nanosleep for monotonic and realtime clocks

> 1199  */

> 1200 static int common_nsleep(const clockid_t which_clock, int flags,

> 1201                          const struct timespec64 *rqtp)

> 1202 {

> 1203         return hrtimer_nanosleep(rqtp, flags & TIMER_ABSTIME ?

> 1204                                  HRTIMER_MODE_ABS : HRTIMER_MODE_REL,

> 1205                                  which_clock);

> 1206 }

> [...]

>

> * kernel/time/hrtimer.c

>

> 1862 static int __sched do_nanosleep(struct hrtimer_sleeper *t, enum hrtimer_mode mode)

> 1863 {

> 1864         struct restart_block *restart;

> 1865

> 1866         do {

> 1867                 set_current_state(TASK_INTERRUPTIBLE);

> 1868                 hrtimer_sleeper_start_expires(t, mode);

> 1869

> 1870                 if (likely(t->task))

> 1871                         freezable_schedule();

> 1872

> 1873                 hrtimer_cancel(&t->timer);

> 1874                 mode = HRTIMER_MODE_ABS;

> 1875

> 1876         } while (t->task && !signal_pending(current));

> 1877

> 1878         __set_current_state(TASK_RUNNING);

> 1879

> 1880         if (!t->task)

> 1881                 return 0;

> 1882

> 1883         restart = &current->restart_block;

> 1884         if (restart->nanosleep.type != TT_NONE) {

> 1885                 ktime_t rem = hrtimer_expires_remaining(&t->timer);

> 1886                 struct timespec64 rmt;

> 1887

> 1888                 if (rem <= 0)

> 1889                         return 0;

> 1890                 rmt = ktime_to_timespec64(rem);

> 1891

> 1892                 return nanosleep_copyout(restart, &rmt);

> 1893         }

> 1894         return -ERESTART_RESTARTBLOCK;

> 1895 }

>

> So the remaining timeout will be set iff the syscall has been interrupted

> (rem > 0) and then nanosleep_copyout will copy out to userspace the remaining

> time value.

>

> So to mimic kernel behaviour we should update it only if it has been

> interrupted.


Ah, my misunderstanding there.

In which case this patch looks good to me. Thanks again for fixing the
regression.

Alistair

>

> >

> >>  #endif /* __ASSUME_TIME64_SYSCALLS */

> >>

> >> @@ -89,7 +87,7 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,

> >>    treq64 = valid_timespec_to_timespec64 (*req);

> >>    r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64);

> >>

> >> -  if (r == 0 && rem != NULL)

> >> +  if (r == EINTR && rem != NULL && (flags & TIMER_ABSTIME) == 0)

> >>      *rem = valid_timespec64_to_timespec (trem64);

> >

> > Same with this one.

> >

> > Alistair

> >

> >>

> >>    return r;

> >> --

> >> 2.17.1

> >>
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c
index 60c61ee277..fc47c58ee7 100644
--- a/sysdeps/unix/sysv/linux/clock_nanosleep.c
+++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c
@@ -52,13 +52,10 @@  __clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec
   r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id,
                                flags, req, rem);
 
-  if (r == 0 || errno != ENOSYS)
-    {
-      return (INTERNAL_SYSCALL_ERROR_P (r, err)
-              ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);
-    }
+  if (r != -ENOSYS)
+    return (INTERNAL_SYSCALL_ERROR_P (r, err)
+            ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);
 # endif /* __NR_clock_nanosleep_time64 */
-  struct timespec ts32, tr32;
 
   if (! in_time_t_range (req->tv_sec))
     {
@@ -66,11 +63,12 @@  __clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec
       return -1;
     }
 
-  ts32 = valid_timespec64_to_timespec (*req);
+  struct timespec tr32;
+  struct timespec ts32 = valid_timespec64_to_timespec (*req);
   r =  INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags,
                                 &ts32, &tr32);
 
-  if (r == 0 && rem != NULL)
+  if (r == -EINTR && rem != NULL && (flags & TIMER_ABSTIME) == 0)
     *rem = valid_timespec_to_timespec64 (tr32);
 #endif /* __ASSUME_TIME64_SYSCALLS */
 
@@ -89,7 +87,7 @@  __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,
   treq64 = valid_timespec_to_timespec64 (*req);
   r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64);
 
-  if (r == 0 && rem != NULL)
+  if (r == EINTR && rem != NULL && (flags & TIMER_ABSTIME) == 0)
     *rem = valid_timespec64_to_timespec (trem64);
 
   return r;