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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2c15f7bf-7852-4aa5-98f1-c8604fe8fc00@lucifer.local>
Date: Mon, 31 Mar 2025 12:48:54 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Ignacio Encinas <ignacio@...cinas.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 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.

> Suggested-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> Signed-off-by: Ignacio Encinas <ignacio@...cinas.com>

Thanks for this, but I don't think this patch works, see below for a
suggested alternative approach.

> ---
> Similar issues have been solved in the past [1]. An actual analysis of
> the data race can be found in [2] and [3].
>
> Lorenzo, I added the Suggested-by as your proposal seems roughly
> equivalent to what I propose.

Sure no problem!

>
> [1] https://lore.kernel.org/all/20210913105550.1569419-1-liupeng256@huawei.com/T/#u
> [2] https://lore.kernel.org/all/900c5035-865d-40b7-8d55-0cbbbc059294@lucifer.local/
> [3] https://lore.kernel.org/linux-mm/iwtvhos74gwrk5v5szlosnkusxqp6ubqy6ytkclkucbjwdw4zr@bwxyrwcnybbz/
> ---
>  include/linux/mm_types.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> 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.

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.

Cheers, Lorenzo


>  		unsigned long hiwater_vm;  /* High-water virtual memory usage */
>
>  		unsigned long total_vm;	   /* Total pages mapped */
>
> ---
> base-commit: 3571e8b091f4270d869dda7a6cc43616c6ad6897
> change-id: 20250315-mm-maxrss-data-race-6ce86b0deb14
>
> Best regards,
> --
> Ignacio Encinas <ignacio@...cinas.com>
>