While staring at include/linux/mm.h, I was wondering why VM_UFFD_MINOR and
VM_SHADOW_STACK share the same bit on arm64. I think I gained enough confidence
now to call it a bug.

The first patch of this series is a straightforward attempt at fixing this
specific bug by changing the bit used by VM_UFFD_MINOR. I cc-ed stable on that
one and I expect it to not be all too controversial.

The rest of the series however is a more zealous refactoring and likely to be
more contentious... :) Since this bug looks like a near miss which could have
been quite severe in terms of security, I think it's worth trying to simplify
the high VMA flag bits code. I tried to consolidate around the current usage of
VM_HIGH_ARCH_* macros but I'm not sure if this is the preferred approach here. I
really don't feel strongly about those refactorings so this is more of a
platform for discussion for people with more mm background, I'll be more than
happy to respin a v2!

This series applies on v6.15-rc5.

Florent Revest (4):
  mm: fix VM_UFFD_MINOR == VM_SHADOW_STACK on USERFAULTFD=y &&
    ARM64_GCS=y
  mm: remove CONFIG_ARCH_USES_HIGH_VMA_FLAGS
  mm: use VM_HIGH_ARCH_* macros consistently
  mm: consolidate VM_HIGH_ARCH_* macros into parametric macros

 arch/arm64/Kconfig   |  3 ---
 arch/powerpc/Kconfig |  1 -
 arch/x86/Kconfig     |  2 --
 include/linux/mm.h   | 49 +++++++++++++++-----------------------------
 mm/Kconfig           |  2 --
 5 files changed, 17 insertions(+), 40 deletions(-)

-- 
2.49.0.967.g6a0df3ecc3-goog

On Tue, May 06, 2025 at 11:52:20AM +0200, Florent Revest wrote:

> While staring at include/linux/mm.h, I was wondering why VM_UFFD_MINOR and
> VM_SHADOW_STACK share the same bit on arm64. I think I gained enough confidence
> now to call it a bug.

Yes, it's a bug - it'll be an add/add conflict with those coming in via
different trees.

On Tue, May 6, 2025 at 3:34 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, May 06, 2025 at 11:52:20AM +0200, Florent Revest wrote:
>
> > While staring at include/linux/mm.h, I was wondering why VM_UFFD_MINOR and
> > VM_SHADOW_STACK share the same bit on arm64. I think I gained enough confidence
> > now to call it a bug.
>
> Yes, it's a bug - it'll be an add/add conflict with those coming in via
> different trees.

Thanks for the review Mark! :)

I just want to make sure I fully understand the point you're making
here, it seems that you are suggesting that 7677f7fd8be7
("userfaultfd: add minor fault registration mode") and ae80e1629aea
("mm: Define VM_SHADOW_STACK for arm64 when we support GCS") came in
from two different trees and independently chose to use the same bit
around the ~same time, is that right ? And that a conflict would have
appeared when they'd eventually get merged into a shared tree ?

I don't think that's what happened in this specific case though. As
far as I can tell, the former was merged in 2021 and the latter was
merged in late 2024. Also, since the hunks got added in very different
parts of include/linux/mm.h, I don't think they caused a noticeable
merge conflict. But I agree it would probably be preferable if these
cases would cause some sort of noticeable merge conflict in the future
...

I'll quickly respin this series to address my typos on patch 4 (sigh)
and add your Reviewed-by tag but just to be clear, my refactorings in
patches 2/3/4 currently focus on using VM_HIGH_ARCH macros
consistently, to make it a bit more obvious to a reader if two
features choose the same bit. But maybe what we would really need
instead is a more obvious way for these bits to be mutually exclusive
and to cause merge conflicts if they get added through independent
trees ? For example, my colleague Brendan Jackman suggested using an
enum for VMA flags bit offsets but I'm not sure what the sentiment is
around that.

On Wed, May 07, 2025 at 03:09:50PM +0200, Florent Revest wrote:

> I just want to make sure I fully understand the point you're making
> here, it seems that you are suggesting that 7677f7fd8be7
> ("userfaultfd: add minor fault registration mode") and ae80e1629aea
> ("mm: Define VM_SHADOW_STACK for arm64 when we support GCS") came in
> from two different trees and independently chose to use the same bit
> around the ~same time, is that right ? And that a conflict would have
> appeared when they'd eventually get merged into a shared tree ?

> I don't think that's what happened in this specific case though. As
> far as I can tell, the former was merged in 2021 and the latter was
> merged in late 2024. Also, since the hunks got added in very different

Yes, indeed - I wrote that initial reply before I saw the actual change.
I wasn't expecting a conflict but rather that what'd happened was that
the bit was allocated independently in two different trees and then due
to the way the allocations are scattered everywhere no conflict had
flagged the issue.

> parts of include/linux/mm.h, I don't think they caused a noticeable
> merge conflict. But I agree it would probably be preferable if these
> cases would cause some sort of noticeable merge conflict in the future

Putting all the flags somewhere in the vicinity of each other would make
the whole thing much more robust, or having some build assert for
uniqueness.

> I'll quickly respin this series to address my typos on patch 4 (sigh)
> and add your Reviewed-by tag but just to be clear, my refactorings in
> patches 2/3/4 currently focus on using VM_HIGH_ARCH macros
> consistently, to make it a bit more obvious to a reader if two
> features choose the same bit. But maybe what we would really need
> instead is a more obvious way for these bits to be mutually exclusive
> and to cause merge conflicts if they get added through independent
> trees ? For example, my colleague Brendan Jackman suggested using an
> enum for VMA flags bit offsets but I'm not sure what the sentiment is
> around that.

I think we need something here, although it wasn't what actually
happened here the potential for the scenario I mentioned above to occur
remains.