| Message ID | bbc30451228f670abeaf1b8aad678b9f6dda4ad3.1654011120.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] mm: rename kernel_init_free_pages to kernel_init_pages | expand |
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; }
On Wed, Jun 1, 2022 at 2:28 PM Marco Elver <elver@google.com> wrote: > > 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: Sounds good, will do in v2! Thanks!
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. */ + + if (!(flags & KASAN_VMALLOC_INIT)) + return (void *)start; + + for (addr = start; addr < start + size; addr += PAGE_SIZE) { + page = virt_to_page(addr); + clear_highpage_tagged(page); + } - if (!is_vmalloc_or_module_addr(start)) return (void *)start; + } /* - * Skip unpoisoning and assigning a pointer tag for non-VM_ALLOC - * mappings as: + * Don't tag non-VM_ALLOC mappings, as: * * 1. Unlike the software KASAN modes, hardware tag-based KASAN only * supports tagging physical memory. Therefore, it can only tag a * single mapping of normal physical pages. * 2. Hardware tag-based KASAN can only tag memory mapped with special - * mapping protection bits, see arch_vmalloc_pgprot_modify(). + * mapping protection bits, see arch_vmap_pgprot_tagged(). * As non-VM_ALLOC mappings can be mapped outside of vmalloc code, * providing these bits would require tracking all non-VM_ALLOC * mappers. @@ -289,15 +299,19 @@ void *__kasan_unpoison_vmalloc(const void *start, unsigned long size, * * For non-VM_ALLOC allocations, page_alloc memory is tagged as usual. */ - if (!(flags & KASAN_VMALLOC_VM_ALLOC)) + if (!(flags & KASAN_VMALLOC_VM_ALLOC)) { + WARN_ON(flags & KASAN_VMALLOC_INIT); return (void *)start; + } /* * Don't tag executable memory. * The kernel doesn't tolerate having the PC register tagged. */ - if (!(flags & KASAN_VMALLOC_PROT_NORMAL)) + if (!(flags & KASAN_VMALLOC_PROT_NORMAL)) { + WARN_ON(flags & KASAN_VMALLOC_INIT); return (void *)start; + } tag = kasan_random_tag(); start = set_tag(start, tag); diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 07db42455dd4..0adf4aa1514d 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -3168,15 +3168,15 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align, /* * Mark the pages as accessible, now that they are mapped. - * The init condition should match the one in post_alloc_hook() - * (except for the should_skip_init() check) to make sure that memory - * is initialized under the same conditions regardless of the enabled - * KASAN mode. + * The condition for setting KASAN_VMALLOC_INIT should complement the + * one in post_alloc_hook() with regards to the __GFP_SKIP_ZERO check + * to make sure that memory is initialized under the same conditions. * Tag-based KASAN modes only assign tags to normal non-executable * allocations, see __kasan_unpoison_vmalloc(). */ kasan_flags |= KASAN_VMALLOC_VM_ALLOC; - if (!want_init_on_free() && want_init_on_alloc(gfp_mask)) + if (!want_init_on_free() && want_init_on_alloc(gfp_mask) && + (gfp_mask & __GFP_SKIP_ZERO)) kasan_flags |= KASAN_VMALLOC_INIT; /* KASAN_VMALLOC_PROT_NORMAL already set if required. */ area->addr = kasan_unpoison_vmalloc(area->addr, real_size, kasan_flags);