Bug 207913

Summary:	Potential data races in `settimeofday`
Product:	Timers	Reporter:	Yanze (dr.funemy)
Component:	Other	Assignee:	john stultz (john.stultz)
Status:	NEW ---
Severity:	normal
Priority:	P1
Hardware:	All
OS:	Linux
Kernel Version:	5.7-rc7	Subsystem:
Regression:	No	Bisected commit-id:
Attachments:	race_report_firsttime race_report_persistent_clock_is_local race_report_vdata

Description Yanze 2020-05-27 02:24:29 UTC

Hello, we are working on a static race detection tool and we tested it on the Linux kernel.

The races we found do not appear to be dangerous to the core functions of the kernel, but we thought it would be better to report it just in case it leads to undefined behavior.

The well-formatted reports are attached.

There are 3 races in total, all potentially happen when the system call `settimeofday` is called concurrently:

1. The race is on `firsttime` in `do_sys_settimeofday64()` in the `kernel/time/time.c`. The read of `firsttime` at line 188 and the write at line 189 are not protected by locks.

2. The race is on `persistent_clock_is_local` in `timekeeping_warp_clock()` in `kernel/time/timekeeping.c`. The write at line 1337 could potentially race with itself, if `settimeofday` is called concurrently

3. The race is on `vdata[CS_HRES_COARSE]` in `update_vsyscall_tz()` in `kernel/time/vsyscall.c`. The write at line 125 and 126 could potentially race with themselves, if `settimeofday` is called concurrently.

Comment 1 Yanze 2020-05-27 02:25:12 UTC

Created attachment 289323 [details]
race_report_firsttime

Comment 2 Yanze 2020-05-27 02:25:47 UTC

Created attachment 289325 [details]
race_report_persistent_clock_is_local

Comment 3 Yanze 2020-05-27 02:26:05 UTC

Created attachment 289327 [details]
race_report_vdata

Comment 4 Yanze 2020-05-27 02:31:57 UTC

*** Bug 207911 has been marked as a duplicate of this bug. ***

Comment 5 john stultz 2020-05-27 03:25:09 UTC

Yea. The timezone setting logic is pretty old and grungy. Mostly its assuming we're only calling it early in boot so its not likely to race with anything.

A possible solution might be in do_sys_settimeofday64() moving firsttime out to a static global, and adding a separate spinlock (say tz_lock) which is taken on line 185 before we set sys_tz and released after line 192.

I think doing that would resolve all the races in this issue.

Do you want to create and submit such a patch?