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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 5 Aug 2017 22:13:50 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Deepa Dinamani <deepa.kernel@...il.com>
Cc:     Al Viro <viro@...iv.linux.org.uk>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux FS-devel Mailing List <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        y2038 Mailman List <y2038@...ts.linaro.org>,
        linux-aio@...ck.org, Benjamin LaHaise <bcrl@...ck.org>
Subject: Re: [PATCH 2/2] io_getevents: Use timespec64 to represent timeouts

On Sat, Aug 5, 2017 at 6:12 AM, Deepa Dinamani <deepa.kernel@...il.com> 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@...il.com>
> Cc: linux-aio@...ck.org

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@...db.de>

       Arnd