| Message ID | 20170914223517.8242-11-pasha.tatashin@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | complete deferred page initialization | expand |
On Thu, Sep 14, 2017 at 06:35:16PM -0400, Pavel Tatashin wrote: > To optimize the performance of struct page initialization, > vmemmap_populate() will no longer zero memory. > > We must explicitly zero the memory that is allocated by vmemmap_populate() > for kasan, as this memory does not go through struct page initialization > path. > > Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com> > Reviewed-by: Steven Sistare <steven.sistare@oracle.com> > Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com> > Reviewed-by: Bob Picco <bob.picco@oracle.com> > --- > arch/arm64/mm/kasan_init.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/arch/arm64/mm/kasan_init.c b/arch/arm64/mm/kasan_init.c > index 81f03959a4ab..e78a9ecbb687 100644 > --- a/arch/arm64/mm/kasan_init.c > +++ b/arch/arm64/mm/kasan_init.c > @@ -135,6 +135,41 @@ static void __init clear_pgds(unsigned long start, > set_pgd(pgd_offset_k(start), __pgd(0)); > } > > +/* > + * Memory that was allocated by vmemmap_populate is not zeroed, so we must > + * zero it here explicitly. > + */ > +static void > +zero_vmemmap_populated_memory(void) > +{ > + struct memblock_region *reg; > + u64 start, end; > + > + for_each_memblock(memory, reg) { > + start = __phys_to_virt(reg->base); > + end = __phys_to_virt(reg->base + reg->size); > + > + if (start >= end) > + break; > + > + start = (u64)kasan_mem_to_shadow((void *)start); > + end = (u64)kasan_mem_to_shadow((void *)end); > + > + /* Round to the start end of the mapped pages */ > + start = round_down(start, SWAPPER_BLOCK_SIZE); > + end = round_up(end, SWAPPER_BLOCK_SIZE); > + memset((void *)start, 0, end - start); > + } > + > + start = (u64)kasan_mem_to_shadow(_text); > + end = (u64)kasan_mem_to_shadow(_end); > + > + /* Round to the start end of the mapped pages */ > + start = round_down(start, SWAPPER_BLOCK_SIZE); > + end = round_up(end, SWAPPER_BLOCK_SIZE); > + memset((void *)start, 0, end - start); > +} I really don't see the need to duplicate the existing logic to iterate over memblocks, calculate the addresses, etc. Why can't we just have a zeroing wrapper? e.g. something like the below. I really don't see why we couldn't have a generic function in core code to do this, even if vmemmap_populate() doesn't. Thanks, Mark. ---->8---- diff --git a/arch/arm64/mm/kasan_init.c b/arch/arm64/mm/kasan_init.c index 81f0395..698d065 100644 --- a/arch/arm64/mm/kasan_init.c +++ b/arch/arm64/mm/kasan_init.c @@ -135,6 +135,17 @@ static void __init clear_pgds(unsigned long start, set_pgd(pgd_offset_k(start), __pgd(0)); } +void kasan_populate_shadow(unsigned long shadow_start, unsigned long shadow_end, + nid_t nid) +{ + shadow_start = round_down(shadow_start, SWAPPER_BLOCK_SIZE); + shadow_end = round_up(shadow_end, SWAPPER_BLOCK_SIZE); + + vmemmap_populate(shadow_start, shadow_end, nid); + + memset((void *)shadow_start, 0, shadow_end - shadow_start); +} + void __init kasan_init(void) { u64 kimg_shadow_start, kimg_shadow_end; @@ -161,8 +172,8 @@ void __init kasan_init(void) clear_pgds(KASAN_SHADOW_START, KASAN_SHADOW_END); - vmemmap_populate(kimg_shadow_start, kimg_shadow_end, - pfn_to_nid(virt_to_pfn(lm_alias(_text)))); + kasah_populate_shadow(kimg_shadow_start, kimg_shadow_end, + pfn_to_nid(virt_to_pfn(lm_alias(_text)))); /* * vmemmap_populate() has populated the shadow region that covers the @@ -191,9 +202,9 @@ void __init kasan_init(void) if (start >= end) break; - vmemmap_populate((unsigned long)kasan_mem_to_shadow(start), - (unsigned long)kasan_mem_to_shadow(end), - pfn_to_nid(virt_to_pfn(start))); + kasan_populate_shadow((unsigned long)kasan_mem_to_shadow(start), + (unsigned long)kasan_mem_to_shadow(end), + pfn_to_nid(virt_to_pfn(start))); } /*
Hi Mark, Thank you for looking at this. We can't do this because page table is not set until cpu_replace_ttbr1() is called. So, we can't do memset() on this memory until then. Pasha
On Thu, Sep 14, 2017 at 09:30:28PM -0400, Pavel Tatashin wrote: > Hi Mark, > > Thank you for looking at this. We can't do this because page table is not > set until cpu_replace_ttbr1() is called. So, we can't do memset() on this > memory until then. I see. Sorry, I had missed that we were on the temporary tables at this point in time. I'm still not keen on duplicating the iteration. Can we split the vmemmap code so that we have a variant that takes a GFP? That way we could explicitly pass __GFP_ZERO for those cases where we want a zeroed page, and are happy to pay the cost of initialization. Thanks Mark.
Hi Mark, I had this option back upto version 3, where zero flag was passed into vmemmap_alloc_block(), but I was asked to remove it, because it required too many changes in other places. So, the current approach is cleaner, but the idea is that kasan should use its own version of vmemmap_populate() for both x86 and ARM, but I think it is outside of the scope of this work. See this comment from Ard Biesheuvel: https://lkml.org/lkml/2017/8/3/948 " KASAN uses vmemmap_populate as a convenience: kasan has nothing to do with vmemmap, but the function already existed and happened to do what KASAN requires. Given that that will no longer be the case, it would be far better to stop using vmemmap_populate altogether, and clone it into a KASAN specific version (with an appropriate name) with the zeroing folded into it. " If you think I should add these function in this project, than sure I can send a new version with kasanmap_populate() functions. Thank you, Pasha On 09/15/2017 04:38 PM, Mark Rutland wrote: > On Thu, Sep 14, 2017 at 09:30:28PM -0400, Pavel Tatashin wrote: >> Hi Mark, Thank you for looking at this. We can't do this because page >> table is not set until cpu_replace_ttbr1() is called. So, we can't do >> memset() on this memory until then. > I see. Sorry, I had missed that we were on the temporary tables at > this point in time. I'm still not keen on duplicating the iteration. > Can we split the vmemmap code so that we have a variant that takes a > GFP? That way we could explicitly pass __GFP_ZERO for those cases > where we want a zeroed page, and are happy to pay the cost of > initialization. Thanks Mark.
On Fri, Sep 15, 2017 at 05:20:59PM -0400, Pavel Tatashin wrote: > Hi Mark, > > I had this option back upto version 3, where zero flag was passed into > vmemmap_alloc_block(), but I was asked to remove it, because it required too > many changes in other places. Ok. Sorry for bringing back a point that had already been covered. > So, the current approach is cleaner, but the idea is that kasan should use > its own version of vmemmap_populate() for both x86 and ARM, but I think it is > outside of the scope of this work. I appreciate that this is unrelated to your ultimate goal, and that this is somewhat frustrating given the KASAN code is arguably abusing the vmemmap_populate() interface. However, I do think we need to migrate the KASAN code to a proper interface immediately, rather than making it worse in the interim. > If you think I should add these function in this project, than sure I can > send a new version with kasanmap_populate() functions. I would very much appreciate if you could send a version with a kasan_map_populate() interface. I'm more than happy to review/test that portion of the series, or to help if there's some problem which makes that difficult. Thanks, Mark.
diff --git a/arch/arm64/mm/kasan_init.c b/arch/arm64/mm/kasan_init.c index 81f03959a4ab..e78a9ecbb687 100644 --- a/arch/arm64/mm/kasan_init.c +++ b/arch/arm64/mm/kasan_init.c @@ -135,6 +135,41 @@ static void __init clear_pgds(unsigned long start, set_pgd(pgd_offset_k(start), __pgd(0)); } +/* + * Memory that was allocated by vmemmap_populate is not zeroed, so we must + * zero it here explicitly. + */ +static void +zero_vmemmap_populated_memory(void) +{ + struct memblock_region *reg; + u64 start, end; + + for_each_memblock(memory, reg) { + start = __phys_to_virt(reg->base); + end = __phys_to_virt(reg->base + reg->size); + + if (start >= end) + break; + + start = (u64)kasan_mem_to_shadow((void *)start); + end = (u64)kasan_mem_to_shadow((void *)end); + + /* Round to the start end of the mapped pages */ + start = round_down(start, SWAPPER_BLOCK_SIZE); + end = round_up(end, SWAPPER_BLOCK_SIZE); + memset((void *)start, 0, end - start); + } + + start = (u64)kasan_mem_to_shadow(_text); + end = (u64)kasan_mem_to_shadow(_end); + + /* Round to the start end of the mapped pages */ + start = round_down(start, SWAPPER_BLOCK_SIZE); + end = round_up(end, SWAPPER_BLOCK_SIZE); + memset((void *)start, 0, end - start); +} + void __init kasan_init(void) { u64 kimg_shadow_start, kimg_shadow_end; @@ -205,8 +240,15 @@ void __init kasan_init(void) pfn_pte(sym_to_pfn(kasan_zero_page), PAGE_KERNEL_RO)); memset(kasan_zero_page, 0, PAGE_SIZE); + cpu_replace_ttbr1(lm_alias(swapper_pg_dir)); + /* + * vmemmap_populate does not zero the memory, so we need to zero it + * explicitly + */ + zero_vmemmap_populated_memory(); + /* At this point kasan is fully initialized. Enable error messages */ init_task.kasan_depth = 0; pr_info("KernelAddressSanitizer initialized\n");