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

On 7/30/2026 8:06 PM, [email protected] wrote:
> 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.
> 
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 8ff285e872ca9..7d7b6bf200805 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -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);
>>   
>>   	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?
> 

It seems this issue exsit already, maybe we should add 
spin_lock(&memcg->peaks_lock) in peak_show()?

> 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?
> 
>>   
>>   	/* initial write, register watcher */
>>   	if (ofp->value == OFP_PEAK_UNSET)
> 

-- 
Best regards
Ridong