A data-race issue in memcg rstat occurs when two distinct code paths
access the same 4-byte region concurrently. KCSAN detection triggers the
following BUG as a result.
BUG: KCSAN: data-race in __count_memcg_events / mem_cgroup_css_rstat_flush
write to 0xffffe8ffff98e300 of 4 bytes by task 5274 on cpu 17:
mem_cgroup_css_rstat_flush (mm/memcontrol.c:5850)
cgroup_rstat_flush_locked (kernel/cgroup/rstat.c:243 (discriminator 7))
cgroup_rstat_flush (./include/linux/spinlock.h:401 kernel/cgroup/rstat.c:278)
mem_cgroup_flush_stats.part.0 (mm/memcontrol.c:767)
memory_numa_stat_show (mm/memcontrol.c:6911)
<snip>
read to 0xffffe8ffff98e300 of 4 bytes by task 410848 on cpu 27:
__count_memcg_events (mm/memcontrol.c:725 mm/memcontrol.c:962)
count_memcg_event_mm.part.0 (./include/linux/memcontrol.h:1097 ./include/linux/memcontrol.h:1120)
handle_mm_fault (mm/memory.c:5483 mm/memory.c:5622)
<snip>
value changed: 0x00000029 -> 0x00000000
The race occurs because two code paths access the same "stats_updates"
location. Although "stats_updates" is a per-CPU variable, it is remotely
accessed by another CPU at
cgroup_rstat_flush_locked()->mem_cgroup_css_rstat_flush(), leading to
the data race mentioned.
Considering that memcg_rstat_updated() is in the hot code path, adding
a lock to protect it may not be desirable, especially since this
variable pertains solely to statistics.
Therefore, annotating accesses to stats_updates with READ/WRITE_ONCE()
can prevent KCSAN splats and potential partial reads/writes.
Suggested-by: Shakeel Butt <[email protected]>
Signed-off-by: Breno Leitao <[email protected]>
---
mm/memcontrol.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index fabce2b50c69..3c99457b36a1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -715,6 +715,7 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
{
struct memcg_vmstats_percpu *statc;
int cpu = smp_processor_id();
+ unsigned int stats_updates;
if (!val)
return;
@@ -722,8 +723,9 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
cgroup_rstat_updated(memcg->css.cgroup, cpu);
statc = this_cpu_ptr(memcg->vmstats_percpu);
for (; statc; statc = statc->parent) {
- statc->stats_updates += abs(val);
- if (statc->stats_updates < MEMCG_CHARGE_BATCH)
+ stats_updates = READ_ONCE(statc->stats_updates) + abs(val);
+ WRITE_ONCE(statc->stats_updates, stats_updates);
+ if (stats_updates < MEMCG_CHARGE_BATCH)
continue;
/*
@@ -731,9 +733,9 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
* redundant. Avoid the overhead of the atomic update.
*/
if (!memcg_vmstats_needs_flush(statc->vmstats))
- atomic64_add(statc->stats_updates,
+ atomic64_add(stats_updates,
&statc->vmstats->stats_updates);
- statc->stats_updates = 0;
+ WRITE_ONCE(statc->stats_updates, 0);
}
}
@@ -5845,7 +5847,7 @@ static void mem_cgroup_css_rstat_flush(struct cgroup_subsys_state *css, int cpu)
}
}
}
- statc->stats_updates = 0;
+ WRITE_ONCE(statc->stats_updates, 0);
/* We are in a per-cpu loop here, only do the atomic write once */
if (atomic64_read(&memcg->vmstats->stats_updates))
atomic64_set(&memcg->vmstats->stats_updates, 0);
--
2.43.0
On Wed, Apr 24, 2024 at 05:59:39AM -0700, Breno Leitao wrote:
> A data-race issue in memcg rstat occurs when two distinct code paths
> access the same 4-byte region concurrently. KCSAN detection triggers the
> following BUG as a result.
>
> BUG: KCSAN: data-race in __count_memcg_events / mem_cgroup_css_rstat_flush
>
> write to 0xffffe8ffff98e300 of 4 bytes by task 5274 on cpu 17:
> mem_cgroup_css_rstat_flush (mm/memcontrol.c:5850)
> cgroup_rstat_flush_locked (kernel/cgroup/rstat.c:243 (discriminator 7))
> cgroup_rstat_flush (./include/linux/spinlock.h:401 kernel/cgroup/rstat.c:278)
> mem_cgroup_flush_stats.part.0 (mm/memcontrol.c:767)
> memory_numa_stat_show (mm/memcontrol.c:6911)
> <snip>
>
> read to 0xffffe8ffff98e300 of 4 bytes by task 410848 on cpu 27:
> __count_memcg_events (mm/memcontrol.c:725 mm/memcontrol.c:962)
> count_memcg_event_mm.part.0 (./include/linux/memcontrol.h:1097 ./include/linux/memcontrol.h:1120)
> handle_mm_fault (mm/memory.c:5483 mm/memory.c:5622)
> <snip>
>
> value changed: 0x00000029 -> 0x00000000
>
> The race occurs because two code paths access the same "stats_updates"
> location. Although "stats_updates" is a per-CPU variable, it is remotely
> accessed by another CPU at
> cgroup_rstat_flush_locked()->mem_cgroup_css_rstat_flush(), leading to
> the data race mentioned.
>
> Considering that memcg_rstat_updated() is in the hot code path, adding
> a lock to protect it may not be desirable, especially since this
> variable pertains solely to statistics.
>
> Therefore, annotating accesses to stats_updates with READ/WRITE_ONCE()
> can prevent KCSAN splats and potential partial reads/writes.
>
> Suggested-by: Shakeel Butt <[email protected]>
> Signed-off-by: Breno Leitao <[email protected]>
Acked-by: Johannes Weiner <[email protected]>
On Wed, Apr 24, 2024 at 05:59:39AM -0700, Breno Leitao wrote:
> A data-race issue in memcg rstat occurs when two distinct code paths
> access the same 4-byte region concurrently. KCSAN detection triggers the
> following BUG as a result.
>
> BUG: KCSAN: data-race in __count_memcg_events / mem_cgroup_css_rstat_flush
>
> write to 0xffffe8ffff98e300 of 4 bytes by task 5274 on cpu 17:
> mem_cgroup_css_rstat_flush (mm/memcontrol.c:5850)
> cgroup_rstat_flush_locked (kernel/cgroup/rstat.c:243 (discriminator 7))
> cgroup_rstat_flush (./include/linux/spinlock.h:401 kernel/cgroup/rstat.c:278)
> mem_cgroup_flush_stats.part.0 (mm/memcontrol.c:767)
> memory_numa_stat_show (mm/memcontrol.c:6911)
> <snip>
>
> read to 0xffffe8ffff98e300 of 4 bytes by task 410848 on cpu 27:
> __count_memcg_events (mm/memcontrol.c:725 mm/memcontrol.c:962)
> count_memcg_event_mm.part.0 (./include/linux/memcontrol.h:1097 ./include/linux/memcontrol.h:1120)
> handle_mm_fault (mm/memory.c:5483 mm/memory.c:5622)
> <snip>
>
> value changed: 0x00000029 -> 0x00000000
>
> The race occurs because two code paths access the same "stats_updates"
> location. Although "stats_updates" is a per-CPU variable, it is remotely
> accessed by another CPU at
> cgroup_rstat_flush_locked()->mem_cgroup_css_rstat_flush(), leading to
> the data race mentioned.
>
> Considering that memcg_rstat_updated() is in the hot code path, adding
> a lock to protect it may not be desirable, especially since this
> variable pertains solely to statistics.
>
> Therefore, annotating accesses to stats_updates with READ/WRITE_ONCE()
> can prevent KCSAN splats and potential partial reads/writes.
>
> Suggested-by: Shakeel Butt <[email protected]>
> Signed-off-by: Breno Leitao <[email protected]>
Acked-by: Shakeel Butt <[email protected]>
On Wed, Apr 24, 2024 at 6:00 AM Breno Leitao <[email protected]> wrote:
>
> A data-race issue in memcg rstat occurs when two distinct code paths
> access the same 4-byte region concurrently. KCSAN detection triggers the
> following BUG as a result.
>
> BUG: KCSAN: data-race in __count_memcg_events / mem_cgroup_css_rstat_flush
>
> write to 0xffffe8ffff98e300 of 4 bytes by task 5274 on cpu 17:
> mem_cgroup_css_rstat_flush (mm/memcontrol.c:5850)
> cgroup_rstat_flush_locked (kernel/cgroup/rstat.c:243 (discriminator 7))
> cgroup_rstat_flush (./include/linux/spinlock.h:401 kernel/cgroup/rstat.c:278)
> mem_cgroup_flush_stats.part.0 (mm/memcontrol.c:767)
> memory_numa_stat_show (mm/memcontrol.c:6911)
> <snip>
>
> read to 0xffffe8ffff98e300 of 4 bytes by task 410848 on cpu 27:
> __count_memcg_events (mm/memcontrol.c:725 mm/memcontrol.c:962)
> count_memcg_event_mm.part.0 (./include/linux/memcontrol.h:1097 ./include/linux/memcontrol.h:1120)
> handle_mm_fault (mm/memory.c:5483 mm/memory.c:5622)
> <snip>
>
> value changed: 0x00000029 -> 0x00000000
>
> The race occurs because two code paths access the same "stats_updates"
> location. Although "stats_updates" is a per-CPU variable, it is remotely
> accessed by another CPU at
> cgroup_rstat_flush_locked()->mem_cgroup_css_rstat_flush(), leading to
> the data race mentioned.
>
> Considering that memcg_rstat_updated() is in the hot code path, adding
> a lock to protect it may not be desirable, especially since this
> variable pertains solely to statistics.
>
> Therefore, annotating accesses to stats_updates with READ/WRITE_ONCE()
> can prevent KCSAN splats and potential partial reads/writes.
>
> Suggested-by: Shakeel Butt <[email protected]>
> Signed-off-by: Breno Leitao <[email protected]>
Reviewed-by: Yosry Ahmed <[email protected]>
, and or posterity:
Fixes: 9cee7e8ef3e3 ("mm: memcg: optimize parent iteration in
memcg_rstat_updated()")
On Wed 24-04-24 05:59:39, Breno Leitao wrote:
> A data-race issue in memcg rstat occurs when two distinct code paths
> access the same 4-byte region concurrently. KCSAN detection triggers the
> following BUG as a result.
>
> BUG: KCSAN: data-race in __count_memcg_events / mem_cgroup_css_rstat_flush
>
> write to 0xffffe8ffff98e300 of 4 bytes by task 5274 on cpu 17:
> mem_cgroup_css_rstat_flush (mm/memcontrol.c:5850)
> cgroup_rstat_flush_locked (kernel/cgroup/rstat.c:243 (discriminator 7))
> cgroup_rstat_flush (./include/linux/spinlock.h:401 kernel/cgroup/rstat.c:278)
> mem_cgroup_flush_stats.part.0 (mm/memcontrol.c:767)
> memory_numa_stat_show (mm/memcontrol.c:6911)
> <snip>
>
> read to 0xffffe8ffff98e300 of 4 bytes by task 410848 on cpu 27:
> __count_memcg_events (mm/memcontrol.c:725 mm/memcontrol.c:962)
> count_memcg_event_mm.part.0 (./include/linux/memcontrol.h:1097 ./include/linux/memcontrol.h:1120)
> handle_mm_fault (mm/memory.c:5483 mm/memory.c:5622)
> <snip>
>
> value changed: 0x00000029 -> 0x00000000
>
> The race occurs because two code paths access the same "stats_updates"
> location. Although "stats_updates" is a per-CPU variable, it is remotely
> accessed by another CPU at
> cgroup_rstat_flush_locked()->mem_cgroup_css_rstat_flush(), leading to
> the data race mentioned.
>
> Considering that memcg_rstat_updated() is in the hot code path, adding
> a lock to protect it may not be desirable, especially since this
> variable pertains solely to statistics.
>
> Therefore, annotating accesses to stats_updates with READ/WRITE_ONCE()
> can prevent KCSAN splats and potential partial reads/writes.
>
> Suggested-by: Shakeel Butt <[email protected]>
> Signed-off-by: Breno Leitao <[email protected]>
Acked-by: Michal Hocko <[email protected]>
It is worth mentioning that the race is harmless.
Thanks!
> ---
> mm/memcontrol.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index fabce2b50c69..3c99457b36a1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -715,6 +715,7 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
> {
> struct memcg_vmstats_percpu *statc;
> int cpu = smp_processor_id();
> + unsigned int stats_updates;
>
> if (!val)
> return;
> @@ -722,8 +723,9 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
> cgroup_rstat_updated(memcg->css.cgroup, cpu);
> statc = this_cpu_ptr(memcg->vmstats_percpu);
> for (; statc; statc = statc->parent) {
> - statc->stats_updates += abs(val);
> - if (statc->stats_updates < MEMCG_CHARGE_BATCH)
> + stats_updates = READ_ONCE(statc->stats_updates) + abs(val);
> + WRITE_ONCE(statc->stats_updates, stats_updates);
> + if (stats_updates < MEMCG_CHARGE_BATCH)
> continue;
>
> /*
> @@ -731,9 +733,9 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
> * redundant. Avoid the overhead of the atomic update.
> */
> if (!memcg_vmstats_needs_flush(statc->vmstats))
> - atomic64_add(statc->stats_updates,
> + atomic64_add(stats_updates,
> &statc->vmstats->stats_updates);
> - statc->stats_updates = 0;
> + WRITE_ONCE(statc->stats_updates, 0);
> }
> }
>
> @@ -5845,7 +5847,7 @@ static void mem_cgroup_css_rstat_flush(struct cgroup_subsys_state *css, int cpu)
> }
> }
> }
> - statc->stats_updates = 0;
> + WRITE_ONCE(statc->stats_updates, 0);
> /* We are in a per-cpu loop here, only do the atomic write once */
> if (atomic64_read(&memcg->vmstats->stats_updates))
> atomic64_set(&memcg->vmstats->stats_updates, 0);
> --
> 2.43.0
>
--
Michal Hocko
SUSE Labs
Hello Michal,
On Fri, May 03, 2024 at 01:27:41PM +0200, Michal Hocko wrote:
> On Wed 24-04-24 05:59:39, Breno Leitao wrote:
> > The race occurs because two code paths access the same "stats_updates"
> > location. Although "stats_updates" is a per-CPU variable, it is remotely
> > accessed by another CPU at
> > cgroup_rstat_flush_locked()->mem_cgroup_css_rstat_flush(), leading to
> > the data race mentioned.
>
> It is worth mentioning that the race is harmless.
Are you suggesting that the race consistently avoids producing corrupt
data, or even if corruption occurs, it's inconsequential because it only
affects statistics?
If there's no data corruption, does it incur any performance drawbacks?
Thanks!