Re: [PATCH 2/2] io_getevents: Use timespec64 to represent timeouts

[Arnd Bergmann]

On Sat, Aug 5, 2017 at 6:12 AM, Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote:
> struct timespec is not y2038 safe. Use y2038 safe
> struct timespec64 to represent timeouts.
> The system call interface itself will be changed as
> part of different series.
>
> Timeouts will not really need more than 32 bits.
> But, replacing these with timespec64 helps verification
> of a y2038 safe kernel by getting rid of timespec
> internally.
>
> Signed-off-by: Deepa Dinamani <deepa.kernel@xxxxxxxxx>
> Cc: linux-aio@xxxxxxxxx

Nice cleanup of the compat path!

>  static long read_events(struct kioctx *ctx, long min_nr, long nr,
>                         struct io_event __user *event,
> -                       struct timespec __user *timeout)
> +                       ktime_t until)
>  {
> -       ktime_t until = KTIME_MAX;
>         long ret = 0;
>
> -       if (timeout) {
> -               struct timespec ts;
> -
> -               if (unlikely(copy_from_user(&ts, timeout, sizeof(ts))))
> -                       return -EFAULT;
> -
> -               until = timespec_to_ktime(ts);
> -       }
> -
>         /*
>          * Note that aio_read_events() is being called as the conditional - i.e.
>          * we're calling it after prepare_to_wait() has set task state to
> @@ -1824,6 +1814,25 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb,
>         return ret;
>  }
>
> +static long do_io_getevents(aio_context_t ctx_id,
> +               long min_nr,
> +               long nr,
> +               struct io_event __user *events,
> +               struct timespec64 *ts)
> +{
> +       ktime_t until = ts ? timespec64_to_ktime(*ts) : KTIME_MAX;
> +       struct kioctx *ioctx = lookup_ioctx(ctx_id);
> +       long ret = -EINVAL;
> +
> +       if (likely(ioctx)) {
> +               if (likely(min_nr <= nr && min_nr >= 0))
> +                       ret = read_events(ioctx, min_nr, nr, events, until);
> +               percpu_ref_put(&ioctx->users);
> +       }
> +
> +       return ret;
> +}

I guess these two functions are small enough that they could be merged
into one, saving a few lines. Then again, fs/aio.c seems to generally use
fairly short functions doing not too much at once, so your approach maybe
fits better with the style of the subsystem.

Either way,

Reviewed-by: Arnd Bergmann <arnd@xxxxxxxx>

       Arnd