mm: correct type for vmalloc vm_flags fields

[PATCH] mm: correct type for vmalloc vm_flags fields

Posted by Lorenzo Stoakes 2 months, 1 week ago

Several functions refer to the unfortunately named 'vm_flags' field when
referencing vmalloc flags, which happens to be the precise same name used
for VMA flags.

As a result these were erroneously changed to use the vm_flags_t type
(which currently is a typedef equivalent to unsigned long).

Currently this has no impact, but in future when vm_flags_t changes this
will result in issues, so change the type to unsigned long to account for
this.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reported-by: Harry Yoo <harry.yoo@oracle.com>
Closes: https://lore.kernel.org/all/aIgSpAnU8EaIcqd9@hyeyoo/
---
 mm/execmem.c  | 8 ++++----
 mm/internal.h | 2 +-
 mm/nommu.c    | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/mm/execmem.c b/mm/execmem.c
index 627e6cf64f4f..2b683e7d864d 100644
--- a/mm/execmem.c
+++ b/mm/execmem.c
@@ -26,7 +26,7 @@ static struct execmem_info default_execmem_info __ro_after_init;

 #ifdef CONFIG_MMU
 static void *execmem_vmalloc(struct execmem_range *range, size_t size,
-			     pgprot_t pgprot, vm_flags_t vm_flags)
+			     pgprot_t pgprot, unsigned long vm_flags)
 {
 	bool kasan = range->flags & EXECMEM_KASAN_SHADOW;
 	gfp_t gfp_flags = GFP_KERNEL | __GFP_NOWARN;
@@ -82,7 +82,7 @@ struct vm_struct *execmem_vmap(size_t size)
 }
 #else
 static void *execmem_vmalloc(struct execmem_range *range, size_t size,
-			     pgprot_t pgprot, vm_flags_t vm_flags)
+			     pgprot_t pgprot, unsigned long vm_flags)
 {
 	return vmalloc(size);
 }
@@ -256,7 +256,7 @@ static void *__execmem_cache_alloc(struct execmem_range *range, size_t size)

 static int execmem_cache_populate(struct execmem_range *range, size_t size)
 {
-	vm_flags_t vm_flags = VM_ALLOW_HUGE_VMAP;
+	unsigned long vm_flags = VM_ALLOW_HUGE_VMAP;
 	struct vm_struct *vm;
 	size_t alloc_size;
 	int err = -ENOMEM;
@@ -373,7 +373,7 @@ void *execmem_alloc(enum execmem_type type, size_t size)
 {
 	struct execmem_range *range = &execmem_info->ranges[type];
 	bool use_cache = range->flags & EXECMEM_ROX_CACHE;
-	vm_flags_t vm_flags = VM_FLUSH_RESET_PERMS;
+	unsigned long vm_flags = VM_FLUSH_RESET_PERMS;
 	pgprot_t pgprot = range->pgprot;
 	void *p;

diff --git a/mm/internal.h b/mm/internal.h
index 28d2d5b051df..142d9302c2ae 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1391,7 +1391,7 @@ int migrate_device_coherent_folio(struct folio *folio);

 struct vm_struct *__get_vm_area_node(unsigned long size,
 				     unsigned long align, unsigned long shift,
-				     vm_flags_t vm_flags, unsigned long start,
+				     unsigned long vm_flags, unsigned long start,
 				     unsigned long end, int node, gfp_t gfp_mask,
 				     const void *caller);

diff --git a/mm/nommu.c b/mm/nommu.c
index 87e1acab0d64..07504d666d6a 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -126,7 +126,7 @@ void *vrealloc_noprof(const void *p, size_t size, gfp_t flags)

 void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
 		unsigned long start, unsigned long end, gfp_t gfp_mask,
-		pgprot_t prot, vm_flags_t vm_flags, int node,
+		pgprot_t prot, unsigned long vm_flags, int node,
 		const void *caller)
 {
 	return __vmalloc_noprof(size, gfp_mask);
--
2.50.1

Re: [PATCH] mm: correct type for vmalloc vm_flags fields

Posted by Vlastimil Babka 2 months, 1 week ago

On 7/29/25 13:49, Lorenzo Stoakes wrote:
> Several functions refer to the unfortunately named 'vm_flags' field when
> referencing vmalloc flags, which happens to be the precise same name used
> for VMA flags.
> 
> As a result these were erroneously changed to use the vm_flags_t type
> (which currently is a typedef equivalent to unsigned long).
> 
> Currently this has no impact, but in future when vm_flags_t changes this
> will result in issues, so change the type to unsigned long to account for
> this.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reported-by: Harry Yoo <harry.yoo@oracle.com>
> Closes: https://lore.kernel.org/all/aIgSpAnU8EaIcqd9@hyeyoo/

Acked-by: Vlastimil Babka <vbabka@suse.cz>

also for "mm: fixup very disguised vmalloc flags parameter" later in this thread

> ---
>  mm/execmem.c  | 8 ++++----
>  mm/internal.h | 2 +-
>  mm/nommu.c    | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/execmem.c b/mm/execmem.c
> index 627e6cf64f4f..2b683e7d864d 100644
> --- a/mm/execmem.c
> +++ b/mm/execmem.c
> @@ -26,7 +26,7 @@ static struct execmem_info default_execmem_info __ro_after_init;
> 
>  #ifdef CONFIG_MMU
>  static void *execmem_vmalloc(struct execmem_range *range, size_t size,
> -			     pgprot_t pgprot, vm_flags_t vm_flags)
> +			     pgprot_t pgprot, unsigned long vm_flags)
>  {
>  	bool kasan = range->flags & EXECMEM_KASAN_SHADOW;
>  	gfp_t gfp_flags = GFP_KERNEL | __GFP_NOWARN;
> @@ -82,7 +82,7 @@ struct vm_struct *execmem_vmap(size_t size)
>  }
>  #else
>  static void *execmem_vmalloc(struct execmem_range *range, size_t size,
> -			     pgprot_t pgprot, vm_flags_t vm_flags)
> +			     pgprot_t pgprot, unsigned long vm_flags)
>  {
>  	return vmalloc(size);
>  }
> @@ -256,7 +256,7 @@ static void *__execmem_cache_alloc(struct execmem_range *range, size_t size)
> 
>  static int execmem_cache_populate(struct execmem_range *range, size_t size)
>  {
> -	vm_flags_t vm_flags = VM_ALLOW_HUGE_VMAP;
> +	unsigned long vm_flags = VM_ALLOW_HUGE_VMAP;
>  	struct vm_struct *vm;
>  	size_t alloc_size;
>  	int err = -ENOMEM;
> @@ -373,7 +373,7 @@ void *execmem_alloc(enum execmem_type type, size_t size)
>  {
>  	struct execmem_range *range = &execmem_info->ranges[type];
>  	bool use_cache = range->flags & EXECMEM_ROX_CACHE;
> -	vm_flags_t vm_flags = VM_FLUSH_RESET_PERMS;
> +	unsigned long vm_flags = VM_FLUSH_RESET_PERMS;
>  	pgprot_t pgprot = range->pgprot;
>  	void *p;
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 28d2d5b051df..142d9302c2ae 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1391,7 +1391,7 @@ int migrate_device_coherent_folio(struct folio *folio);
> 
>  struct vm_struct *__get_vm_area_node(unsigned long size,
>  				     unsigned long align, unsigned long shift,
> -				     vm_flags_t vm_flags, unsigned long start,
> +				     unsigned long vm_flags, unsigned long start,
>  				     unsigned long end, int node, gfp_t gfp_mask,
>  				     const void *caller);
> 
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 87e1acab0d64..07504d666d6a 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -126,7 +126,7 @@ void *vrealloc_noprof(const void *p, size_t size, gfp_t flags)
> 
>  void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
>  		unsigned long start, unsigned long end, gfp_t gfp_mask,
> -		pgprot_t prot, vm_flags_t vm_flags, int node,
> +		pgprot_t prot, unsigned long vm_flags, int node,
>  		const void *caller)
>  {
>  	return __vmalloc_noprof(size, gfp_mask);
> --
> 2.50.1

Re: [PATCH] mm: correct type for vmalloc vm_flags fields

Posted by Lorenzo Stoakes 2 months, 1 week ago

Hi Andrew,

Please apply the below to this series.

Again to re-emphasise, there's no _actual_ issue here (using a typedef for
unsigned long vs unsigned long), it's just fixing up things in preparation
for later changes where this will matter.

Thanks,
Lorenzo

----8<----
From 94ccb0c9e49bf3f9a5a31e2f1da4a722b0a2de50 Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Tue, 29 Jul 2025 13:48:50 +0100
Subject: [PATCH] mm: fixup very disguised vmalloc flags parameter

The declare_vma() function in arm64 arch code liberally mixes the concepts
of VMAs and near-identically named vmalloc data structures, so I
accidentally changed the 'vm_flags' field' that is assigned to a 'vma'
thinking it was... the vm_flags field of a vma, which it turns out, it
isn't.

Revert the type from vm_flags_t to unsigned long.

Given vm_flags_t == unsigned long, there is no change in any behaviour
before or after this patch, but in future this will matter.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 arch/arm64/mm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 20a89ab97dc5..34e5d78af076 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -721,7 +721,7 @@ void mark_rodata_ro(void)

 static void __init declare_vma(struct vm_struct *vma,
 			       void *va_start, void *va_end,
-			       vm_flags_t vm_flags)
+			       unsigned long vm_flags)
 {
 	phys_addr_t pa_start = __pa_symbol(va_start);
 	unsigned long size = va_end - va_start;
--
2.50.1

Re: [PATCH] mm: correct type for vmalloc vm_flags fields

Posted by Harry Yoo 2 months, 1 week ago

On Tue, Jul 29, 2025 at 01:54:07PM +0100, Lorenzo Stoakes wrote:
> Hi Andrew,
> 
> Please apply the below to this series.
> 
> Again to re-emphasise, there's no _actual_ issue here (using a typedef for
> unsigned long vs unsigned long), it's just fixing up things in preparation
> for later changes where this will matter.
> 
> Thanks,
> Lorenzo
> 
> ----8<----
> From 94ccb0c9e49bf3f9a5a31e2f1da4a722b0a2de50 Mon Sep 17 00:00:00 2001
> From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Date: Tue, 29 Jul 2025 13:48:50 +0100
> Subject: [PATCH] mm: fixup very disguised vmalloc flags parameter
> 
> The declare_vma() function in arm64 arch code liberally mixes the concepts
> of VMAs and near-identically named vmalloc data structures, so I
> accidentally changed the 'vm_flags' field' that is assigned to a 'vma'
> thinking it was... the vm_flags field of a vma, which it turns out, it
> isn't.
> 
> Revert the type from vm_flags_t to unsigned long.
> 
> Given vm_flags_t == unsigned long, there is no change in any behaviour
> before or after this patch, but in future this will matter.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---

Apologies for the last minute review and not reporting it at one go.

For both patches,
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

>  arch/arm64/mm/mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 20a89ab97dc5..34e5d78af076 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -721,7 +721,7 @@ void mark_rodata_ro(void)
> 
>  static void __init declare_vma(struct vm_struct *vma,
>  			       void *va_start, void *va_end,
> -			       vm_flags_t vm_flags)
> +			       unsigned long vm_flags)
>  {
>  	phys_addr_t pa_start = __pa_symbol(va_start);
>  	unsigned long size = va_end - va_start;
> --
> 2.50.1

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH] mm: correct type for vmalloc vm_flags fields

Posted by David Hildenbrand 2 months, 1 week ago

On 29.07.25 14:54, Lorenzo Stoakes wrote:
> Hi Andrew,
> 
> Please apply the below to this series.
> 
> Again to re-emphasise, there's no _actual_ issue here (using a typedef for
> unsigned long vs unsigned long), it's just fixing up things in preparation
> for later changes where this will matter.
> 
> Thanks,
> Lorenzo
> 
> ----8<----
>  From 94ccb0c9e49bf3f9a5a31e2f1da4a722b0a2de50 Mon Sep 17 00:00:00 2001
> From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Date: Tue, 29 Jul 2025 13:48:50 +0100
> Subject: [PATCH] mm: fixup very disguised vmalloc flags parameter
> 
> The declare_vma() function in arm64 arch code liberally mixes the concepts
> of VMAs and near-identically named vmalloc data structures, so I
> accidentally changed the 'vm_flags' field' that is assigned to a 'vma'
> thinking it was... the vm_flags field of a vma, which it turns out, it
> isn't.
> 
> Revert the type from vm_flags_t to unsigned long.
> 
> Given vm_flags_t == unsigned long, there is no change in any behaviour
> before or after this patch, but in future this will matter.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>   arch/arm64/mm/mmu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 20a89ab97dc5..34e5d78af076 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -721,7 +721,7 @@ void mark_rodata_ro(void)
> 
>   static void __init declare_vma(struct vm_struct *vma,
>   			       void *va_start, void *va_end,
> -			       vm_flags_t vm_flags)
> +			       unsigned long vm_flags)
>   {
>   	phys_addr_t pa_start = __pa_symbol(va_start);
>   	unsigned long size = va_end - va_start;
> --
> 2.50.1
> 

For both (however they may or may not get sqashed)

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm: correct type for vmalloc vm_flags fields

Posted by Pedro Falcato 2 months, 1 week ago

On Tue, Jul 29, 2025 at 12:49:06PM +0100, Lorenzo Stoakes wrote:
> Several functions refer to the unfortunately named 'vm_flags' field when
> referencing vmalloc flags, which happens to be the precise same name used
> for VMA flags.
> 
> As a result these were erroneously changed to use the vm_flags_t type
> (which currently is a typedef equivalent to unsigned long).
> 
> Currently this has no impact, but in future when vm_flags_t changes this
> will result in issues, so change the type to unsigned long to account for
> this.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reported-by: Harry Yoo <harry.yoo@oracle.com>
> Closes: https://lore.kernel.org/all/aIgSpAnU8EaIcqd9@hyeyoo/

Reviewed-by: Pedro Falcato <pfalcato@suse.de>

I think the existence of this mistake really tells us that we _really_ need some sort
of type checking of this stuff, in the future.

-- 
Pedro

Re: [PATCH] mm: correct type for vmalloc vm_flags fields

Posted by Lorenzo Stoakes 2 months, 1 week ago

Again to emphasise - the original series is not broken, this is just laying
the ground for a future change and fixing places where _that_ change would
be broken (and picked up then).

So there's nothing to be worried about here.

On Tue, Jul 29, 2025 at 01:28:31PM +0100, Pedro Falcato wrote:
> On Tue, Jul 29, 2025 at 12:49:06PM +0100, Lorenzo Stoakes wrote:
> > Several functions refer to the unfortunately named 'vm_flags' field when
> > referencing vmalloc flags, which happens to be the precise same name used
> > for VMA flags.
> >
> > As a result these were erroneously changed to use the vm_flags_t type
> > (which currently is a typedef equivalent to unsigned long).
> >
> > Currently this has no impact, but in future when vm_flags_t changes this
> > will result in issues, so change the type to unsigned long to account for
> > this.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > Reported-by: Harry Yoo <harry.yoo@oracle.com>
> > Closes: https://lore.kernel.org/all/aIgSpAnU8EaIcqd9@hyeyoo/
>
> Reviewed-by: Pedro Falcato <pfalcato@suse.de>

Thanks.

>
> I think the existence of this mistake really tells us that we _really_ need some sort
> of type checking of this stuff, in the future.

I guess you forgot the off-list conversation in which I said I was going to
do exactly what you suggest here...

But yes, I already felt this way (as there seems no sensible way to make
static tools check carefully, and it's enormously easy to miss something),
and this is precisely what I intend to do.

Re: [PATCH] mm: correct type for vmalloc vm_flags fields

Posted by Harry Yoo 2 months, 1 week ago

On Tue, Jul 29, 2025 at 12:49:06PM +0100, Lorenzo Stoakes wrote:
> Several functions refer to the unfortunately named 'vm_flags' field when
> referencing vmalloc flags, which happens to be the precise same name used
> for VMA flags.
> 
> As a result these were erroneously changed to use the vm_flags_t type
> (which currently is a typedef equivalent to unsigned long).
> 
> Currently this has no impact, but in future when vm_flags_t changes this
> will result in issues, so change the type to unsigned long to account for
> this.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reported-by: Harry Yoo <harry.yoo@oracle.com>
> Closes: https://lore.kernel.org/all/aIgSpAnU8EaIcqd9@hyeyoo/
> ---

I see one more thing in patch 3 of the series:

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 8fcf59ba39db..248d96349fd0 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -720,7 +720,7 @@ void mark_rodata_ro(void)

 static void __init declare_vma(struct vm_struct *vma,
 			       void *va_start, void *va_end,
-			       unsigned long vm_flags)
+			       vm_flags_t vm_flags)
 {
 	phys_addr_t pa_start = __pa_symbol(va_start);
 	unsigned long size = va_end - va_start;


With that, all looks good.

"struct vm_struct *vma" makes it even more confusing by the way...

-- 
Cheers,
Harry / Hyeonggon

>  mm/execmem.c  | 8 ++++----
>  mm/internal.h | 2 +-
>  mm/nommu.c    | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)

Re: [PATCH] mm: correct type for vmalloc vm_flags fields

Posted by Lorenzo Stoakes 2 months, 1 week ago

On Tue, Jul 29, 2025 at 09:10:50PM +0900, Harry Yoo wrote:
> I see one more thing in patch 3 of the series:

Can we please try to do this review closer to the series in future, and
ideally in one go. Doing things this way isn't helpful.

While you're right (this function is just awful, no wonder - struct
vm_struct vma...!), the vm_flags_t series is in NO WAY broken, since
vm_flags_t == unsigned long.

I expect there to be a few 'stragglers' of one kind or another, when I do
the next stage of the work _all such callers_ will be detected (as the
kernel will not compile otherwise).

Anyway of course we should fix this, the TL; DR is this isn't an urgent
fix.

I'll send a fix-patch.

>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 8fcf59ba39db..248d96349fd0 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -720,7 +720,7 @@ void mark_rodata_ro(void)
>
>  static void __init declare_vma(struct vm_struct *vma,

^ this is beyond appalling.

Somebody from arm side should really fix this because this is just insane.

>  			       void *va_start, void *va_end,
> -			       unsigned long vm_flags)
> +			       vm_flags_t vm_flags)
>  {
>  	phys_addr_t pa_start = __pa_symbol(va_start);
>  	unsigned long size = va_end - va_start;

Re: [PATCH] mm: correct type for vmalloc vm_flags fields

Posted by Harry Yoo 2 months, 1 week ago

On Tue, Jul 29, 2025 at 09:10:50PM +0900, Harry Yoo wrote:
> On Tue, Jul 29, 2025 at 12:49:06PM +0100, Lorenzo Stoakes wrote:
> > Several functions refer to the unfortunately named 'vm_flags' field when
> > referencing vmalloc flags, which happens to be the precise same name used
> > for VMA flags.
> > 
> > As a result these were erroneously changed to use the vm_flags_t type
> > (which currently is a typedef equivalent to unsigned long).
> > 
> > Currently this has no impact, but in future when vm_flags_t changes this
> > will result in issues, so change the type to unsigned long to account for
> > this.
> > 
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > Reported-by: Harry Yoo <harry.yoo@oracle.com>
> > Closes: https://lore.kernel.org/all/aIgSpAnU8EaIcqd9@hyeyoo/
> > ---
> 
> I see one more thing in patch 3 of the series:
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 8fcf59ba39db..248d96349fd0 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -720,7 +720,7 @@ void mark_rodata_ro(void)
> 
>  static void __init declare_vma(struct vm_struct *vma,
>  			       void *va_start, void *va_end,
> -			       unsigned long vm_flags)
> +			       vm_flags_t vm_flags)
>  {
>  	phys_addr_t pa_start = __pa_symbol(va_start);
>  	unsigned long size = va_end - va_start;
> 
> 
> With that, all looks good.

I mean, the hunk above is from patch 3 and it should be reverted too.

> "struct vm_struct *vma" makes it even more confusing by the way...
> 
> -- 
> Cheers,
> Harry / Hyeonggon
> 
> >  mm/execmem.c  | 8 ++++----
> >  mm/internal.h | 2 +-
> >  mm/nommu.c    | 2 +-
> >  3 files changed, 6 insertions(+), 6 deletions(-)