Re: [PATCH] mm: mark mm_struct.hiwater_rss as data racy

[Lorenzo Stoakes]

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@xxxxxxxxxxxxxxxxxxxxxxxxx

You'll want a:
Closes: https://lore.kernel.org/all/67e3390c.050a0220.1ec46.0001.GAE@xxxxxxxxxx/

Here too.

> Suggested-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> Signed-off-by: Ignacio Encinas <ignacio@xxxxxxxxxxxx>

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@xxxxxxxxxx/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@xxxxxxxxxxxx>
>