Re: [PATCH] powerpc: Fix signature of pfn_to_kaddr()

[Christophe Leroy]

Le 07/11/2023 à 06:57, Michael Ellerman a écrit :
> Linus Walleij <linus.walleij@xxxxxxxxxx> 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@xxxxxxxxx>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202311061940.4pBrm44u-lkp@xxxxxxxxx/
>   
> 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