linux-kernel - Re: [PATCH] kernel/time: Add hr_sleep syscall, a high-resolution sleep service

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] [day] [month] [year] [list]

Message-ID: <CALCETrWfnL=3M3nmmHs-a3si5JptSCtF6cEtHVtsDNwA5mHnRg@mail.gmail.com>
Date:   Fri, 15 Jan 2021 10:52:49 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     Marco Faltelli <marco.faltelli@...roma2.it>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>
Subject: Re: [PATCH] kernel/time: Add hr_sleep syscall, a high-resolution
 sleep service

On Fri, Jan 15, 2021 at 10:14 AM Marco Faltelli
<marco.faltelli@...roma2.it> wrote:
>
> hr_sleep is a new system call engineered for nanosecond time scale
> granularities.
> With respect to nanosleep, it uses a single value representation
> of the sleep period.
> hr_sleep achieves 15x improvement for microsecond scale timers
> w.r.t. nanosleep:

You need to explain what 15x means.

> +/**
> + * hr_sleep - a high-resolution sleep service for fine-grained timeouts
> + * @nanoseconds:       the requested sleep period in nanoseconds
> + *
> + * Returns:
> + * 0 when the sleep request successfully terminated
> + * -EINVAL if a sleep period < 0 is requested
> + */
> +SYSCALL_DEFINE1(hr_sleep, long, nanoseconds)
> +{
> +       DECLARE_WAIT_QUEUE_HEAD(the_queue);//here we use a private queue
> +       control_record *control;
> +       ktime_t ktime_interval;
> +
> +       if (nanoseconds < 0)
> +               return -EINVAL;
> +
> +       if (nanoseconds == 0)
> +               return 0;
> +
> +       ktime_interval = ktime_set(0, nanoseconds);
> +       control = (control_record *)((void *) current->stack + sizeof(struct thread_info));

What is this trying to do other than writing to memory you don't own?

In C, the way you create a structure on the stack is:

struct control_record control;

done, problem solved.  (Also, on modern kernels, thread_info is not on
the stack, so your arithmetic makes no sense.

> +       wait_event_interruptible(the_queue, control->awake == 1);

Once you start checking the return value (which is absolutely
necessary), you will discover that you need to do something sensible
when the return value is nonzero.  At that point you will start
contemplating how to restart an interrupted wait, and you will have to
consider the merits of absolute vs relative timeouts and you might
discover that the complexity of nanosleep(2) is not an accident.

Sure, the seconds/nanoseconds split in nanosleep(2) is probably not
ideal, and a better API could probably be designed, but your hr_sleep
needs a lot of work to cover all the cases.

--Andy