| Message ID | 1582319350-23515-1-git-send-email-cai@lca.pw (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [-next] power/qos: fix a data race in pm_qos_*_value | expand |
On Fri, Feb 21, 2020 at 10:09 PM Qian Cai <cai@lca.pw> wrote: > > cpu_latency_constraints.target_value could be accessed concurrently as > noticed by KCSAN, Yes, they could, pretty much by design. So why *exactly* is this a problem? > LTP: starting ppoll01 > BUG: KCSAN: data-race in cpu_latency_qos_limit / pm_qos_update_target It may be a bug under certain conditions, but you don't mention what conditions they are. Reporting it as a general bug is not accurate at the very least. > write to 0xffffffff99081470 of 4 bytes by task 27532 on cpu 2: > pm_qos_update_target+0xa4/0x370 > pm_qos_set_value at kernel/power/qos.c:78 > cpu_latency_qos_apply+0x3b/0x50 > cpu_latency_qos_remove_request+0xea/0x270 > cpu_latency_qos_release+0x4b/0x70 > __fput+0x187/0x3d0 > ____fput+0x1e/0x30 > task_work_run+0xbf/0x130 > do_exit+0xa78/0xfd0 > do_group_exit+0x8b/0x180 > __x64_sys_exit_group+0x2e/0x30 > do_syscall_64+0x91/0xb05 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > read to 0xffffffff99081470 of 4 bytes by task 0 on cpu 41: > cpu_latency_qos_limit+0x1f/0x30 > pm_qos_read_value at kernel/power/qos.c:55 > cpuidle_governor_latency_req+0x4f/0x80 > cpuidle_governor_latency_req at drivers/cpuidle/governor.c:114 > menu_select+0x6b/0xc29 > cpuidle_select+0x50/0x70 > do_idle+0x214/0x280 > cpu_startup_entry+0x1d/0x1f > start_secondary+0x1b2/0x230 > secondary_startup_64+0xb6/0xc0 > > Reported by Kernel Concurrency Sanitizer on: > CPU: 41 PID: 0 Comm: swapper/41 Tainted: G L 5.6.0-rc2-next-20200221+ #7 > Hardware name: HPE ProLiant DL385 Gen10/ProLiant DL385 Gen10, BIOS A40 07/10/2019 > > The read is outside pm_qos_lock critical section which results in a data > race. This is purely theoretical AFAICS and so it should be presented this way. Also the call traces above don't add much value to the changelog, so maybe try to explain what the problem is in English. > Fix it by adding a pair of READ|WRITE_ONCE(). > > Signed-off-by: Qian Cai <cai@lca.pw> > --- > kernel/power/qos.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/power/qos.c b/kernel/power/qos.c > index 32927682bcc4..db0bed2cae26 100644 > --- a/kernel/power/qos.c > +++ b/kernel/power/qos.c > @@ -52,7 +52,7 @@ > */ > s32 pm_qos_read_value(struct pm_qos_constraints *c) > { > - return c->target_value; > + return READ_ONCE(c->target_value); > } > > static int pm_qos_get_value(struct pm_qos_constraints *c) > @@ -75,7 +75,7 @@ static int pm_qos_get_value(struct pm_qos_constraints *c) > > static void pm_qos_set_value(struct pm_qos_constraints *c, s32 value) > { > - c->target_value = value; > + WRITE_ONCE(c->target_value, value); > } > > /** > -- > 1.8.3.1 >
> On Feb 23, 2020, at 7:12 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > > It may be a bug under certain conditions, but you don't mention what > conditions they are. Reporting it as a general bug is not accurate at > the very least. Could we rule out load tearing, store tearing and reload of global_req in cpuidle_governor_latency() for all compilers and architectures which could introduce logic bugs? int global_req = cpu_latency_qos_limit(); if (device_req > global_req) device_req = global_req; If under register pressure, the compiler might get ride of the tmp variable, i.e., If (device_req > cpu_latency_qos_limit()) —-> race with the writer. device_req = cpu_latency_qos_limit();
On Mon, Feb 24, 2020 at 2:01 AM Qian Cai <cai@lca.pw> wrote: > > > > > On Feb 23, 2020, at 7:12 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > It may be a bug under certain conditions, but you don't mention what > > conditions they are. Reporting it as a general bug is not accurate at > > the very least. > > Could we rule out load tearing, store tearing and reload of global_req in cpuidle_governor_latency() for all compilers and architectures which could introduce logic bugs? > > int global_req = cpu_latency_qos_limit(); > > if (device_req > global_req) > device_req = global_req; > > If under register pressure, the compiler might get ride of the tmp variable, i.e., > > If (device_req > cpu_latency_qos_limit()) > —-> race with the writer. > device_req = cpu_latency_qos_limit(); Yes, there is a race here with or without the WRITE_ONCE()/READ_ONCE() annotations (note that these annotations don't prevent CPUs from reordering things, so device_req may be set before global_req regardless). However, worst-case it may cause an old value to be used and that can happen anyway if the entire cpuidle_governor_latency_req() runs between the curr_value update and pm_qos_set_value() in pm_qos_update_target(), for example. IOW, there is no guarantee that the new value will be used immediately after updating a QoS request anyway. I agree with adding the annotations (I was considering posting a patch doing that myself), but just as a matter of making the intention clear.
On Mon, 2020-02-24 at 10:54 +0100, Rafael J. Wysocki wrote: > On Mon, Feb 24, 2020 at 2:01 AM Qian Cai <cai@lca.pw> wrote: > > > > > > > > > On Feb 23, 2020, at 7:12 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > It may be a bug under certain conditions, but you don't mention what > > > conditions they are. Reporting it as a general bug is not accurate at > > > the very least. > > > > Could we rule out load tearing, store tearing and reload of global_req in cpuidle_governor_latency() for all compilers and architectures which could introduce logic bugs? > > > > int global_req = cpu_latency_qos_limit(); > > > > if (device_req > global_req) > > device_req = global_req; > > > > If under register pressure, the compiler might get ride of the tmp variable, i.e., > > > > If (device_req > cpu_latency_qos_limit()) > > —-> race with the writer. > > device_req = cpu_latency_qos_limit(); > > Yes, there is a race here with or without the WRITE_ONCE()/READ_ONCE() > annotations (note that these annotations don't prevent CPUs from > reordering things, so device_req may be set before global_req > regardless). > > However, worst-case it may cause an old value to be used and that can > happen anyway if the entire cpuidle_governor_latency_req() runs > between the curr_value update and pm_qos_set_value() in > pm_qos_update_target(), for example. > > IOW, there is no guarantee that the new value will be used immediately > after updating a QoS request anyway. > > I agree with adding the annotations (I was considering posting a patch > doing that myself), but just as a matter of making the intention > clear. OK, how about this updated texts? [PATCH -next] power/qos: annotate a data race in pm_qos_*_value cpu_latency_constraints.target_value could be accessed concurrently via, cpu_latency_qos_apply pm_qos_update_target pm_qos_set_value cpuidle_governor_latency_req cpu_latency_qos_limit pm_qos_read_value The read is outside pm_qos_lock critical section which results in a data race. However, the worst case is that an old value to be used and that can happen anyway, so annotate this data race using a pair of READ|WRITE_ONCE().
On Mon, Feb 24, 2020 at 8:02 PM Qian Cai <cai@lca.pw> wrote: > > On Mon, 2020-02-24 at 10:54 +0100, Rafael J. Wysocki wrote: > > On Mon, Feb 24, 2020 at 2:01 AM Qian Cai <cai@lca.pw> wrote: > > > > > > > > > > > > > On Feb 23, 2020, at 7:12 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > It may be a bug under certain conditions, but you don't mention what > > > > conditions they are. Reporting it as a general bug is not accurate at > > > > the very least. > > > > > > Could we rule out load tearing, store tearing and reload of global_req in cpuidle_governor_latency() for all compilers and architectures which could introduce logic bugs? > > > > > > int global_req = cpu_latency_qos_limit(); > > > > > > if (device_req > global_req) > > > device_req = global_req; > > > > > > If under register pressure, the compiler might get ride of the tmp variable, i.e., > > > > > > If (device_req > cpu_latency_qos_limit()) > > > —-> race with the writer. > > > device_req = cpu_latency_qos_limit(); > > > > Yes, there is a race here with or without the WRITE_ONCE()/READ_ONCE() > > annotations (note that these annotations don't prevent CPUs from > > reordering things, so device_req may be set before global_req > > regardless). > > > > However, worst-case it may cause an old value to be used and that can > > happen anyway if the entire cpuidle_governor_latency_req() runs > > between the curr_value update and pm_qos_set_value() in > > pm_qos_update_target(), for example. > > > > IOW, there is no guarantee that the new value will be used immediately > > after updating a QoS request anyway. > > > > I agree with adding the annotations (I was considering posting a patch > > doing that myself), but just as a matter of making the intention > > clear. > > OK, how about this updated texts? > > [PATCH -next] power/qos: annotate a data race in pm_qos_*_value > > cpu_latency_constraints.target_value could be accessed concurrently via, > > cpu_latency_qos_apply > pm_qos_update_target > pm_qos_set_value > > cpuidle_governor_latency_req > cpu_latency_qos_limit > pm_qos_read_value > > The read is outside pm_qos_lock critical section which results in a data race. > However, the worst case is that an old value to be used and that can happen > anyway, so annotate this data race using a pair of READ|WRITE_ONCE(). I would rather say something like this: The target_value field in struct pm_qos_constraints is used for lockless access to the effective constraint value of a given QoS list, so the readers of it cannot expect it to always reflect the most recent effective constraint value. However, they can and do expect it to be equal to a valid effective constraint value computed at a certain time in the past (event though it may not be the most recent one), so add READ|WRITE_ONCE() annotations around the target_value accesses to prevent the compiler from possibly causing that expectation to be unmet by generating code in an exceptionally convoluted way.
> On Feb 25, 2020, at 7:10 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > > The target_value field in struct pm_qos_constraints is used for > lockless access to the effective constraint value of a given QoS list, > so the readers of it cannot expect it to always reflect the most > recent effective constraint value. However, they can and do expect it > to be equal to a valid effective constraint value computed at a > certain time in the past (event though it may not be the most recent > one), so add READ|WRITE_ONCE() annotations around the target_value > accesses to prevent the compiler from possibly causing that > expectation to be unmet by generating code in an exceptionally > convoluted way. Perfect. I’ll send a v2 for that unless you would like to squash it in.
diff --git a/kernel/power/qos.c b/kernel/power/qos.c index 32927682bcc4..db0bed2cae26 100644 --- a/kernel/power/qos.c +++ b/kernel/power/qos.c @@ -52,7 +52,7 @@ */ s32 pm_qos_read_value(struct pm_qos_constraints *c) { - return c->target_value; + return READ_ONCE(c->target_value); } static int pm_qos_get_value(struct pm_qos_constraints *c) @@ -75,7 +75,7 @@ static int pm_qos_get_value(struct pm_qos_constraints *c) static void pm_qos_set_value(struct pm_qos_constraints *c, s32 value) { - c->target_value = value; + WRITE_ONCE(c->target_value, value); } /**
cpu_latency_constraints.target_value could be accessed concurrently as noticed by KCSAN, LTP: starting ppoll01 BUG: KCSAN: data-race in cpu_latency_qos_limit / pm_qos_update_target write to 0xffffffff99081470 of 4 bytes by task 27532 on cpu 2: pm_qos_update_target+0xa4/0x370 pm_qos_set_value at kernel/power/qos.c:78 cpu_latency_qos_apply+0x3b/0x50 cpu_latency_qos_remove_request+0xea/0x270 cpu_latency_qos_release+0x4b/0x70 __fput+0x187/0x3d0 ____fput+0x1e/0x30 task_work_run+0xbf/0x130 do_exit+0xa78/0xfd0 do_group_exit+0x8b/0x180 __x64_sys_exit_group+0x2e/0x30 do_syscall_64+0x91/0xb05 entry_SYSCALL_64_after_hwframe+0x49/0xbe read to 0xffffffff99081470 of 4 bytes by task 0 on cpu 41: cpu_latency_qos_limit+0x1f/0x30 pm_qos_read_value at kernel/power/qos.c:55 cpuidle_governor_latency_req+0x4f/0x80 cpuidle_governor_latency_req at drivers/cpuidle/governor.c:114 menu_select+0x6b/0xc29 cpuidle_select+0x50/0x70 do_idle+0x214/0x280 cpu_startup_entry+0x1d/0x1f start_secondary+0x1b2/0x230 secondary_startup_64+0xb6/0xc0 Reported by Kernel Concurrency Sanitizer on: CPU: 41 PID: 0 Comm: swapper/41 Tainted: G L 5.6.0-rc2-next-20200221+ #7 Hardware name: HPE ProLiant DL385 Gen10/ProLiant DL385 Gen10, BIOS A40 07/10/2019 The read is outside pm_qos_lock critical section which results in a data race. Fix it by adding a pair of READ|WRITE_ONCE(). Signed-off-by: Qian Cai <cai@lca.pw> --- kernel/power/qos.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)