[PATCH] mm: memcontrol: serialize concurrent memory.min/low writes

[Song Hu]

page_counter_set_min() and page_counter_set_low() require callers to
serialize updates to the same counter.

The v2 write path does not provide that guarantee: kernfs serializes
only writes through the same struct file, while each echo opens the
file independently, and cgroup_file_write() invokes the handler
directly, so concurrent updates run in parallel.

The store and the subsequent propagation replay are not atomic, and
propagate_protected_usage() recomputes protection accounting from the
current page_counter limits instead of a value captured when the
write started.  Two concurrent updates can therefore interleave such
that the replay associated with an older limit overwrites accounting
computed from the newer one: writer A stores 100, writer B stores 200
and replays, then A's replay runs last and leaves min_usage and the
ancestors' children_min_usage computed from the stale 100 while
counter->min is 200.  The wrong accounting
persists until the next charge or uncharge re-runs the propagation;
an idle group with resident pages never self-heals, and reclaim
meanwhile distributes sibling protection by the wrong numbers.  No
corruption beyond mispriced protection.

Serialize the write handlers with a shared mutex, the same shape the
v1 controller already uses for its limit writes (memcg_max_mutex in
memcontrol-v1.c).  The offline and reset paths, which clear the
limits under cgroup_mutex, take the same mutex, so every memcg
writer of page_counter_set_min()/set_low() ends up serialized.  The
mutex is only ever taken as a leaf in process context, so there is
no ordering hazard.  memory.high and memory.max need no lock:
neither propagates protection, and their setters are already atomic
(a plain store and an xchg respectively).

Fixes: bf8d5d52ffe8 ("memcg: introduce memory.min")
Fixes: 230671533d64 ("mm: memory.low hierarchical behavior")
Signed-off-by: Song Hu <husong@xxxxxxxxxx>
---
 mm/memcontrol.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 84f4a8fd4d16..87ca7bc1aef1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4458,14 +4458,27 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
 	return -ENOMEM;
 }
 
+/*
+ * The store-plus-replay sequences of page_counter_set_min() and
+ * page_counter_set_low() must not interleave across writers: a stale
+ * replay can overwrite a newer one and protection accounting stays
+ * wrong until the next charge or uncharge re-runs it.  The walk in
+ * those helpers crosses counters, so this lock has to stay global --
+ * per-cgroup locking would still let a descendant writer and an
+ * ancestor writer race.
+ */
+static DEFINE_MUTEX(memcg_protected_mutex);
+
 static void mem_cgroup_css_offline(struct cgroup_subsys_state *css)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
 
 	memcg1_css_offline(memcg);
 
+	mutex_lock(&memcg_protected_mutex);
 	page_counter_set_min(&memcg->memory, 0);
 	page_counter_set_low(&memcg->memory, 0);
+	mutex_unlock(&memcg_protected_mutex);
 	page_counter_set_high(&memcg->memory, PAGE_COUNTER_MAX);
 	page_counter_set_high(&memcg->swap, PAGE_COUNTER_MAX);
 
@@ -4548,8 +4561,10 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
 	page_counter_set_max(&memcg->kmem, PAGE_COUNTER_MAX);
 	page_counter_set_max(&memcg->tcpmem, PAGE_COUNTER_MAX);
 #endif
+	mutex_lock(&memcg_protected_mutex);
 	page_counter_set_min(&memcg->memory, 0);
 	page_counter_set_low(&memcg->memory, 0);
+	mutex_unlock(&memcg_protected_mutex);
 	page_counter_set_high(&memcg->memory, PAGE_COUNTER_MAX);
 	page_counter_set_high(&memcg->swap, PAGE_COUNTER_MAX);
 	memcg_wb_domain_size_changed(memcg);
@@ -4924,7 +4939,9 @@ static ssize_t memory_min_write(struct kernfs_open_file *of,
 	if (err)
 		return err;
 
+	mutex_lock(&memcg_protected_mutex);
 	page_counter_set_min(&memcg->memory, min);
+	mutex_unlock(&memcg_protected_mutex);
 
 	return nbytes;
 }
@@ -4947,7 +4964,9 @@ static ssize_t memory_low_write(struct kernfs_open_file *of,
 	if (err)
 		return err;
 
+	mutex_lock(&memcg_protected_mutex);
 	page_counter_set_low(&memcg->memory, low);
+	mutex_unlock(&memcg_protected_mutex);
 
 	return nbytes;
 }
-- 
2.53.0