Re: [PATCH v2 09/14] mm: Pcpu chunk address tag reset

[Maciej Wieczor-Retman]

On 2025-03-20 at 18:39:35 +0100, Andrey Ryabinin wrote:
>On Tue, Feb 18, 2025 at 9:19 AM Maciej Wieczor-Retman
><maciej.wieczor-retman@xxxxxxxxx> wrote:
>>
>> The problem presented here is related to NUMA systems and tag-based
>> KASAN mode. Getting to it can be explained in the following points:
>>
>>         1. A new chunk is created with pcpu_create_chunk() and
>>            vm_structs are allocated. On systems with one NUMA node only
>>            one is allocated, but with more NUMA nodes at least a second
>>            one will be allocated too.
>>
>>         2. chunk->base_addr is assigned the modified value of
>>            vms[0]->addr and thus inherits the tag of this allocated
>>            structure.
>>
>>         3. In pcpu_alloc() for each possible cpu pcpu_chunk_addr() is
>>            executed which calculates per cpu pointers that correspond to
>>            the vms structure addresses. The calculations are based on
>>            adding an offset from a table to chunk->base_addr.
>>
>> Here the problem presents itself since for addresses based on vms[1] and
>> up, the tag will be different than the ones based on vms[0] (base_addr).
>> The tag mismatch happens and an error is reported.
>>
>> Reset the base_addr tag, since it will disable tag checks for pointers
>> derived arithmetically from base_addr that would inherit its tag.
>>
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@xxxxxxxxx>
>> ---
>>  mm/percpu-vm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/percpu-vm.c b/mm/percpu-vm.c
>> index cd69caf6aa8d..e13750d804f7 100644
>> --- a/mm/percpu-vm.c
>> +++ b/mm/percpu-vm.c
>> @@ -347,7 +347,7 @@ static struct pcpu_chunk *pcpu_create_chunk(gfp_t gfp)
>>         }
>>
>>         chunk->data = vms;
>> -       chunk->base_addr = vms[0]->addr - pcpu_group_offsets[0];
>> +       chunk->base_addr = kasan_reset_tag(vms[0]->addr) - pcpu_group_offsets[0];
>
>This looks like a generic tags mode bug. I mean that arm64 is also
>affected by this.
>I assume it just wasn't noticed before because arm64 with multiple
>NUMAs are much less common.

That was my assumption as well

>
>With this change tag-mode KASAN won't be able to catch bugus accesses
>to pcpu areas.
>I'm thinking it would be better to fix this on the pcpu_get_vm_areas()
>area side by replacing
>this
>    for (area = 0; area < nr_vms; area++)
>        vms[area]->addr = kasan_unpoison_vmalloc(vms[area]->addr,
>                                             vms[area]->size,
>KASAN_VMALLOC_PROT_NORMAL);
>
>with something like
>    kasan_unpoison_vmap_areas(vms, nr_vms);
>which will unpoison all areas using the same tag.
>
>Thoughts?

I was looking for a solution that would preserve the individual tags for each
area. But so far I didn't come up with anything that would work. I first assumed
that some per-cpu pointers would always be derived from from vms[0]->addr, and
some from vms[1]->addr. For example first half of the cpus would be tied to
vms[0]->addr, second half to vms[1]->addr (since areas are allocated based on
the NUMA layout as far as I know from the docs). But that didn't work and
pointers popped up that were from the second half of the cores but had tags from
the vms[0]->addr.

I think unpoisoning all the areas with the same tag would work, but could it
create issues for out-of-bounds accesses? From my testing the areas are nowhere
near each other but I don't know if that's a given or just by accident?

TLDR: can the areas be adjecent and therefor break OOB checking?

-- 
Kind regards
Maciej Wieczór-Retman