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

[Jason Gunthorpe <jgg@ziepe.ca>]

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