[PATCH v2 1/2] memcg: acquire peaks_lock when reading memory.peak

[PATCH v2 1/2] memcg: acquire peaks_lock when reading memory.peak

[Ridong]

From: Ridong Chen <chenridong@xiaomi.com>

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/20260730115314.1069089-1-ridong.chen@linux.dev?part=1
Fixes: c6f53ed8f213 ("mm, memcg: cg2 memory{.swap,}.peak write handlers")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Ridong Chen <chenridong@xiaomi.com>
---
 mm/memcontrol.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index dd6b1c298345..2da55b778ae3 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4711,8 +4711,13 @@ static int peak_show(struct seq_file *sf, void *v, struct page_counter *pc)
 static int memory_peak_show(struct seq_file *sf, void *v)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(sf));
+	int ret;
 
-	return peak_show(sf, v, &memcg->memory);
+	spin_lock(&memcg->peaks_lock);
+	ret = peak_show(sf, v, &memcg->memory);
+	spin_unlock(&memcg->peaks_lock);
+
+	return ret;
 }
 
 static int peak_open(struct kernfs_open_file *of)
@@ -5790,8 +5795,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;
 
-	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,
-- 
2.34.1

Re: [PATCH v2 0/2] mm, memcg: fix memory.peak reset clobbering other fds' watermark

[Andrew Morton]

On Fri,  7 Aug 2026 16:59:58 +0800 Ridong <ridong.chen@linux.dev> 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/20260807090000.1532495-1-ridong.chen@linux.dev

Re: [PATCH v2 0/2] mm, memcg: fix memory.peak reset clobbering other fds' watermark

[Ridong Chen]

On 8/8/2026 4:22 AM, Andrew Morton wrote:
> On Fri,  7 Aug 2026 16:59:58 +0800 Ridong <ridong.chen@linux.dev> 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.
> 

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.

> Sashiko might have found a similar race on the writer side:
> 	https://sashiko.dev/#/patchset/20260807090000.1532495-1-ridong.chen@linux.dev

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

-- 
Best regards
Ridong

Re: [PATCH v2 0/2] mm, memcg: fix memory.peak reset clobbering other fds' watermark

[Ridong Chen]

On 8/10/2026 10:58 AM, Ridong Chen wrote:
> 
> 
> On 8/8/2026 4:22 AM, Andrew Morton wrote:
>> On Fri,  7 Aug 2026 16:59:58 +0800 Ridong <ridong.chen@linux.dev> 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.
>>
> 
> 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.
> 
>> Sashiko might have found a similar race on the writer side:
>>     https://sashiko.dev/#/patchset/20260807090000.1532495-1-ridong.chen@linux.dev
> 
> Yes, that's the writer side that patch 2 fixes;
> 

Hello everyone,

I would appreciate a review of this series. Should any feedback come up, I will 
update both the patch and the cover letter accordingly.

-- 
Best regards
Ridong

Re: [PATCH v2 1/2] memcg: acquire peaks_lock when reading memory.peak

[Johannes Weiner]

On Fri, Aug 07, 2026 at 04:59:59PM +0800, Ridong wrote:
> From: Ridong Chen <chenridong@xiaomi.com>
> 
> 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/20260730115314.1069089-1-ridong.chen@linux.dev?part=1
> Fixes: c6f53ed8f213 ("mm, memcg: cg2 memory{.swap,}.peak write handlers")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Ridong Chen <chenridong@xiaomi.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH v2 2/2] mm, memcg: fix memory.peak reset clobbering other fds' watermark

[Johannes Weiner]

On Fri, Aug 07, 2026 at 05:00:00PM +0800, Ridong wrote:
> From: Ridong Chen <chenridong@xiaomi.com>
> 
> 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 <chenridong@xiaomi.com>
> ---
>  mm/memcontrol.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 2da55b778ae3..28577beeb3d0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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);
>  
>  	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 <hannes@cmpxchg.org>