Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Daniel Axtens <dja@axtens.net>
To:     Marco Elver <elver@google.com>,
        Kefeng Wang <wangkefeng.wang@huawei.com>
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Andrey Ryabinin <ryabinin.a.a@gmail.com>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        kasan-dev@googlegroups.com, linux-mm@kvack.org
Subject: Re: [PATCH -next 3/3] kasan: arm64: Fix pcpu_page_first_chunk crash
 with KASAN_VMALLOC
In-Reply-To: <YOMfcE7V7lSE3N/z@elver.google.com>
References: <20210705111453.164230-1-wangkefeng.wang@huawei.com>
 <20210705111453.164230-4-wangkefeng.wang@huawei.com>
 <YOMfcE7V7lSE3N/z@elver.google.com>
Date:   Tue, 06 Jul 2021 10:04:31 +1000
Message-ID: <87bl7gxq7k.fsf@dja-thinkpad.axtens.net>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hi,

Marco Elver <elver@google.com> writes:

> On Mon, Jul 05, 2021 at 07:14PM +0800, Kefeng Wang wrote:
> [...]
>> +#ifdef CONFIG_KASAN_VMALLOC
>> +void __init __weak kasan_populate_early_vm_area_shadow(void *start,
>> +						       unsigned long size)
>
> This should probably not be __weak, otherwise you now have 2 __weak
> functions.
>
>> +{
>> +	unsigned long shadow_start, shadow_end;
>> +
>> +	if (!is_vmalloc_or_module_addr(start))
>> +		return;
>> +
>> +	shadow_start = (unsigned long)kasan_mem_to_shadow(start);
>> +	shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
>> +	shadow_end = (unsigned long)kasan_mem_to_shadow(start + size);
>> +	shadow_end = ALIGN(shadow_end, PAGE_SIZE);
>> +	kasan_map_populate(shadow_start, shadow_end,
>> +			   early_pfn_to_nid(virt_to_pfn(start)));
>> +}
>> +#endif
>
> This function looks quite generic -- would any of this also apply to
> other architectures? I see that ppc and sparc at least also define
> CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK.

So I checked with my latest KASAN ppc64 series and my code also breaks
in a very similar way if you boot with percpu_alloc=page. It's not
something I knew about or tested with before!

Unfortunately kasan_map_populate - despite having a very
generic-sounding name - is actually arm64 specific. I don't know if
kasan_populate_early_shadow (which is generic) would be able to fill the
role or not. If we could keep it generic that would be better.

It looks like arm64 does indeed populate the kasan_early_shadow_p{te,md..}
values, but I don't really understand what it's doing - is it possible
to use the generic kasan_populate_early_shadow on arm64?

If so, should we put the call inside of vm_area_register_early?

Kind regards,
Daniel

>
>>  void __init kasan_init(void)
>>  {
>>  	kasan_init_shadow();
>> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
>> index 5310e217bd74..79d3895b0240 100644
>> --- a/include/linux/kasan.h
>> +++ b/include/linux/kasan.h
>> @@ -49,6 +49,8 @@ extern p4d_t kasan_early_shadow_p4d[MAX_PTRS_PER_P4D];
>>  int kasan_populate_early_shadow(const void *shadow_start,
>>  				const void *shadow_end);
>>  
>> +void kasan_populate_early_vm_area_shadow(void *start, unsigned long size);
>> +
>>  static inline void *kasan_mem_to_shadow(const void *addr)
>>  {
>>  	return (void *)((unsigned long)addr >> KASAN_SHADOW_SCALE_SHIFT)
>> diff --git a/mm/kasan/init.c b/mm/kasan/init.c
>> index cc64ed6858c6..d39577d088a1 100644
>> --- a/mm/kasan/init.c
>> +++ b/mm/kasan/init.c
>> @@ -279,6 +279,11 @@ int __ref kasan_populate_early_shadow(const void *shadow_start,
>>  	return 0;
>>  }
>>  
>> +void __init __weak kasan_populate_early_vm_area_shadow(void *start,
>> +						       unsigned long size)
>> +{
>> +}
>
> I'm just wondering if this could be a generic function, perhaps with an
> appropriate IS_ENABLED() check of a generic Kconfig option
> (CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK ?) to short-circuit it, if it's
> not only an arm64 problem.
>
> But I haven't looked much further, so would appeal to you to either
> confirm or reject this idea.
>
> Thanks,
> -- Marco