Re: [PATCH v2 28/34] kasan, vmalloc: add vmalloc support to HW_TAGS

[Vincenzo Frascino <vincenzo.frascino@arm.com>]

Hi Andrey,

On 12/6/21 9:44 PM, andrey.konovalov@linux.dev wrote:
> From: Andrey Konovalov <andreyknvl@google.com>
> 
> This patch adds vmalloc tagging support to HW_TAGS KASAN.
> 

Can we reorganize the patch description in line with what I commented on patch 24?

> The key difference between HW_TAGS and the other two KASAN modes
> when it comes to vmalloc: HW_TAGS KASAN can only assign tags to
> physical memory. The other two modes have shadow memory covering
> every mapped virtual memory region.
> 
> This patch makes __kasan_unpoison_vmalloc() for HW_TAGS KASAN:
> 
> - Skip non-VM_ALLOC mappings as HW_TAGS KASAN can only tag a single
>   mapping of normal physical memory; see the comment in the function.
> - Generate a random tag, tag the returned pointer and the allocation,
>   and initialize the allocation at the same time.
> - Propagate the tag into the page stucts to allow accesses through
>   page_address(vmalloc_to_page()).
> 
> The rest of vmalloc-related KASAN hooks are not needed:
> 
> - The shadow-related ones are fully skipped.
> - __kasan_poison_vmalloc() is kept as a no-op with a comment.
> 
> Poisoning and zeroing of physical pages that are backing vmalloc()
> allocations are skipped via __GFP_SKIP_KASAN_UNPOISON and
> __GFP_SKIP_ZERO: __kasan_unpoison_vmalloc() does that instead.
> 
> This patch allows enabling CONFIG_KASAN_VMALLOC with HW_TAGS
> and adjusts CONFIG_KASAN_VMALLOC description:
> 
> - Mention HW_TAGS support.
> - Remove unneeded internal details: they have no place in Kconfig
>   description and are already explained in the documentation.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> Co-developed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> 
> ---
> 
> Changes v1->v2:
> - Allow enabling CONFIG_KASAN_VMALLOC with HW_TAGS in this patch.
> - Move memory init for page_alloc pages backing vmalloc() into
>   kasan_unpoison_vmalloc().
> ---
>  include/linux/kasan.h | 30 +++++++++++++--
>  lib/Kconfig.kasan     | 20 +++++-----
>  mm/kasan/hw_tags.c    | 89 +++++++++++++++++++++++++++++++++++++++++++
>  mm/kasan/shadow.c     | 11 +++++-
>  mm/vmalloc.c          | 32 +++++++++++++---
>  5 files changed, 162 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 6a2619759e93..0bdc2b824b9c 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -417,19 +417,40 @@ static inline void kasan_init_hw_tags(void) { }
>  
>  #ifdef CONFIG_KASAN_VMALLOC
>  
> +#if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
> +
>  void kasan_populate_early_vm_area_shadow(void *start, unsigned long size);
>  int kasan_populate_vmalloc(unsigned long addr, unsigned long size);
>  void kasan_release_vmalloc(unsigned long start, unsigned long end,
>  			   unsigned long free_region_start,
>  			   unsigned long free_region_end);
>  
> +#else /* CONFIG_KASAN_GENERIC || CONFIG_KASAN_SW_TAGS */
> +
> +static inline void kasan_populate_early_vm_area_shadow(void *start,
> +						       unsigned long size)
> +{ }
> +static inline int kasan_populate_vmalloc(unsigned long start,
> +					unsigned long size)
> +{
> +	return 0;
> +}
> +static inline void kasan_release_vmalloc(unsigned long start,
> +					 unsigned long end,
> +					 unsigned long free_region_start,
> +					 unsigned long free_region_end) { }
> +
> +#endif /* CONFIG_KASAN_GENERIC || CONFIG_KASAN_SW_TAGS */
> +
>  void * __must_check __kasan_unpoison_vmalloc(const void *start,
> -					     unsigned long size);
> +					     unsigned long size,
> +					     bool vm_alloc, bool init);
>  static __always_inline void * __must_check kasan_unpoison_vmalloc(
> -					const void *start, unsigned long size)
> +					const void *start, unsigned long size,
> +					bool vm_alloc, bool init)

Can we replace booleans with enumerations? It should make the code clearer on
the calling site.

...

With these changes:

Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

---

Regards,
Vincenzo