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

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 +2 -1

index 60145aadfc5e..881e7c459c64 100644
@@ -4692,7 +4692,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, old_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);

@@ -4700,11 +4700,12 @@ static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
 	spin_lock(&memcg->peaks_lock);
 
 	usage = page_counter_read(pc);
+	old_watermark = READ_ONCE(pc->local_watermark);

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 60145aadfc5e..881e7c459c64 100644
@@ -4692,7 +4692,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, old_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);

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

Quoted 46 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 +2 -1

index 60145aadfc5e..881e7c459c64 100644
@@ -4692,7 +4692,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, old_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);

@@ -4700,11 +4700,12 @@ static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
  	spin_lock(&memcg->peaks_lock);
 
  	usage = page_counter_read(pc);
+	old_watermark = READ_ONCE(pc->local_watermark);
  	WRITE_ONCE(pc->local_watermark, usage);
 

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

Quoted 5 lines

mm/memcontrol.c +2 -2

  	list_for_each_entry(peer_ctx, watchers, list)
-		if (usage > peer_ctx->value)
-			WRITE_ONCE(peer_ctx->value, usage);
+		if (peer_ctx != ofp && old_watermark > peer_ctx->value)
+			WRITE_ONCE(peer_ctx->value, old_watermark);

Ah, because B was previously reporting the higher local_watermark, and its peer_ctx->value was actually low. Fixing it to current usage is wrong in that case. It must remember local_watermark.

What about if usage is bigger than old_watermark? Then we don't update the peer_ctx just yet. local_watermark is updated and propagated into the peers on the next reset. I guess it's correct, but it's kind of tricky to follow.

Would it be easier to understand if we mirrored the max() from peak_show() here?

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

	peer_watermark = max(usage, local_watermark);

	list_for_each_entry(...)
		if (peer_ctx != ofp && peer_watermark > peer_ctx->value)
			WRITE_ONCE(peer_ctx->value, peer_watermark);

This code hurts my head. No strong feelings either way ;)

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Lockless readers in peak_show can transiently observe a lost peak due to an incorrect multi-variable update order and missing memory barriers in peak_write. --

--- Patch [1]: [PATCH] mm, memcg: fix memory.peak reset clobbering other fds' watermark --- commit 15d8c2c42e3dd5de2e4db8178cc6e094af81f2fe
Author: Ridong Chen <[email protected]>

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

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.

Quoted 15 lines

mm/memcontrol.c +1 -1

index 8ff285e872ca9..7d7b6bf200805 100644
@@ -4740,7 +4740,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, old_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);

@@ -4748,11 +4748,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 4756

Quoted 10 lines

mm/memcontrol.c +3 -2

 
  	usage = page_counter_read(pc);
+	old_watermark = 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 && old_watermark > peer_ctx->value)
+			WRITE_ONCE(peer_ctx->value, old_watermark);

[Severity: Medium]
Will this update order create a race window where readers can transiently observe a lower peak?

In mm/memcontrol.c:peak_show(), readers locklessly check these values:

	u64 fd_peak = READ_ONCE(ofp->value), peak;

	/* User wants global or local peak? */
	if (fd_peak == OFP_PEAK_UNSET)
		peak = pc->watermark;
	else
		peak = max(fd_peak, READ_ONCE(pc->local_watermark));

If pc->local_watermark is lowered to usage here in peak_write() before the watchers' peer_ctx->value fields are updated to old_watermark, can a concurrent lockless reader in peak_show() interleave to observe the lowered local_watermark while still reading their old lower fd_peak value?

If they observe both lower values, this would temporarily violate the monotonic property of the peak metric and return a lower peak than expected.

Should the watchers be updated with old_watermark before lowering pc->local_watermark to usage, or are memory barriers needed to ensure the correct order?

Quoted 3 lines

mm/memcontrol.c +0 -0

  	/* initial write, register watcher */
  	if (ofp->value == OFP_PEAK_UNSET)