[PATCH 0/5] mm: Pass pointers to page accessors

[PATCH 0/5] mm: Pass pointers to page accessors

[Linus Walleij]

In a recent change to the Arm architecture with the end goal
of removing highmem we need to convert virt_to_phys() and
virt_to_pfn() to static inline functions.

This will make them strongly typed.

However since virt_to_* is always implemented as macros they
have become polymorphic and accept both (void *) and
e.g. unsigned long as arguments.

Other functions such as virt_to_page() simply wrap
virt_to_pfn() and get affected indirectly.

To be able to proceed, patch mm to use (void *) as argument
to affected functions in all instances.

Linus Walleij (5):
  lib/test_free_pages.c: Pass a pointer to virt_to_page()
  mm/highmem: Pass a pointer to virt_to_page()
  mm: kfence: Pass a pointer to virt_to_page()
  mm: gup: Pass a pointer to virt_to_page()
  mm: nommu: Pass a pointer to virt_to_page()

 lib/test_free_pages.c | 2 +-
 mm/gup.c              | 2 +-
 mm/highmem.c          | 2 +-
 mm/kfence/core.c      | 4 ++--
 mm/nommu.c            | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.36.1

[PATCH 1/5] lib/test_free_pages.c: Pass a pointer to virt_to_page()

[Linus Walleij]

A pointer into virtual memory is represented by a (void *)
not an u32, so the compiler warns:

lib/test_free_pages.c:20:50: warning: passing argument 1
  of 'virt_to_pfn' makes pointer from integer without a
  cast [-Wint-conversion]

Fix this with an explicit cast.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 lib/test_free_pages.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/test_free_pages.c b/lib/test_free_pages.c
index 25ae1ac2624a..9ebf6f5549f3 100644
--- a/lib/test_free_pages.c
+++ b/lib/test_free_pages.c
@@ -17,7 +17,7 @@ static void test_free_pages(gfp_t gfp)
 
 	for (i = 0; i < 1000 * 1000; i++) {
 		unsigned long addr = __get_free_pages(gfp, 3);
-		struct page *page = virt_to_page(addr);
+		struct page *page = virt_to_page((void *)addr);
 
 		/* Simulate page cache getting a speculative reference */
 		get_page(page);
-- 
2.36.1

[PATCH 2/5] mm/highmem: Pass a pointer to virt_to_page()

[Linus Walleij]

Functions that work on a pointer to virtual memory such as
virt_to_pfn() and users of that function such as
virt_to_page() are supposed to pass a pointer to virtual
memory, ideally a (void *) or other pointer. However since
many architectures implement virt_to_pfn() as a macro,
this function becomes polymorphic and accepts both a
(unsigned long) and a (void *).

If we instead implement a proper virt_to_pfn(void *addr)
function the following happens (occurred on arch/arm):

mm/highmem.c:153:29: warning: passing argument 1 of
  'virt_to_pfn' makes pointer from integer without a
  cast [-Wint-conversion]

We already have a proper void * pointer in the scope of this
function named "vaddr" so pass that instead.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 mm/highmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/highmem.c b/mm/highmem.c
index 1a692997fac4..e92a7ceb30e8 100644
--- a/mm/highmem.c
+++ b/mm/highmem.c
@@ -150,7 +150,7 @@ struct page *__kmap_to_page(void *vaddr)
 		return pte_page(pkmap_page_table[i]);
 	}
 
-	return virt_to_page(addr);
+	return virt_to_page(vaddr);
 }
 EXPORT_SYMBOL(__kmap_to_page);
 
-- 
2.36.1

[PATCH 3/5] mm: kfence: Pass a pointer to virt_to_page()

[Linus Walleij]

Functions that work on a pointer to virtual memory such as
virt_to_pfn() and users of that function such as
virt_to_page() are supposed to pass a pointer to virtual
memory, ideally a (void *) or other pointer. However since
many architectures implement virt_to_pfn() as a macro,
this function becomes polymorphic and accepts both a
(unsigned long) and a (void *).

If we instead implement a proper virt_to_pfn(void *addr)
function the following happens (occurred on arch/arm):

mm/kfence/core.c:558:30: warning: passing argument 1
  of 'virt_to_pfn' makes pointer from integer without a
  cast [-Wint-conversion]

In one case we can refer to __kfence_pool directly (and
that is a proper (char *) pointer) and in the other call
site we use an explicit cast.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Alexander Potapenko <glider@google.com>
Cc: Marco Elver <elver@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: kasan-dev@googlegroups.com
Cc: linux-mm@kvack.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 mm/kfence/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 4e7cd4c8e687..153cde62ad72 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -543,7 +543,7 @@ static unsigned long kfence_init_pool(void)
 	if (!arch_kfence_init_pool())
 		return addr;
 
-	pages = virt_to_page(addr);
+	pages = virt_to_page(__kfence_pool);
 
 	/*
 	 * Set up object pages: they must have PG_slab set, to avoid freeing
@@ -657,7 +657,7 @@ static bool kfence_init_pool_late(void)
 	/* Same as above. */
 	free_size = KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool);
 #ifdef CONFIG_CONTIG_ALLOC
-	free_contig_range(page_to_pfn(virt_to_page(addr)), free_size / PAGE_SIZE);
+	free_contig_range(page_to_pfn(virt_to_page((void *)addr)), free_size / PAGE_SIZE);
 #else
 	free_pages_exact((void *)addr, free_size);
 #endif
-- 
2.36.1

Re: [PATCH 3/5] mm: kfence: Pass a pointer to virt_to_page()

[Marco Elver]

On Thu, 30 Jun 2022 at 10:43, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Functions that work on a pointer to virtual memory such as
> virt_to_pfn() and users of that function such as
> virt_to_page() are supposed to pass a pointer to virtual
> memory, ideally a (void *) or other pointer. However since
> many architectures implement virt_to_pfn() as a macro,
> this function becomes polymorphic and accepts both a
> (unsigned long) and a (void *).
>
> If we instead implement a proper virt_to_pfn(void *addr)
> function the following happens (occurred on arch/arm):
>
> mm/kfence/core.c:558:30: warning: passing argument 1
>   of 'virt_to_pfn' makes pointer from integer without a
>   cast [-Wint-conversion]
>
> In one case we can refer to __kfence_pool directly (and
> that is a proper (char *) pointer) and in the other call
> site we use an explicit cast.
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: kasan-dev@googlegroups.com
> Cc: linux-mm@kvack.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Marco Elver <elver@google.com>

> ---
>  mm/kfence/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index 4e7cd4c8e687..153cde62ad72 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -543,7 +543,7 @@ static unsigned long kfence_init_pool(void)
>         if (!arch_kfence_init_pool())
>                 return addr;
>
> -       pages = virt_to_page(addr);
> +       pages = virt_to_page(__kfence_pool);
>
>         /*
>          * Set up object pages: they must have PG_slab set, to avoid freeing
> @@ -657,7 +657,7 @@ static bool kfence_init_pool_late(void)
>         /* Same as above. */
>         free_size = KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool);
>  #ifdef CONFIG_CONTIG_ALLOC
> -       free_contig_range(page_to_pfn(virt_to_page(addr)), free_size / PAGE_SIZE);
> +       free_contig_range(page_to_pfn(virt_to_page((void *)addr)), free_size / PAGE_SIZE);
>  #else
>         free_pages_exact((void *)addr, free_size);
>  #endif
> --
> 2.36.1
>

[PATCH 4/5] mm: gup: Pass a pointer to virt_to_page()

[Linus Walleij]

Functions that work on a pointer to virtual memory such as
virt_to_pfn() and users of that function such as
virt_to_page() are supposed to pass a pointer to virtual
memory, ideally a (void *) or other pointer. However since
many architectures implement virt_to_pfn() as a macro,
this function becomes polymorphic and accepts both a
(unsigned long) and a (void *).

If we instead implement a proper virt_to_pfn(void *addr)
function the following happens (occurred on arch/arm):

  mm/gup.c: In function '__get_user_pages_locked':
  mm/gup.c:1599:49: warning: passing argument 1 of 'virt_to_pfn'
    makes pointer from integer without a cast [-Wint-conversion]
    pages[i] = virt_to_page(start);

Fix this with an explicit cast.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 mm/gup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/gup.c b/mm/gup.c
index 551264407624..543c68da65f1 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1672,7 +1672,7 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
 			goto finish_or_fault;
 
 		if (pages) {
-			pages[i] = virt_to_page(start);
+			pages[i] = virt_to_page((void *)start);
 			if (pages[i])
 				get_page(pages[i]);
 		}
-- 
2.36.1

Re: [PATCH 4/5] mm: gup: Pass a pointer to virt_to_page()

[Jason Gunthorpe]

On Thu, Jun 30, 2022 at 10:41:23AM +0200, Linus Walleij wrote:
> Functions that work on a pointer to virtual memory such as
> virt_to_pfn() and users of that function such as
> virt_to_page() are supposed to pass a pointer to virtual
> memory, ideally a (void *) or other pointer. However since
> many architectures implement virt_to_pfn() as a macro,
> this function becomes polymorphic and accepts both a
> (unsigned long) and a (void *).

I wonder if there is merit to convert x86 to use an inline after this
goes in to prevent this polymorphic mistake?

> If we instead implement a proper virt_to_pfn(void *addr)
> function the following happens (occurred on arch/arm):
> 
>   mm/gup.c: In function '__get_user_pages_locked':
>   mm/gup.c:1599:49: warning: passing argument 1 of 'virt_to_pfn'
>     makes pointer from integer without a cast [-Wint-conversion]
>     pages[i] = virt_to_page(start);
> 
> Fix this with an explicit cast.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-mm@kvack.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  mm/gup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 551264407624..543c68da65f1 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1672,7 +1672,7 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
>  			goto finish_or_fault;
>  
>  		if (pages) {
> -			pages[i] = virt_to_page(start);
> +			pages[i] = virt_to_page((void *)start);

That 'start' is actually a userspace addres so it technically is a
__user pointer, but the missing context here is that this is a NOMMU
special function, so I guess it is right as is?

Still, it is NOP to what it is now so:

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH 4/5] mm: gup: Pass a pointer to virt_to_page()

[Linus Walleij]

On Fri, Jul 1, 2022 at 2:29 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Thu, Jun 30, 2022 at 10:41:23AM +0200, Linus Walleij wrote:
> > Functions that work on a pointer to virtual memory such as
> > virt_to_pfn() and users of that function such as
> > virt_to_page() are supposed to pass a pointer to virtual
> > memory, ideally a (void *) or other pointer. However since
> > many architectures implement virt_to_pfn() as a macro,
> > this function becomes polymorphic and accepts both a
> > (unsigned long) and a (void *).
>
> I wonder if there is merit to convert x86 to use an inline after this
> goes in to prevent this polymorphic mistake?

I have a patch like that, x86 uses the asm generic version IIUC:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-integrator.git/patch/?id=ed4befbf783c06820e9d4620c7fd254a36d608fe

There was also Xen:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-integrator.git/patch/?id=43ab4e5d6738273055dca21031e23b53b3721889

The plan is to trickle in a few of those after I fixed up all the mm
and drivers that were doing this with unsigned longs.
I managed to convert all architectures except one to use
static inlines for virt_to_pfn.

I was actually a bit surprised how few sites there were.

Yours,
Linus Walleij

[PATCH 5/5] mm: nommu: Pass a pointer to virt_to_page()

[Linus Walleij]

Functions that work on a pointer to virtual memory such as
virt_to_pfn() and users of that function such as
virt_to_page() are supposed to pass a pointer to virtual
memory, ideally a (void *) or other pointer. However since
many architectures implement virt_to_pfn() as a macro,
this function becomes polymorphic and accepts both a
(unsigned long) and a (void *).

If we instead implement a proper virt_to_pfn(void *addr)
function the following happens (occurred on arch/arm):

  mm/nommu.c: In function 'free_page_series':
  mm/nommu.c:501:50: warning: passing argument 1 of 'virt_to_pfn'
  makes pointer from integer without a cast [-Wint-conversion]
  struct page *page = virt_to_page(from);

Fix this with an explicit cast.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 mm/nommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/nommu.c b/mm/nommu.c
index 9d7afc2d959e..e819cbc21b39 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -500,7 +500,7 @@ static void delete_nommu_region(struct vm_region *region)
 static void free_page_series(unsigned long from, unsigned long to)
 {
 	for (; from < to; from += PAGE_SIZE) {
-		struct page *page = virt_to_page(from);
+		struct page *page = virt_to_page((void *)from);
 
 		atomic_long_dec(&mmap_pages_allocated);
 		put_page(page);
-- 
2.36.1

Re: [PATCH 0/5] mm: Pass pointers to page accessors

[Andrew Morton]

On Thu, 30 Jun 2022 10:41:19 +0200 Linus Walleij <linus.walleij@linaro.org> wrote:

> In a recent change to the Arm architecture with the end goal
> of removing highmem we need to convert virt_to_phys() and
> virt_to_pfn() to static inline functions.
> 
> This will make them strongly typed.
> 
> However since virt_to_* is always implemented as macros they
> have become polymorphic and accept both (void *) and
> e.g. unsigned long as arguments.
> 
> Other functions such as virt_to_page() simply wrap
> virt_to_pfn() and get affected indirectly.
> 
> To be able to proceed, patch mm to use (void *) as argument
> to affected functions in all instances.

It would be nice if someone were to teach x86 and others to use static
inlines.  Get rid of those stupid macros and improve coverage for your
changes.

Re: [PATCH 0/5] mm: Pass pointers to page accessors

[Linus Walleij]

On Sat, Jul 2, 2022 at 1:00 AM Andrew Morton <akpm@linux-foundation.org> wrote:

> It would be nice if someone were to teach x86 and others to use static
> inlines.  Get rid of those stupid macros and improve coverage for your
> changes.

I share your view on this.

I have patches for that, for virt_to_pfn() at least on all archs except
one.

Since it seems the MM maintainer likes the idea I will talk to Arnd
to have this merged and also possibly look at some more of these
macros.

Yours,
Linus Walleij