Message-ID: <64r5sm2aqgphrs5t4vzzgz7qitn3efmxpxjzv4wsaeyuncrn56@tdu4z7uzxluq>
Date: Fri, 20 Jun 2025 19:46:56 +0100
From: Pedro Falcato <pfalcato@...e.de>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Christian Brauner <brauner@...nel.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, Russell King <linux@...linux.org.uk>, 
	Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>, 
	Madhavan Srinivasan <maddy@...ux.ibm.com>, Michael Ellerman <mpe@...erman.id.au>, 
	Nicholas Piggin <npiggin@...il.com>, Christophe Leroy <christophe.leroy@...roup.eu>, 
	"David S . Miller" <davem@...emloft.net>, Andreas Larsson <andreas@...sler.com>, 
	Jarkko Sakkinen <jarkko@...nel.org>, Dave Hansen <dave.hansen@...ux.intel.com>, 
	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	"H . Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, Alexander Viro <viro@...iv.linux.org.uk>, 
	Jan Kara <jack@...e.cz>, Kees Cook <kees@...nel.org>, Peter Xu <peterx@...hat.com>, 
	David Hildenbrand <david@...hat.com>, Zi Yan <ziy@...dia.com>, 
	Baolin Wang <baolin.wang@...ux.alibaba.com>, "Liam R . Howlett" <Liam.Howlett@...cle.com>, 
	Nico Pache <npache@...hat.com>, Ryan Roberts <ryan.roberts@....com>, Dev Jain <dev.jain@....com>, 
	Barry Song <baohua@...nel.org>, Xu Xin <xu.xin16@....com.cn>, 
	Chengming Zhou <chengming.zhou@...ux.dev>, Hugh Dickins <hughd@...gle.com>, 
	Vlastimil Babka <vbabka@...e.cz>, Mike Rapoport <rppt@...nel.org>, 
	Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>, Rik van Riel <riel@...riel.com>, 
	Harry Yoo <harry.yoo@...cle.com>, Dan Williams <dan.j.williams@...el.com>, 
	Matthew Wilcox <willy@...radead.org>, Steven Rostedt <rostedt@...dmis.org>, 
	Masami Hiramatsu <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, 
	Jason Gunthorpe <jgg@...pe.ca>, John Hubbard <jhubbard@...dia.com>, 
	Muchun Song <muchun.song@...ux.dev>, Oscar Salvador <osalvador@...e.de>, Jann Horn <jannh@...gle.com>, 
	Johannes Weiner <hannes@...xchg.org>, Qi Zheng <zhengqi.arch@...edance.com>, 
	Shakeel Butt <shakeel.butt@...ux.dev>, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org, kvm@...r.kernel.org, 
	sparclinux@...r.kernel.org, linux-sgx@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	linux-mm@...ck.org, nvdimm@...ts.linux.dev, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] mm: change vm_get_page_prot() to accept vm_flags_t
 argument

On Thu, Jun 19, 2025 at 09:49:03AM +0100, Lorenzo Stoakes wrote:
> On Thu, Jun 19, 2025 at 10:42:14AM +0200, Christian Brauner wrote:
> > If you change vm_flags_t to u64 you probably want to compile with some
> > of these integer truncation options when you're doing the conversion.
> > Because otherwise you risk silently truncating the upper 32bits when
> > assigning to a 32bit variable. We've had had a patch series that almost
> > introduced a very subtle bug when it tried to add the first flag outside
> > the 32bit range in the lookup code a while ago. That series never made
> > it but it just popped back into my head when I read your series.
> 
> Yeah am very wary of this, it's a real concern. I'm not sure how precisely we
> might enable such options but only in this instance? Because presumably we are
> intentionally narrowing in probably quite a few places.
> 
> Pedro mentioned that there might be compiler options to help so I'm guessing
> this is the same thing as to what you're thinking here?

I was thinking about -Wnarrowing but sadly it seems that this is only for C++
code. Also MSVC is quite strict (even in C) when it comes to this stuff, so you
could also add MSVC support to the kernel, small task :P

One could in theory add support for this stuff in GCC, but I would expect it
to flag almost everything in the kernel (e.g long -> int implicit conversions).
> 
> I also considered a sparse flag, Pedro mentioned bitwise, but then I worry that
> we'd have to __force in a million places to make that work and it'd be
> non-obvious.

Here's an example for __bitwise usage taken from block:

typedef unsigned int __bitwise blk_insert_t;
#define BLK_MQ_INSERT_AT_HEAD		((__force blk_insert_t)0x01)


then in block/blk-mq.c:

	if (flags & BLK_MQ_INSERT_AT_HEAD)
		list_add(&rq->queuelist, &hctx->dispatch);


So doing regular old flag checks with the bitwise & operator seems to work fine.
Assignment itself should also just work. So as long as we're vm_flags_t-typesafe
there should be no problem? I think.

> 
> Matthew's original concept for this was to simply wrap an array, but that'd
> require a complete rework of every single place where we use VMA flags (perhaps
> we could mitigate it a _bit_ with a vm_flags_val() helper that grabs a u64?)
> 

I think the real question is whether we expect to ever require > 64 flags for
VMAs? If so, going with an array would be the best option here. Though in that
case I would guess we probably want to hide the current vm_flags member in
vm_area_struct first, providing some vm_flags_is_set() and whatnot.

-- 
Pedro

Powered by blists - more mailing lists