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
