Re: [PATCH v3 13/14] mm: Unpoison pcpu chunks with base address tag

[Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>]

On 2025-04-04 at 11:08:12 -0700, Dave Hansen wrote:
>On 4/4/25 06:14, Maciej Wieczor-Retman 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.
>> 
>> Unpoison all the vms[]->addr with the same tag to resolve the mismatch.
>
>I think there's a bit too much superfluous information in there. For
>instance, it's not important to talk about how or why there can be more
>than one chunk, just say there _can_ be more than one.
>
>	1. There can be more than one chunk
>	2. The chunks are virtually contiguous
>	3. Since they are virtually contiguous, the chunks are all
>	   addressed from a single base address
>	4. The base address has a tag
>	5. The base address points at the first chunk and thus inherits
>	   the tag of the first chunk
>	6. The subsequent chunks will be accessed with the tag from the
>	   first chunk
>	7. Thus, the subsequent chunks need to have their tag set to
>	   match that of the first chunk.
>
>Right?

They don't seem to be virtuall contiguous. At least from testing on a live
system, QEMU and Simics I never saw any be contiguous. And I double checked
today too. But your version is nice, I'll just drop 2 and 3 and I think it still
will make sense, right?

>
>> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
>> index 54481f8c30c5..bd033b2ba383 100644
>> --- a/include/linux/kasan.h
>> +++ b/include/linux/kasan.h
>> @@ -613,6 +613,13 @@ static __always_inline void kasan_poison_vmalloc(const void *start,
>>  		__kasan_poison_vmalloc(start, size);
>>  }
>>  
>> +void __kasan_unpoison_vmap_areas(struct vm_struct **vms, int nr_vms);
>> +static __always_inline void kasan_unpoison_vmap_areas(struct vm_struct **vms, int nr_vms)
>> +{
>> +	if (kasan_enabled())
>> +		__kasan_unpoison_vmap_areas(vms, nr_vms);
>> +}
>> +
>>  #else /* CONFIG_KASAN_VMALLOC */
>>  
>>  static inline void kasan_populate_early_vm_area_shadow(void *start,
>> @@ -637,6 +644,9 @@ static inline void *kasan_unpoison_vmalloc(const void *start,
>>  static inline void kasan_poison_vmalloc(const void *start, unsigned long size)
>>  { }
>>  
>> +static inline void kasan_unpoison_vmap_areas(struct vm_struct **vms, int nr_vms)
>> +{ }
>> +
>>  #endif /* CONFIG_KASAN_VMALLOC */
>>  
>>  #if (defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)) && \
>> diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
>> index 88d1c9dcb507..9496f256bc0f 100644
>> --- a/mm/kasan/shadow.c
>> +++ b/mm/kasan/shadow.c
>> @@ -582,6 +582,17 @@ void __kasan_poison_vmalloc(const void *start, unsigned long size)
>>  	kasan_poison(start, size, KASAN_VMALLOC_INVALID, false);
>>  }
>>  
>> +void __kasan_unpoison_vmap_areas(struct vm_struct **vms, int nr_vms)
>> +{
>> +	int area;
>> +
>> +	for (area = 0 ; area < nr_vms ; area++) {
>> +		kasan_poison(vms[area]->addr, vms[area]->size,
>> +			     arch_kasan_get_tag(vms[0]->addr), false);
>> +		arch_kasan_set_tag(vms[area]->addr, arch_kasan_get_tag(vms[0]->addr));
>> +	}
>> +}
>
>-ENOCOMMENTS

Right, I'll add a description why that's needed.

>
>>  #else /* CONFIG_KASAN_VMALLOC */
>>  
>>  int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>> index 61981ee1c9d2..fbd56bf8aeb2 100644
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -4783,8 +4783,7 @@ struct vm_struct **pcpu_get_vm_areas(const unsigned long *offsets,
>>  	 * non-VM_ALLOC mappings, see __kasan_unpoison_vmalloc().
>>  	 */
>>  	for (area = 0; area < nr_vms; area++)
>> -		vms[area]->addr = kasan_unpoison_vmalloc(vms[area]->addr,
>> -				vms[area]->size, KASAN_VMALLOC_PROT_NORMAL);
>> +		kasan_unpoison_vmap_areas(vms, nr_vms);
>>  
>>  	kfree(vas);
>>  	return vms;
>
>So, the right way to do this is refactor, first, then add your changes
>after. This really wants to be two patches.

Sure, I'll try splitting it.

-- 
Kind regards
Maciej Wieczór-Retman