Re: [PATCH] Fix clock_nanosleep when interrupted by a signal

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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
>>