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:

quoted

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