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

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]>

Acked-by Johannes Weiner <[email protected]>

Acked-by Shakeel Butt <[email protected]>

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

Pre-existing issues:
- [High] The patch fails to address a data loss bug in peak_write() where a reset permanently destroys the historical peak for all other registered FDs. --

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

memcg: acquire peaks_lock when reading memory.peak

This commit fixes a race condition where a reader can transiently observe a lower memory peak. It acquires the peaks_lock in memory_peak_show and swap_peak_show so the reader sees a consistent snapshot of the memory values.

mm/memcontrol.c static int memory_peak_show(struct seq_file *sf, void *v) · line 4718

Quoted 11 lines

mm/memcontrol.c +1 -0

index 1d3339520809f..1546993168610 100644
@@ -4713,6 +4713,7 @@ static int memory_peak_show(struct seq_file *sf, void *v)
  {
  	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(sf));
 
+	guard(spinlock)(&memcg->peaks_lock);
  	return peak_show(sf, v, &memcg->memory);
  }

[Severity: High]
This is a pre-existing issue, but there appears to be a data loss bug in the peak memory tracking API during resets that this patch does not address.

Looking at peak_write(), when one watcher resets the peak, it updates the peer watchers using the current usage instead of the old pc->local_watermark:

mm/memcontrol.c:peak_write() {
     ...
	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);
     ...
}

Does this incorrectly discard the historical memory peak for all other registered watchers when any one watcher resets the peak?

If memory usage spiked and dropped before a watcher calls peak_write(), propagating the current (lower) usage instead of the old pc->local_watermark would cause the other watchers to lose visibility of the spike. This could lead to userspace tools like OOM daemons under-provisioning memory or failing to detect usage spikes.

Could the loop propagate the old pc->local_watermark instead of the current usage to preserve the historical peak?

[ ... ]

mm/memcontrol.c static int swap_peak_show(struct seq_file *sf, void *v) · line 5864

Quoted 7 lines

mm/memcontrol.c +1 -0

@@ -5858,6 +5859,7 @@ static int swap_peak_show(struct seq_file *sf, void *v)
  {
  	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(sf));
 
+	guard(spinlock)(&memcg->peaks_lock);
  	return peak_show(sf, v, &memcg->swap);
  }

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")

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

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

Acked-by Tao Cui <[email protected]>

Acked-by Johannes Weiner <[email protected]>

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

From: Ridong Chen <[email protected]>

The memory.peak / memory.swap.peak per-fd watermark tracking has two issues. Each open fd is a watcher and reads back max(its own value, the shared local_watermark); both bugs live in that scheme.

Worst case for both is the same and is userspace-visible: 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 its peak usage under-reports it.

Patch 1 (read side) fixes the race Sashiko pointed out in the v1 review [1]: peak_show() inspects local_watermark and the per-fd values without holding peaks_lock, so a reader that races an unrelated peak_write() reset briefly observes the lowered value. Transient. It takes peaks_lock in the show path.

Patch 2 (write side) fixes peak_write(): on a reset it stores the current usage into the other watchers instead of the old watermark, so once usage has dropped from a peak a reset on one fd drags every other fd's peak down too, even fds that never reset.

---
Changes since v3:
- Switch to guard(spinlock) in the peak readers, suggested by Muchun.

Changes since v2:
- Spell out the worst-case userspace-visible effect, per Andrew's Go back to v1 [2].

Changes since v1:
- New patch 1: hold peaks_lock in the peak readers (Sashiko).
- Patch 2: floor the peers with max(usage, local_watermark), mirroring peak_show(), and skip the writing fd (Johannes Weiner).

[1] https://sashiko.dev/#/patchset/[email protected]?part=1
[2] https://lore.kernel.org/all/[email protected]/

Ridong Chen (2):
memcg: acquire peaks_lock when reading memory.peak mm, memcg: fix memory.peak reset clobbering other fds' watermark

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

Quoted 7 lines

Ridong Chen (2):
memcg: acquire peaks_lock when reading memory.peak mm, memcg: fix memory.peak reset clobbering other fds' watermark

Quoted 3 lines

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

Hi all,

Just a quick reminder on this series, could it be applied? Sending this note in case it got buried.