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

On 8/14/2026 11:48 AM, [email protected] wrote:
> 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.
> 
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 1d3339520809f..1546993168610 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -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?
> 
> [ ... ]
> 
>> @@ -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);
>>   }
> 

Thanks.

Patch 2/2 will fix this issue.

-- 
Best regards
Ridong