Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 1 Jun 2022 14:28:48 +0200
From:   Marco Elver <elver@google.com>
To:     andrey.konovalov@linux.dev
Cc:     Alexander Potapenko <glider@google.com>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Andrey Ryabinin <ryabinin.a.a@gmail.com>,
        kasan-dev@googlegroups.com,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Andrey Konovalov <andreyknvl@google.com>
Subject: Re: [PATCH 3/3] kasan: fix zeroing vmalloc memory with HW_TAGS
Message-ID: <YpdbgGjjz954Us/y@elver.google.com>
References: <4c76a95aff79723de76df146a10888a5a9196faf.1654011120.git.andreyknvl@google.com>
 <bbc30451228f670abeaf1b8aad678b9f6dda4ad3.1654011120.git.andreyknvl@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <bbc30451228f670abeaf1b8aad678b9f6dda4ad3.1654011120.git.andreyknvl@google.com>
User-Agent: Mutt/2.1.4 (2021-12-11)
Precedence: bulk

On Tue, May 31, 2022 at 05:43PM +0200, andrey.konovalov@linux.dev wrote:
> From: Andrey Konovalov <andreyknvl@google.com>
> 
> HW_TAGS KASAN skips zeroing page_alloc allocations backing vmalloc
> mappings via __GFP_SKIP_ZERO. Instead, these pages are zeroed via
> kasan_unpoison_vmalloc() by passing the KASAN_VMALLOC_INIT flag.
> 
> The problem is that __kasan_unpoison_vmalloc() does not zero pages
> when either kasan_vmalloc_enabled() or is_vmalloc_or_module_addr() fail.
> 
> Thus:
> 
> 1. Change __vmalloc_node_range() to only set KASAN_VMALLOC_INIT when
>    __GFP_SKIP_ZERO is set.
> 
> 2. Change __kasan_unpoison_vmalloc() to always zero pages when the
>    KASAN_VMALLOC_INIT flag is set.
> 
> 3. Add WARN_ON() asserts to check that KASAN_VMALLOC_INIT cannot be set
>    in other early return paths of __kasan_unpoison_vmalloc().
> 
> Also clean up the comment in __kasan_unpoison_vmalloc.
> 
> Fixes: 23689e91fb22 ("kasan, vmalloc: add vmalloc tagging for HW_TAGS")
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  mm/kasan/hw_tags.c | 30 ++++++++++++++++++++++--------
>  mm/vmalloc.c       | 10 +++++-----
>  2 files changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> index 9e1b6544bfa8..c0ec01eadf20 100644
> --- a/mm/kasan/hw_tags.c
> +++ b/mm/kasan/hw_tags.c
> @@ -263,21 +263,31 @@ void *__kasan_unpoison_vmalloc(const void *start, unsigned long size,
>  	u8 tag;
>  	unsigned long redzone_start, redzone_size;
>  
> -	if (!kasan_vmalloc_enabled())
> -		return (void *)start;
> +	if (!kasan_vmalloc_enabled() || !is_vmalloc_or_module_addr(start)) {
> +		struct page *page;
> +		const void *addr;
> +
> +		/* Initialize memory if required. */
> +

This whole block of code looks out-of-place in this function, since it's
not at all related to unpoisoning but a fallback if KASAN-vmalloc is off
but we still want to initialize the memory.

Maybe to ease readability here I'd change it to look like:


diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
index 11f661a2494b..227c20d09258 100644
--- a/mm/kasan/hw_tags.c
+++ b/mm/kasan/hw_tags.c
@@ -257,6 +257,21 @@ static void unpoison_vmalloc_pages(const void *addr, u8 tag)
 	}
 }
 
+/*
+ * Explicit initialization of pages if KASAN does not handle VM_ALLOC
+ * allocations.
+ */
+static void init_vmalloc_pages_explicit(const void *start, unsigned long size)
+{
+	const void *addr;
+
+	for (addr = start; addr < start + size; addr += PAGE_SIZE) {
+		struct page *page = virt_to_page(addr);
+
+		clear_highpage_kasan_tagged(page);
+	}
+}
+
 void *__kasan_unpoison_vmalloc(const void *start, unsigned long size,
 				kasan_vmalloc_flags_t flags)
 {
@@ -264,19 +279,8 @@ void *__kasan_unpoison_vmalloc(const void *start, unsigned long size,
 	unsigned long redzone_start, redzone_size;
 
 	if (!kasan_vmalloc_enabled() || !is_vmalloc_or_module_addr(start)) {
-		struct page *page;
-		const void *addr;
-
-		/* Initialize memory if required. */
-
-		if (!(flags & KASAN_VMALLOC_INIT))
-			return (void *)start;
-
-		for (addr = start; addr < start + size; addr += PAGE_SIZE) {
-			page = virt_to_page(addr);
-			clear_highpage_kasan_tagged(page);
-		}
-
+		if (flags & KASAN_VMALLOC_INIT)
+			init_vmalloc_pages_explicit(start, size);
 		return (void *)start;
 	}