Message-ID: <219c5d09-0099-83e9-b21b-299fa513decd@alu.unizg.hr>
Date:   Wed, 8 Mar 2023 16:59:41 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH 8/8] selftests/proc: Assert clock_gettime(CLOCK_BOOTTIME)
 VS /proc/uptime monotonicity
Content-Language: en-US, hr
To:     Frederic Weisbecker <frederic@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        Wei Li <liwei391@huawei.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Yu Liao <liaoyu15@huawei.com>, Hillf Danton <hdanton@sina.com>,
        Ingo Molnar <mingo@kernel.org>
References: <20230222144649.624380-1-frederic@kernel.org>
 <20230222144649.624380-9-frederic@kernel.org>
From:   Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
In-Reply-To: <20230222144649.624380-9-frederic@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Frederic,

On 2/22/23 15:46, Frederic Weisbecker wrote:
> The first field of /proc/uptime relies on the CLOCK_BOOTTIME clock which
> can also be fetched from clock_gettime() API.
> 
> Improve the test coverage while verifying the monotonicity of
> CLOCK_BOOTTIME accross both interfaces.
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Yu Liao <liaoyu15@huawei.com>
> Cc: Hillf Danton <hdanton@sina.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Wei Li <liwei391@huawei.com>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> ---
>   .../testing/selftests/proc/proc-uptime-001.c  | 21 ++++++++++++++----
>   .../testing/selftests/proc/proc-uptime-002.c  | 22 +++++++++++++++----
>   tools/testing/selftests/proc/proc-uptime.h    | 12 ++++++++++
>   3 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/proc/proc-uptime-001.c b/tools/testing/selftests/proc/proc-uptime-001.c
> index 35bddd9dd60b..f335eec5067e 100644
> --- a/tools/testing/selftests/proc/proc-uptime-001.c
> +++ b/tools/testing/selftests/proc/proc-uptime-001.c
> @@ -13,9 +13,9 @@
>    * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
>    * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>    */
> -// Test that boottime value in /proc/uptime increments monotonically.
> -// We don't test idle time monotonicity due to broken iowait task
> -// counting, cf: comment above get_cpu_idle_time_us()
> +// Test that boottime value in /proc/uptime and CLOCK_BOOTTIME increment
> +// monotonically. We don't test idle time monotonicity due to broken iowait
> +// task counting, cf: comment above get_cpu_idle_time_us()
>   #undef NDEBUG
>   #include <assert.h>
>   #include <stdint.h>
> @@ -27,7 +27,7 @@
>   
>   int main(void)
>   {
> -	uint64_t start, u0, u1;
> +	uint64_t start, u0, u1, c0, c1;
>   	int fd;
>   
>   	fd = open("/proc/uptime", O_RDONLY);
> @@ -35,10 +35,23 @@ int main(void)
>   
>   	u0 = proc_uptime(fd);
>   	start = u0;
> +	c0 = clock_boottime();
> +
>   	do {
>   		u1 = proc_uptime(fd);
> +		c1 = clock_boottime();
> +
> +		/* Is /proc/uptime monotonic ? */
>   		assert(u1 >= u0);
> +
> +		/* Is CLOCK_BOOTTIME monotonic ? */
> +		assert(c1 >= c0);
> +
> +		/* Is CLOCK_BOOTTIME VS /proc/uptime monotonic ? */
> +		assert(c0 >= u0);
> +
>   		u0 = u1;
> +		c0 = c1;
>   	} while (u1 - start < 100);
>   
>   	return 0;
> diff --git a/tools/testing/selftests/proc/proc-uptime-002.c b/tools/testing/selftests/proc/proc-uptime-002.c
> index 7ad79d5eaa84..ae453daa96c1 100644
> --- a/tools/testing/selftests/proc/proc-uptime-002.c
> +++ b/tools/testing/selftests/proc/proc-uptime-002.c
> @@ -13,9 +13,10 @@
>    * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
>    * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>    */
> -// Test that boottime value in /proc/uptime increments monotonically
> -// while shifting across CPUs. We don't test idle time monotonicity
> -// due to broken iowait task counting, cf: comment above get_cpu_idle_time_us()
> +// Test that boottime value in /proc/uptime and CLOCK_BOOTTIME increment
> +// monotonically while shifting across CPUs. We don't test idle time
> +// monotonicity due to broken iowait task counting, cf: comment above
> +// get_cpu_idle_time_us()
>   #undef NDEBUG
>   #include <assert.h>
>   #include <errno.h>
> @@ -43,10 +44,10 @@ static inline int sys_sched_setaffinity(pid_t pid, unsigned int len, unsigned lo
>   
>   int main(void)
>   {
> +	uint64_t u0, u1, c0, c1;
>   	unsigned int len;
>   	unsigned long *m;
>   	unsigned int cpu;
> -	uint64_t u0, u1;
>   	int fd;
>   
>   	/* find out "nr_cpu_ids" */
> @@ -62,6 +63,8 @@ int main(void)
>   	assert(fd >= 0);
>   
>   	u0 = proc_uptime(fd);
> +	c0 = clock_boottime();
> +
>   	for (cpu = 0; cpu < len * 8; cpu++) {
>   		memset(m, 0, len);
>   		m[cpu / (8 * sizeof(unsigned long))] |= 1UL << (cpu % (8 * sizeof(unsigned long)));
> @@ -70,8 +73,19 @@ int main(void)
>   		sys_sched_setaffinity(0, len, m);
>   
>   		u1 = proc_uptime(fd);
> +		c1 = clock_boottime();
> +
> +		/* Is /proc/uptime monotonic ? */
>   		assert(u1 >= u0);
> +
> +		/* Is CLOCK_BOOTTIME monotonic ? */
> +		assert(c1 >= c0);
> +
> +		/* Is CLOCK_BOOTTIME VS /proc/uptime monotonic ? */
> +		assert(c0 >= u0);
> +
>   		u0 = u1;
> +		c0 = c1;
>   	}
>   
>   	return 0;
> diff --git a/tools/testing/selftests/proc/proc-uptime.h b/tools/testing/selftests/proc/proc-uptime.h
> index ca55abeb0ccc..730cce4a3d73 100644
> --- a/tools/testing/selftests/proc/proc-uptime.h
> +++ b/tools/testing/selftests/proc/proc-uptime.h
> @@ -19,9 +19,21 @@
>   #include <string.h>
>   #include <stdlib.h>
>   #include <unistd.h>
> +#include <time.h>
>   
>   #include "proc.h"
>   
> +static uint64_t clock_boottime(void)
> +{
> +	struct timespec ts;
> +	int err;
> +
> +	err = clock_gettime(CLOCK_BOOTTIME, &ts);
> +	assert(err >= 0);
> +
> +	return (ts.tv_sec * 100) + (ts.tv_nsec / 10000000);
> +}
> +
>   static uint64_t proc_uptime(int fd)
>   {
>   	uint64_t val1, val2;

 From what I see, you round the CLOCK_BOOTIME time to 1/100ths of a second.

A simple program that queries clock_getres() on system clocks gives this
result:

clock_res [CLOCK_REALTIME] = 0.000000001s
clock_res [CLOCK_REALTIME_COARSE] = 0.004000000s
clock_res [CLOCK_MONOTONIC] = 0.000000001s
clock_res [CLOCK_MONOTONIC_COARSE] = 0.004000000s
clock_res [CLOCK_MONOTONIC_RAW] = 0.000000001s
clock_res [CLOCK_BOOTTIME] = 0.000000001s
clock_res [CLOCK_PROCESS_CPUTIME_ID] = 0.000000001s
clock_res [CLOCK_THREAD_CPUTIME_ID] = 0.000000001s

A number of programs may depend i.e. on CLOCK_REALTIME or CLOCK_BOOTIME to give
different result each nanosecond.

I came across this when generating nonces for HMACs according to recommendations
from RFC 4086 "Randomness Requirements for Security".

If the value of CLOCK_BOOTTIME or CLOCK_REALTIME is incremented not in what
clock_getres() gives, but at best in 1/100th of second instead, that would seriously
weaken our security (for as you know, in many cryptographic uses nonces need not
be random, but MUST NOT ever repeat nor go backwards).

Could we modify the test for this assumption, or is the assumption wrong?

Here the test for CLOCK_PROCESS_CPUTIME_ID and CLOCK_THREAD_CPUTIME_ID
increasing monotonically with guaranteed increased value of nanoseconds
would also seem good.

Maybe this is already covered in another test case, but it seems that all
clocks should be guaranteed to be monotonically increasing, and increased
at least by one nanosecond with each syscall, or many algorithms would break.

In other words, CLOCK_BOOTTIME should be tested to increase monotonically in
the resolution given by clock_getres (CLOCK_BOOTTIME, &tv_res), not in 1/100ths
of second (IMHO).

Am I wrong in my assumption?

Thank you,
Mirsad

-- 
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu

System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia