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

[Alistair Francis <alistair23 at gmail dot com>]

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