clock_gettime(CLOCK_THREAD_CPUTIME_ID) was not monotonic due to race

[firk_cantconnect.ru]

Summary

There is a race between switchtime/runtime reading in kern_thread_cputime() and switchtime/runtime updating in statclock(). Critical section does not prevent statclock and so not enough, replaced it with PROC_STAT spinlock. Bug introduced around 12.1 when runtime updating added to statclock.

PR: 262273

Test Plan

#include <stdio.h>
#include <time.h>

int main(void)
{
        struct timespec before, after;

        while (1) {
                clock_gettime(CLOCK_THREAD_CPUTIME_ID, &before);
                clock_gettime(CLOCK_THREAD_CPUTIME_ID, &after);

                printf("before: %lu:%lu after %lu:%lu\n",
                        before.tv_sec, before.tv_nsec,
                        after.tv_sec, after.tv_nsec);

                if (after.tv_sec < before.tv_sec ||
                        after.tv_sec==before.tv_sec && after.tv_nsec<before.tv_nsec) return -1;
        }
        return 0;
}

This program (better to run with | tail) failing on something like this:

before: 0:18195000 after 0:18198000
before: 0:26005000 after 0:18258000

[markj]

head/sys/kern/kern_time.c

266

If the intent is only to disable interrupts to avoid a race with statclock(), then a spinlock_enter()/_exit() section replacing the critical_enter()/_exit() would be cheaper and clearer.

368

What's the purpose of acquiring the proc lock?

[firk_cantconnect.ru]

head/sys/kern/kern_time.c

368

Everywhere I've seen PROC_STATLOCK's they were wrapped by PROC_LOCK's. It seems this wasn't a rule, just coincidence.

[firk_cantconnect.ru]

simplify things according to replies

[markj]

Looks ok, thanks. I'll try the patch and commit if it fixes the test case. Is "firk <[e-mail]>" the desired author for the git commit?

head/sys/kern/kern_time.c

368

It depends. For CLOCK_PROCESS_CPUTIME_ID, the proc lock is required because kern_process_cputime()->rufetch() will iterate over the set of threads belonging to the process, and this set is synchronized by the proc lock. It may serve a different purpose elsewhere.

[firk_cantconnect.ru]

Is "firk <[e-mail]>" the desired author for the git commit?

Yes.

[markj]

Committed in https://reviews.freebsd.org/R10:28d08dc7d051a4e058cc0004cf4dd884f87037a2 , the review link is wrong, sorry.