linux-kernel - Re: [PATCH] mm: mark mm_struct.hiwater

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8c946f46-6453-4537-ab41-0e9dbde684e8@iencinas.com>
Date: Mon, 31 Mar 2025 20:08:12 +0200
From: Ignacio Encinas Rubio <ignacio@...cinas.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
 "Liam R. Howlett" <Liam.Howlett@...cle.com>,
 linux-kernel-mentees@...ts.linux.dev, skhan@...uxfoundation.org,
 linux-kernel@...r.kernel.org,
 syzbot+419c4b42acc36c420ad3@...kaller.appspotmail.com
Subject: Re: [PATCH] mm: mark mm_struct.hiwater_rss as data racy



On 31/3/25 13:48, Lorenzo Stoakes wrote:
> On Sun, Mar 30, 2025 at 02:02:04PM +0200, Ignacio Encinas wrote:
>> mm_struct.hiwater_rss can be accessed concurrently without proper
>> synchronization as reported by KCSAN.
>>
>> Given that this just provides accounting information and that the extra
>> accuracy isn't worth the potential slowdown, let's annotate is
>> __data_racy to make KCSAN happy.
>>
>> Reported-by: syzbot+419c4b42acc36c420ad3@...kaller.appspotmail.com
> 
> You'll want a:
> Closes: https://lore.kernel.org/all/67e3390c.050a0220.1ec46.0001.GAE@google.com/
> 
> Here too.

Thanks for pointing it out. I saw some people didn't use it, but I'll
add it. 

>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>> index 0234f14f2aa6bea42a8a62ccb915c94f556cd3cc..84c86951a978aad07ab4ecefbfff77e7418d8402 100644
>> --- a/include/linux/mm_types.h
>> +++ b/include/linux/mm_types.h
>> @@ -19,6 +19,7 @@
>>  #include <linux/workqueue.h>
>>  #include <linux/seqlock.h>
>>  #include <linux/percpu_counter.h>
>> +#include <linux/compiler_types.h>
>>
>>  #include <asm/mmu.h>
>>
>> @@ -939,7 +940,7 @@ struct mm_struct {
>>  #endif
>>
>>
>> -		unsigned long hiwater_rss; /* High-watermark of RSS usage */
>> +		unsigned long __data_racy hiwater_rss; /* High-watermark of RSS usage */
> 
> This translates to volatile if KCSAN is enabled, and I really don't want to
> apply that unnecessarily given the impliciations/any weirdness we might
> observe as a result that might be confounding.
> 
> I also don't want to _across the board_ say 'hey we don't care about races
> for this'.
> 
> I think use of data_race() would make more sense.

Makes sense. The logic behind using __data_racy was to convey that data
races were fine in this case because it's just statistical information.
>From what you're saying I misinterpreted the situation :)

> Probably we're fine doing this in update_hiwater_rss(), so something like:
> 
> static inline void update_hiwater_rss(struct mm_struct *mm)
> {
> 	unsigned long _rss = get_mm_rss(mm);
> 
> 	if (data_race(mm->hiwater_rss) < _rss)
> 		mm->hiwater_rss = _rss;
> }
> 
> This labels it as 'we don't care', is a no-op if KCSAN is disabled and
> abstracts the decision to this specific point.

Thanks for the suggestion. I agree this looks fine. If there are other 
races (for which get_mm_hiwater_rss seems the only candidate) these can 
be examined on their own.

I'll adapt your suggestion, reword the commit and send a v2.

Thanks!