mm, memcg: fix memory.peak reset clobbering other fds' watermark

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] peak_write() destroys transient peaks for all other watchers by propagating the current usage instead of the old local_watermark. --

--- Patch [1]: [PATCH v2 1/2] memcg: acquire peaks_lock when reading memory.peak --- commit 7238a507204c3e18dae7c130c8f9f1da91b169b0
Author: Ridong Chen <[email protected]>

memcg: acquire peaks_lock when reading memory.peak

The commit fixes a race condition where a reader of memory.peak could transiently observe a lower peak. It adds peaks_lock acquisition in memory_peak_show and swap_peak_show to ensure readers see a consistent snapshot of the peak metrics.

Link https://sashiko.dev/#/patchset/[email protected]?part=1

Quoted 4 lines

mm/memcontrol.c +0 -0

index 6061c0ba923bf..15c65249849e3 100644

[ ... ]

Quoted 5 lines

mm/memcontrol.c +1 -0

@@ -5806,8 +5811,13 @@ static u64 swap_current_read(struct cgroup_subsys_state *css,
  static int swap_peak_show(struct seq_file *sf, void *v)
  {
  	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(sf));
+	int ret;

mm/memcontrol.c static u64 swap_current_read(struct cgroup_subsys_state *css, · line 5823

Quoted 10 lines

mm/memcontrol.c +5 -1

 
-	return peak_show(sf, v, &memcg->swap);
+	spin_lock(&memcg->peaks_lock);
+	ret = peak_show(sf, v, &memcg->swap);
+	spin_unlock(&memcg->peaks_lock);
+
+	return ret;
  }
 
  static ssize_t swap_peak_write(struct kernfs_open_file *of, char *buf,

[Severity: High]
This is a pre-existing issue, but does peak_write() destroy transient peaks for other watchers by propagating the current usage instead of the old local_watermark?

Looking at peak_write() in mm/memcontrol.c:

	usage = page_counter_read(pc);
	WRITE_ONCE(pc->local_watermark, usage);

	list_for_each_entry(peer_ctx, watchers, list)
		if (usage > peer_ctx->value)
			WRITE_ONCE(peer_ctx->value, usage);

If watcher A resets its peak when usage has dropped, peak_write() uses the current lower usage. If watcher B was tracking a previous peak, it seems watcher B's value is updated to the current usage rather than the previous peak (the old local_watermark).

For example, if watcher B resets, and then usage spikes to 100 and drops to 50, when watcher A resets, peak_write() will see usage is 50. Watcher B's value would be updated to 50 instead of 100, and the previous peak of 100 would be lost.

Can this cause workload schedulers reading the peak to receive an artificially low value, leading them to over-pack workloads?

From: Ridong Chen <[email protected]>

Sashiko reported that a reader can transiently observe a lower peak within a race window [1]. peak_show() returns max(local_watermark, ofp->value), but peak_write() updates those two under peaks_lock while the reader takes no lock. The interleaving is:

writer (reset on fd A) reader (fd B)
---------------------- -------------
usage = page_counter_read(pc)
WRITE_ONCE(local_watermark, usage)
// watermark lowered to usage

                                         lw  = READ_ONCE(local_watermark)
                                         // sees the lowered usage
                                         val = READ_ONCE(ofp->value)
                                         // B's value not updated yet
                                         return max(lw, val)
                                         // both low -> low peak

WRITE_ONCE(peer_ctx->value, usage)
// B updated, but too late

Fix it by acquiring peaks_lock when reading the peak, so the reader sees a consistent snapshot of local_watermark and the per-fd values. The same race applies to memory.swap.peak, which shares peaks_lock and the peak_write() path, so take the lock there as well.

[1] https://sashiko.dev/#/patchset/[email protected]?part=1

Fixes c6f53ed8f213 ("mm, memcg: cg2 memory{.swap,}.peak write handlers")

Assisted-by Claude:claude-opus-4-8

Signed-off-by Ridong Chen <[email protected]>

From: Ridong Chen <[email protected]>

Sashiko reported that a reader can transiently observe a lower peak within a race window [1]. peak_show() returns max(local_watermark, ofp->value), but peak_write() updates those two under peaks_lock while the reader takes no lock. The interleaving is:

writer (reset on fd A) reader (fd B)
---------------------- -------------
usage = page_counter_read(pc)
WRITE_ONCE(local_watermark, usage)
// watermark lowered to usage

                                         lw  = READ_ONCE(local_watermark)
                                         // sees the lowered usage
                                         val = READ_ONCE(ofp->value)
                                         // B's value not updated yet
                                         return max(lw, val)
                                         // both low -> low peak

WRITE_ONCE(peer_ctx->value, usage)
// B updated, but too late

Fix it by acquiring peaks_lock when reading the peak, so the reader sees a consistent snapshot of local_watermark and the per-fd values. The same race applies to memory.swap.peak, which shares peaks_lock and the peak_write() path, so take the lock there as well.

[1] https://sashiko.dev/#/patchset/[email protected]?part=1

Fixes c6f53ed8f213 ("mm, memcg: cg2 memory{.swap,}.peak write handlers")

Assisted-by Claude:claude-opus-4-8

Signed-off-by Ridong Chen <[email protected]>

From: Ridong Chen <[email protected]>

Writing to memory.peak resets the peak for that fd only. Each fd is a watcher and reads back max(its own value, the shared local_watermark).

peak_write() resets by lowering local_watermark to the current usage. To keep the other watchers' peaks it then walks the watcher list, but it stores the current usage into them instead of the old watermark. So once usage has dropped from a peak, a reset on one fd wrongly drags every other fd's peak down too, even fds that never reset.

Reproduced on 7.2.0-rc5-next under QEMU, two fds A and B on one cgroup:
B sees the peak (410624 KB), usage drops, then A resets -- and B's peak collapses to 1060 KB although B never reset. With this patch B keeps reading 410624 KB.

Fix: save the old watermark before lowering it and use that to floor the other watchers, so a reset only affects the fd that issued it.

Fixes c6f53ed8f213 ("mm, memcg: cg2 memory{.swap,}.peak write handlers")

Assisted-by Claude:claude-opus-4-8

Signed-off-by Ridong Chen <[email protected]>

---
 mm/memcontrol.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

mm/memcontrol.c +1 -1

index 2da55b778ae3..28577beeb3d0 100644
@@ -4746,7 +4746,7 @@ static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
 			  loff_t off, struct page_counter *pc,
 			  struct list_head *watchers)
 {
-	unsigned long usage;
+	unsigned long usage, peer_watermark;
 	struct cgroup_of_peak *peer_ctx;
 	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
 	struct cgroup_of_peak *ofp = of_peak(of);

@@ -4754,11 +4754,12 @@ static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
 	spin_lock(&memcg->peaks_lock);

Quoted 41 lines · Ridong

From: Ridong Chen <[email protected]>

Writing to memory.peak resets the peak for that fd only. Each fd is a watcher and reads back max(its own value, the shared local_watermark).

peak_write() resets by lowering local_watermark to the current usage. To keep the other watchers' peaks it then walks the watcher list, but it stores the current usage into them instead of the old watermark. So once usage has dropped from a peak, a reset on one fd wrongly drags every other fd's peak down too, even fds that never reset.

Reproduced on 7.2.0-rc5-next under QEMU, two fds A and B on one cgroup:
B sees the peak (410624 KB), usage drops, then A resets -- and B's peak collapses to 1060 KB although B never reset. With this patch B keeps reading 410624 KB.

Fix: save the old watermark before lowering it and use that to floor the other watchers, so a reset only affects the fd that issued it.

Fixes c6f53ed8f213 ("mm, memcg: cg2 memory{.swap,}.peak write handlers")

Assisted-by Claude:claude-opus-4-8

Signed-off-by Ridong Chen <[email protected]>

---
  mm/memcontrol.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

mm/memcontrol.c +1 -1

index 2da55b778ae3..28577beeb3d0 100644
@@ -4746,7 +4746,7 @@ static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
  			  loff_t off, struct page_counter *pc,
  			  struct list_head *watchers)
  {
-	unsigned long usage;
+	unsigned long usage, peer_watermark;
  	struct cgroup_of_peak *peer_ctx;
  	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
  	struct cgroup_of_peak *ofp = of_peak(of);

@@ -4754,11 +4754,12 @@ static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
  	spin_lock(&memcg->peaks_lock);

mm/memcontrol.c static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size_t nbytes, · line 4762

Quoted 10 lines

mm/memcontrol.c +3 -2

 
  	usage = page_counter_read(pc);
+	peer_watermark = max(usage, READ_ONCE(pc->local_watermark));
  	WRITE_ONCE(pc->local_watermark, usage);
 
  	list_for_each_entry(peer_ctx, watchers, list)
-		if (usage > peer_ctx->value)
-			WRITE_ONCE(peer_ctx->value, usage);
+		if (peer_ctx != ofp && peer_watermark > peer_ctx->value)
+			WRITE_ONCE(peer_ctx->value, peer_watermark);

Sorry for letting your previous reply sit unanswered. You made a good point on the peer_watermark = max(usage, local_watermark) being pointless because that's how local_watermark moves to begin with.

So what you had before was indeed better. It was just me missing that detail. Could you please go back to your original? Feel free to include:

Acked-by Johannes Weiner <[email protected]>

Two fixes for the per-fd memory.peak / memory.swap.peak handlers added in c6f53ed8f213. Each open fd reads back max(its own value, the shared local_watermark), and both bugs live in that scheme.

Quoted 3 lines · On Fri, 7 Aug 2026 16:59:58 +0800 Ridong <[email protected]> wrote:

Two fixes for the per-fd memory.peak / memory.swap.peak handlers added in c6f53ed8f213. Each open fd reads back max(its own value, the shared local_watermark), and both bugs live in that scheme.

Thanks.

We're missing the preferred description of the worst-case userspace-visible effects of the bug. It appears they're quite minor so I'll assume this series is a post-7.2 thing.

Sashiko might have found a similar race on the writer side:

	https://sashiko.dev/#/patchset/[email protected]

Quoted 6 lines · Andrew Morton

Quoted 3 lines · On Fri,  7 Aug 2026 16:59:58 +0800 Ridong <[email protected]> wrote:

Two fixes for the per-fd memory.peak / memory.swap.peak handlers added in c6f53ed8f213. Each open fd reads back max(its own value, the shared local_watermark), and both bugs live in that scheme.

Quoted 6 lines

Thanks.

We're missing the preferred description of the worst-case userspace-visible effects of the bug.  It appears they're quite minor so I'll assume this series is a post-7.2 thing.

Agreed, post-7.2 is fine and neither is stable material.

Worst case for both: a reader of memory.peak (or memory.swap.peak) gets a value lower than the true peak, so a tool that sizes or bills a cgroup by peak usage under-reports it.

- Patch 1 (reader side, take peaks_lock in peak_show): a reader that races an unrelated reset briefly gets the low value. Transient.

- Patch 2 (writer side, peak_write reset): a reset on one fd stores the current usage into the other watchers instead of the old watermark, dragging every other fd's peak down.

I'll spell this out in both changelogs in v2.

Quoted 2 lines

Sashiko might have found a similar race on the writer side:
    https://sashiko.dev/#/patchset/[email protected]

Yes, that's the writer side that patch 2 fixes;