diff mbox series

powerpc: Fix signature of pfn_to_kaddr()

Message ID 20231106-virt-to-pfn-fix-ppc-v1-1-93197a7ccab4@linaro.org
State New
Headers
Series powerpc: Fix signature of pfn_to_kaddr() |

Commit Message

Linus Walleij Nov. 6, 2023, 1:44 p.m. UTC 
  There is a const in the returned value from pfn_to_kaddr()
but there are consumers that want to modify the result
and the generic function pfn_to_virt() in <asm-generic/page.h>
does allow this, so let's relax this requirement and do not
make the returned value const.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202311061940.4pBrm44u-lkp@intel.com/
Fixes: 58b6fed89ab0 ("powerpc: Make virt_to_pfn() a static inline")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
The remaining warnings from the test robot appear a bit bogus.
If someone knows what to do about them, please help. The warnings
often properly uncovers problems that have been around forever
due to these functions being disguised as macros.
---
 arch/powerpc/include/asm/page.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


---
base-commit: d2f51b3516dade79269ff45eae2a7668ae711b25
change-id: 20231106-virt-to-pfn-fix-ppc-d91de47c6017

Best regards,

Comments

Michael Ellerman Nov. 7, 2023, 5:57 a.m. UTC | #1 
Linus Walleij <linus.walleij@linaro.org> writes:
> There is a const in the returned value from pfn_to_kaddr()
> but there are consumers that want to modify the result
> and the generic function pfn_to_virt() in <asm-generic/page.h>
> does allow this, so let's relax this requirement and do not
> make the returned value const.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202311061940.4pBrm44u-lkp@intel.com/
 
I'm struggling to connect the removal of const with those bug reports.
It looks like all those warnings are about 0xc000000000000000 being
outside the range of unsigned long when building 32-bit.

Is it the right bug report link?

The current signature of:

  static inline const void *pfn_to_kaddr(unsigned long pfn) ...

seems OK to me.

It allows code like:

  const void *p = pfn_to_kaddr(pfn);
  p++;

But errors for:

  const void *p = pfn_to_kaddr(pfn);
  unsigned long *q = p;
  *q = 0;

  error: initialization discards ‘const’ qualifier from pointer target type


Having said that it looks like almost every caller of pfn_to_kaddr()
casts the result to unsigned long, so possibly that would be the better
return type in terms of the actual usage. Although that would conflict
with __va() which returns void * :/

cheers
Linus Walleij Nov. 7, 2023, 8:06 a.m. UTC | #2 
On Tue, Nov 7, 2023 at 6:57 AM Michael Ellerman <mpe@ellerman.id.au> wrote:

> I'm struggling to connect the removal of const with those bug reports.
> It looks like all those warnings are about 0xc000000000000000 being
> outside the range of unsigned long when building 32-bit.

Aha right. I wonder what actually causes that.

> Is it the right bug report link?

Yeah I'm just bad at understanding these reports.

> The current signature of:
>
>   static inline const void *pfn_to_kaddr(unsigned long pfn) ...
>
> seems OK to me.

OK then, drop this patch.

Yours,
Linus Walleij
Christophe Leroy Nov. 8, 2023, 7:24 p.m. UTC | #3 
Le 07/11/2023 à 06:57, Michael Ellerman a écrit :
> Linus Walleij <linus.walleij@linaro.org> writes:
>> There is a const in the returned value from pfn_to_kaddr()
>> but there are consumers that want to modify the result
>> and the generic function pfn_to_virt() in <asm-generic/page.h>
>> does allow this, so let's relax this requirement and do not
>> make the returned value const.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202311061940.4pBrm44u-lkp@intel.com/
>   
> I'm struggling to connect the removal of const with those bug reports.
> It looks like all those warnings are about 0xc000000000000000 being
> outside the range of unsigned long when building 32-bit.
> 
> Is it the right bug report link?
> 
> The current signature of:
> 
>    static inline const void *pfn_to_kaddr(unsigned long pfn) ...
> 
> seems OK to me.
> 
> It allows code like:
> 
>    const void *p = pfn_to_kaddr(pfn);
>    p++;
> 
> But errors for:
> 
>    const void *p = pfn_to_kaddr(pfn);
>    unsigned long *q = p;
>    *q = 0;
> 
>    error: initialization discards ‘const’ qualifier from pointer target type
> 
> 
> Having said that it looks like almost every caller of pfn_to_kaddr()
> casts the result to unsigned long, so possibly that would be the better
> return type in terms of the actual usage. Although that would conflict
> with __va() which returns void * :/

I think the return type is right, and callers should be fixed to avoid 
the cast to unsigned long.

As an exemple, the only core generic caller is kasan, with the following:

	start_kaddr = (unsigned long)pfn_to_kaddr(mem_data->start_pfn);
	shadow_start = (unsigned long)kasan_mem_to_shadow((void *)start_kaddr);
	...
	if (WARN_ON(mem_data->nr_pages % KASAN_GRANULE_SIZE) ||
		WARN_ON(start_kaddr % KASAN_MEMORY_PER_SHADOW_PAGE))
		return NOTIFY_BAD;

I think start_kaddr should be declared as void* instead of 
unsigned_long, and the cast should only be performed inside the WARN_ON()


In powerpc we have vmalloc_to_phys() with:

	return __pa(pfn_to_kaddr(pfn)) + offset_in_page(va);

 From my point of view that's the correct way to go, with no casts.


Christophe
Michael Ellerman Nov. 10, 2023, 6:16 a.m. UTC | #4 
Linus Walleij <linus.walleij@linaro.org> writes:
> On Tue, Nov 7, 2023 at 6:57 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
>> I'm struggling to connect the removal of const with those bug reports.
>> It looks like all those warnings are about 0xc000000000000000 being
>> outside the range of unsigned long when building 32-bit.
>
> Aha right. I wonder what actually causes that.

It is the 32-bit VDSO being built:

      VDSO32C arch/powerpc/kernel/vdso/vgettimeofday-32.o
    In file included from <built-in>:4:
    In file included from /home/michael/linux/lib/vdso/gettimeofday.c:5:
    In file included from ../include/vdso/datapage.h:137:
    In file included from ../arch/powerpc/include/asm/vdso/gettimeofday.h:7:
    ../arch/powerpc/include/asm/page.h:230:9: warning: result of comparison of constant 13835058055282163712 with expression of type 'unsigned long' is always true [-Wtautological-constant-out-of-range-compare]
      230 |         return __pa(kaddr) >> PAGE_SHIFT;
          |                ^~~~~~~~~~~
    ../arch/powerpc/include/asm/page.h:217:37: note: expanded from macro '__pa'
      217 |         VIRTUAL_WARN_ON((unsigned long)(x) < PAGE_OFFSET);              \
          |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
    ../arch/powerpc/include/asm/page.h:202:73: note: expanded from macro 'VIRTUAL_WARN_ON'
      202 | #define VIRTUAL_WARN_ON(x)      WARN_ON(IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && (x))
          |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
    ../arch/powerpc/include/asm/bug.h:88:25: note: expanded from macro 'WARN_ON'
       88 |         int __ret_warn_on = !!(x);                              \
          |                                ^


Which is a bit of a frankenstein, because we're building 32-bit VDSO
code for a 64-bit kernel, and using some of the kernel headers for that.

So the warning is correct, we are doing a tautological comparison.
Except that we're not actually using that code in the VDSO, it's just
included in the VDSO because it needs PAGE_SHIFT.

Anyway none of that is your fault, you just had the misfortune of
touching page.h :)

I think I see a way to clean it up. Will send a patch.

cheers
diff mbox series

Patch

diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index e5fcc79b5bfb..5243e48dc13a 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -230,7 +230,7 @@  static inline unsigned long virt_to_pfn(const void *kaddr)
 	return __pa(kaddr) >> PAGE_SHIFT;
 }
 
-static inline const void *pfn_to_kaddr(unsigned long pfn)
+static inline void *pfn_to_kaddr(unsigned long pfn)
 {
 	return __va(pfn << PAGE_SHIFT);
 }