Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1181775 > unrolled thread

Re: [PATCH v2 5/5] arm64: add KASan support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 5/5] arm64: add KASan support Andrey Ryabinin <a.ryabinin@samsung.com> - 2015-07-10 19:20 +0200
    Re: [PATCH v2 5/5] arm64: add KASan support Catalin Marinas <catalin.marinas@arm.com> - 2015-07-14 17:10 +0200
      Re: [PATCH v2 5/5] arm64: add KASan support Andrey Ryabinin <a.ryabinin@samsung.com> - 2015-07-15 11:00 +0200
        Re: [PATCH v2 5/5] arm64: add KASan support Andrey Ryabinin <a.ryabinin@samsung.com> - 2015-07-16 17:40 +0200
          Re: [PATCH v2 5/5] arm64: add KASan support Catalin Marinas <catalin.marinas@arm.com> - 2015-07-16 18:10 +0200
            Re: [PATCH v2 5/5] arm64: add KASan support Andrey Ryabinin <a.ryabinin@samsung.com> - 2015-07-17 15:20 +0200

#1181775 — Re: [PATCH v2 5/5] arm64: add KASan support

>>  	select HAVE_ARCH_KGDB
>>  	select HAVE_ARCH_SECCOMP_FILTER
>>  	select HAVE_ARCH_TRACEHOOK
>> @@ -119,6 +120,12 @@ config GENERIC_CSUM
>>  config GENERIC_CALIBRATE_DELAY
>>  	def_bool y
>>  
>> +config KASAN_SHADOW_OFFSET
>> +	hex
>> +	default 0xdfff200000000000 if ARM64_VA_BITS_48
>> +	default 0xdffffc8000000000 if ARM64_VA_BITS_42
>> +	default 0xdfffff9000000000 if ARM64_VA_BITS_39
>> +
> 
> How were these numbers generated? I can probably guess but we need a
> comment in this file and a BUILD_BUG elsewhere (kasan_init.c) if we
> change the memory map and they no longer match.
> 

Ok, will do.

Probably the simplest way to get this number is:
	KASAN_SHADOW_END - (1ULL << (64 - 3))

64 is number of bits in pointer, 3 is KASAN_SHADOW_SCALE_SHIFT,
so [KASAN_SHADOW_OFFSET, KASAN_SHADOW_END] covers [0, -1ULL] addresses.


>> diff --git a/arch/arm64/include/asm/kasan.h b/arch/arm64/include/asm/kasan.h
>> new file mode 100644
>> index 0000000..65ac50d
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/kasan.h
>> @@ -0,0 +1,24 @@
>> +#ifndef __ASM_KASAN_H
>> +#define __ASM_KASAN_H
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +#ifdef CONFIG_KASAN
>> +
>> +#include <asm/memory.h>
>> +
>> +/*
>> + * KASAN_SHADOW_START: beginning of the kernel virtual addresses.
>> + * KASAN_SHADOW_END: KASAN_SHADOW_START + 1/8 of kernel virtual addresses.
>> + */
>> +#define KASAN_SHADOW_START      (UL(0xffffffffffffffff) << (VA_BITS))
>> +#define KASAN_SHADOW_END        (KASAN_SHADOW_START + (1UL << (VA_BITS - 3)))
> 
> Can you define a VA_START in asm/memory.h so that we avoid this long
> list of f's here and in pgtable.h?
> 

Sure, will do.

> Another BUILD_BUG we need is to ensure that KASAN_SHADOW_START/END
> covers an exact number of pgd entries, otherwise the logic in
> kasan_init.c can go wrong (it seems to be the case in all VA_BITS
> configurations but just in case we forget about this requirement in the
> future).
> 
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index bd5db28..8700f66 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -40,7 +40,14 @@
>>   *	fixed mappings and modules
>>   */
>>  #define VMEMMAP_SIZE		ALIGN((1UL << (VA_BITS - PAGE_SHIFT)) * sizeof(struct page), PUD_SIZE)
>> +
>> +#ifndef CONFIG_KASAN
>>  #define VMALLOC_START		(UL(0xffffffffffffffff) << VA_BITS)
> 
> And here we could just use VA_START.
> 
>> +#else
>> +#include <asm/kasan.h>
>> +#define VMALLOC_START		KASAN_SHADOW_END
>> +#endif
> 
> We could add a SZ_64K guard page here (just in case, the KASan shadow
> probably never reaches KASAN_SHADOW_END).
> 

Ok.

>> diff --git a/arch/arm64/include/asm/string.h b/arch/arm64/include/asm/string.h
>> index 64d2d48..bff522c 100644
>> --- a/arch/arm64/include/asm/string.h
>> +++ b/arch/arm64/include/asm/string.h
>> @@ -36,17 +36,33 @@ extern __kernel_size_t strnlen(const char *, __kernel_size_t);
>>  
>>  #define __HAVE_ARCH_MEMCPY
>>  extern void *memcpy(void *, const void *, __kernel_size_t);
>> +extern void *__memcpy(void *, const void *, __kernel_size_t);
>>  
>>  #define __HAVE_ARCH_MEMMOVE
>>  extern void *memmove(void *, const void *, __kernel_size_t);
>> +extern void *__memmove(void *, const void *, __kernel_size_t);
>>  
>>  #define __HAVE_ARCH_MEMCHR
>>  extern void *memchr(const void *, int, __kernel_size_t);
>>  
>>  #define __HAVE_ARCH_MEMSET
>>  extern void *memset(void *, int, __kernel_size_t);
>> +extern void *__memset(void *, int, __kernel_size_t);
>>  
>>  #define __HAVE_ARCH_MEMCMP
>>  extern int memcmp(const void *, const void *, size_t);
>>  
>> +
>> +#if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)
>> +
>> +/*
>> + * For files that not instrumented (e.g. mm/slub.c) we
> 
> Missing an "are".
> 
>> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
>> index dcd06d1..cfe5ea5 100644
>> --- a/arch/arm64/include/asm/thread_info.h
>> +++ b/arch/arm64/include/asm/thread_info.h
>> @@ -24,10 +24,18 @@
>>  #include <linux/compiler.h>
>>  
>>  #ifndef CONFIG_ARM64_64K_PAGES
>> +#ifndef CONFIG_KASAN
>>  #define THREAD_SIZE_ORDER	2
>> +#else
>> +#define THREAD_SIZE_ORDER	3
>> +#endif
>>  #endif
>>  
>> +#ifndef CONFIG_KASAN
>>  #define THREAD_SIZE		16384
>> +#else
>> +#define THREAD_SIZE		32768
>> +#endif
>>  #define THREAD_START_SP		(THREAD_SIZE - 16)
> 
> Have you actually seen it failing with the 16KB THREAD_SIZE? You may run
> into other problems with 8 4KB pages per stack.
> 

Actually no, so I guess that we could try with 16K.

I've seen it failing on ARM32 with 8K stack (we use some old version of kasan for our ARM kernels),
but that's a different story


>>  #ifndef __ASSEMBLY__
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index 19f915e..650b1e8 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -486,6 +486,9 @@ __mmap_switched:
>>  	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
>>  	str_l	x24, memstart_addr, x6		// Save PHYS_OFFSET
>>  	mov	x29, #0
>> +#ifdef CONFIG_KASAN
>> +	b	kasan_early_init
>> +#endif
>>  	b	start_kernel
>>  ENDPROC(__mmap_switched)
> 
> I think we still have swapper_pg_dir in x26 at this point, could you
> instead do:
> 
> 	mov	x0, x26
> 	bl	kasan_map_early_shadow
> 
> Actually, I don't think kasan_map_early_shadow() even needs this
> argument, it uses pgd_offset_k() anyway.
> 

Indeed, just "bl	kasan_map_early_shadow" would be enough.


>> diff --git a/arch/arm64/mm/kasan_init.c b/arch/arm64/mm/kasan_init.c
>> new file mode 100644
>> index 0000000..35dbd84
>> --- /dev/null
>> +++ b/arch/arm64/mm/kasan_init.c
>> @@ -0,0 +1,143 @@
>> +#include <linux/kasan.h>
>> +#include <linux/kernel.h>
>> +#include <linux/memblock.h>
>> +#include <linux/start_kernel.h>
>> +
>> +#include <asm/page.h>
>> +#include <asm/pgalloc.h>
>> +#include <asm/pgtable.h>
>> +#include <asm/tlbflush.h>
>> +
>> +unsigned char kasan_zero_page[PAGE_SIZE] __page_aligned_bss;
> 
> So that's needed because the shadow memory is mapped before paging_init
> is called and we don't have the zero page set up yet. Please add a
> comment.
> 

Actually this page has two purposes, so naming is bad here.
There was a debate (in kasan for x86_64 thread) about its name, but nobody
come up wit a good name.

So I'll add following comment:

/*
 * This page serves two purposes:
 *   - It used as early shadow memory. The entire shadow region populated with this
 *      page, before we will be able to setup normal shadow memory.
 *   - Latter it reused it as zero shadow to cover large ranges of memory
 *      that allowed to access, but not handled by kasan (vmalloc/vmemmap ...).
 */


>> +static pgd_t tmp_page_table[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
> 
> This doesn't need a full PAGE_SIZE alignment, just PGD_SIZE. You could
> also rename to tmp_pg_dir for consistency with swapper and idmap.
> 

Ok.

>> +
>> +#if CONFIG_PGTABLE_LEVELS > 3
>> +pud_t kasan_zero_pud[PTRS_PER_PUD] __page_aligned_bss;
>> +#endif
>> +#if CONFIG_PGTABLE_LEVELS > 2
>> +pmd_t kasan_zero_pmd[PTRS_PER_PMD] __page_aligned_bss;
>> +#endif
>> +pte_t kasan_zero_pte[PTRS_PER_PTE] __page_aligned_bss;
>> +
>> +static void __init kasan_early_pmd_populate(unsigned long start,
>> +					unsigned long end, pud_t *pud)
>> +{
>> +	unsigned long addr;
>> +	unsigned long next;
>> +	pmd_t *pmd;
>> +
>> +	pmd = pmd_offset(pud, start);
>> +	for (addr = start; addr < end; addr = next, pmd++) {
>> +		pmd_populate_kernel(&init_mm, pmd, kasan_zero_pte);
>> +		next = pmd_addr_end(addr, end);
>> +	}
>> +}
>> +
>> +static void __init kasan_early_pud_populate(unsigned long start,
>> +					unsigned long end, pgd_t *pgd)
>> +{
>> +	unsigned long addr;
>> +	unsigned long next;
>> +	pud_t *pud;
>> +
>> +	pud = pud_offset(pgd, start);
>> +	for (addr = start; addr < end; addr = next, pud++) {
>> +		pud_populate(&init_mm, pud, kasan_zero_pmd);
>> +		next = pud_addr_end(addr, end);
>> +		kasan_early_pmd_populate(addr, next, pud);
>> +	}
>> +}
>> +
>> +static void __init kasan_map_early_shadow(pgd_t *pgdp)
>> +{
>> +	int i;
>> +	unsigned long start = KASAN_SHADOW_START;
>> +	unsigned long end = KASAN_SHADOW_END;
>> +	unsigned long addr;
>> +	unsigned long next;
>> +	pgd_t *pgd;
>> +
>> +	for (i = 0; i < PTRS_PER_PTE; i++)
>> +		set_pte(&kasan_zero_pte[i], pfn_pte(
>> +				virt_to_pfn(kasan_zero_page), PAGE_KERNEL));
> 
> Does this need to be writable? If yes, is there anything writing
> non-zero values to it?
> 

Yes. Before kasan_init() this needs to be writable for stack instrumentation.
In function's prologue GCC generates some code that writes to shadow marking
redzones around stack variables.

So the page will contain some garbage, however it doesn't matter for early
stage of boot. Kasan will ignore any bad shadow value before kasan_init().

>> +
>> +	pgd = pgd_offset_k(start);
>> +	for (addr = start; addr < end; addr = next, pgd++) {
>> +		pgd_populate(&init_mm, pgd, kasan_zero_pud);
>> +		next = pgd_addr_end(addr, end);
>> +		kasan_early_pud_populate(addr, next, pgd);
>> +	}
> 
> I prefer to use "do ... while" constructs similar to __create_mapping()
> (or zero_{pgd,pud,pmd}_populate as you are more familiar with them).
> 
> But what I don't get here is that you repopulate the pud page for every
> pgd (and so on for pmd). You don't need this recursive call all the way
> to kasan_early_pmd_populate() but just sequential:
> 

This repopulation needed for 3,2 level page tables configurations.

E.g. for 3-level page tables we need to call pud_populate(&init_mm, pud, kasan_zero_pmd)
for each pud in [KASAN_SHADOW_START, KASAN_SHADOW_END] range, this causes repopopulation for 4-level
page tables, since we need to pud_populate() only [KASAN_SHADOW_START, KASAN_SHADOW_START + PGDIR_SIZE] range.

> 	kasan_early_pte_populate();
> 	kasan_early_pmd_populate(..., pte);
> 	kasan_early_pud_populate(..., pmd);
> 	kasan_early_pgd_populate(..., pud);
> 
> (or in reverse order)
> 

Unless, I'm missing something, this will either work only with 4-level page tables.
We could do this without repopulation by using CONFIG_PGTABLE_LEVELS ifdefs.



> That's because you don't have enough pte/pmd/pud pages to cover the
> range (i.e. you need 512 pte pages for a pmd page) but you just reuse
> the same table page to make all of them pointing to kasan_zero_page.
> 
>> +void __init kasan_early_init(void)
>> +{
>> +	kasan_map_early_shadow(swapper_pg_dir);
>> +	start_kernel();
>> +}
>> +
>> +static void __init clear_pgds(unsigned long start,
>> +			unsigned long end)
>> +{
>> +	/*
>> +	 * Remove references to kasan page tables from
>> +	 * swapper_pg_dir. pgd_clear() can't be used
>> +	 * here because it's nop on 2,3-level pagetable setups
>> +	 */
>> +	for (; start && start < end; start += PGDIR_SIZE)
>> +		set_pgd(pgd_offset_k(start), __pgd(0));
>> +}
>> +
>> +static void __init cpu_set_ttbr1(unsigned long ttbr1)
>> +{
>> +	asm(
>> +	"	msr	ttbr1_el1, %0\n"
>> +	"	isb"
>> +	:
>> +	: "r" (ttbr1));
>> +}
>> +
>> +void __init kasan_init(void)
>> +{
>> +	struct memblock_region *reg;
>> +
>> +	/*
>> +	 * We are going to perform proper setup of shadow memory.
>> +	 * At first we should unmap early shadow (clear_pgds() call bellow).
>> +	 * However, instrumented code couldn't execute without shadow memory.
>> +	 * tmp_page_table used to keep early shadow mapped until full shadow
>> +	 * setup will be finished.
>> +	 */
>> +	memcpy(tmp_page_table, swapper_pg_dir, sizeof(tmp_page_table));
>> +	cpu_set_ttbr1(__pa(tmp_page_table));
>> +	flush_tlb_all();
>> +
>> +	clear_pgds(KASAN_SHADOW_START, KASAN_SHADOW_END);
>> +
>> +	kasan_populate_zero_shadow((void *)KASAN_SHADOW_START,
>> +			kasan_mem_to_shadow((void *)MODULES_VADDR));
>> +
>> +	for_each_memblock(memory, reg) {
>> +		void *start = (void *)__phys_to_virt(reg->base);
>> +		void *end = (void *)__phys_to_virt(reg->base + reg->size);
>> +
>> +		if (start >= end)
>> +			break;
>> +
>> +		/*
>> +		 * end + 1 here is intentional. We check several shadow bytes in
>> +		 * advance to slightly speed up fastpath. In some rare cases
>> +		 * we could cross boundary of mapped shadow, so we just map
>> +		 * some more here.
>> +		 */
>> +		vmemmap_populate((unsigned long)kasan_mem_to_shadow(start),
>> +				(unsigned long)kasan_mem_to_shadow(end) + 1,
>> +				pfn_to_nid(virt_to_pfn(start)));
> 
> Is the only reason for sparsemem vmemmap dependency to reuse this
> function? Maybe at some point you could factor this out and not require
> SPARSEMEM_VMEMMAP to be enabled.
> 

Yes, this is the only reason and I'll get rid of this dependency some day.

> About the "end + 1", what you actually get is an additional full section
> (PMD_SIZE) with the 4KB page configuration. Since the shadow is 1/8 of
> the VA space, do we need a check for memblocks within 8 * 2MB of each
> other?
> 

Overlaps should be ok. vmemmap_populate will handle it.

>> +	}
>> +
>> +	memset(kasan_zero_page, 0, PAGE_SIZE);
> 
> Has anyone written to this page? Actually, what's its use after we
> enabled the proper KASan shadow mappings?
> 

As said before, in function's prologue GCC generates code that writes to shadow
so it writes to this page.

After kasan_init() this page used as shadow and covers large portions of memory
which are not handled by kasan (vmalloc/vmemmap). We just assume that any access
to this memory region is good.

>> +	cpu_set_ttbr1(__pa(swapper_pg_dir));
>> +	flush_tlb_all();
>> +
>> +	/* At this point kasan is fully initialized. Enable error messages */
>> +	init_task.kasan_depth = 0;
>> +}
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1183769

On Fri, Jul 10, 2015 at 08:11:03PM +0300, Andrey Ryabinin wrote:
> >> +#if CONFIG_PGTABLE_LEVELS > 3
> >> +pud_t kasan_zero_pud[PTRS_PER_PUD] __page_aligned_bss;
> >> +#endif
> >> +#if CONFIG_PGTABLE_LEVELS > 2
> >> +pmd_t kasan_zero_pmd[PTRS_PER_PMD] __page_aligned_bss;
> >> +#endif
> >> +pte_t kasan_zero_pte[PTRS_PER_PTE] __page_aligned_bss;
> >> +
> >> +static void __init kasan_early_pmd_populate(unsigned long start,
> >> +					unsigned long end, pud_t *pud)
> >> +{
> >> +	unsigned long addr;
> >> +	unsigned long next;
> >> +	pmd_t *pmd;
> >> +
> >> +	pmd = pmd_offset(pud, start);
> >> +	for (addr = start; addr < end; addr = next, pmd++) {
> >> +		pmd_populate_kernel(&init_mm, pmd, kasan_zero_pte);
> >> +		next = pmd_addr_end(addr, end);
> >> +	}
> >> +}
> >> +
> >> +static void __init kasan_early_pud_populate(unsigned long start,
> >> +					unsigned long end, pgd_t *pgd)
> >> +{
> >> +	unsigned long addr;
> >> +	unsigned long next;
> >> +	pud_t *pud;
> >> +
> >> +	pud = pud_offset(pgd, start);
> >> +	for (addr = start; addr < end; addr = next, pud++) {
> >> +		pud_populate(&init_mm, pud, kasan_zero_pmd);
> >> +		next = pud_addr_end(addr, end);
> >> +		kasan_early_pmd_populate(addr, next, pud);
> >> +	}
> >> +}
> >> +
> >> +static void __init kasan_map_early_shadow(pgd_t *pgdp)
> >> +{
> >> +	int i;
> >> +	unsigned long start = KASAN_SHADOW_START;
> >> +	unsigned long end = KASAN_SHADOW_END;
> >> +	unsigned long addr;
> >> +	unsigned long next;
> >> +	pgd_t *pgd;
> >> +
> >> +	for (i = 0; i < PTRS_PER_PTE; i++)
> >> +		set_pte(&kasan_zero_pte[i], pfn_pte(
> >> +				virt_to_pfn(kasan_zero_page), PAGE_KERNEL));
> >> +
> >> +	pgd = pgd_offset_k(start);
> >> +	for (addr = start; addr < end; addr = next, pgd++) {
> >> +		pgd_populate(&init_mm, pgd, kasan_zero_pud);
> >> +		next = pgd_addr_end(addr, end);
> >> +		kasan_early_pud_populate(addr, next, pgd);
> >> +	}
> > 
> > I prefer to use "do ... while" constructs similar to __create_mapping()
> > (or zero_{pgd,pud,pmd}_populate as you are more familiar with them).
> > 
> > But what I don't get here is that you repopulate the pud page for every
> > pgd (and so on for pmd). You don't need this recursive call all the way
> > to kasan_early_pmd_populate() but just sequential:
> 
> This repopulation needed for 3,2 level page tables configurations.
> 
> E.g. for 3-level page tables we need to call pud_populate(&init_mm,
> pud, kasan_zero_pmd) for each pud in [KASAN_SHADOW_START,
> KASAN_SHADOW_END] range, this causes repopopulation for 4-level page
> tables, since we need to pud_populate() only [KASAN_SHADOW_START,
> KASAN_SHADOW_START + PGDIR_SIZE] range.

I'm referring to writing the same information multiple times over the
same entry. kasan_map_early_shadow() goes over each pgd entry and writes
the address of kasan_zero_pud. That's fine so far. However, in the same
loop you call kasan_early_pud_populate(). The latter retrieves the pud
page via pud_offset(pgd, start) which would always be kasan_zero_pud
because that's what you wrote via pgd_populate() in each pgd entry. So
for each pgd entry, you keep populating the same kasan_zero_pud page
with pointers to kasan_zero_pmd. And so on for the pmd.

> > 	kasan_early_pte_populate();
> > 	kasan_early_pmd_populate(..., pte);
> > 	kasan_early_pud_populate(..., pmd);
> > 	kasan_early_pgd_populate(..., pud);
> > 
> > (or in reverse order)
> 
> Unless, I'm missing something, this will either work only with 4-level
> page tables. We could do this without repopulation by using
> CONFIG_PGTABLE_LEVELS ifdefs.

Or you could move kasan_early_*_populate outside the loop. You already
do this for the pte at the beginning of the kasan_map_early_shadow()
function (and it probably makes more sense to create a separate
kasan_early_pte_populate).

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1184426

On 07/14/2015 06:04 PM, Catalin Marinas wrote:
> On Fri, Jul 10, 2015 at 08:11:03PM +0300, Andrey Ryabinin wrote:
>>>> +#if CONFIG_PGTABLE_LEVELS > 3
>>>> +pud_t kasan_zero_pud[PTRS_PER_PUD] __page_aligned_bss;
>>>> +#endif
>>>> +#if CONFIG_PGTABLE_LEVELS > 2
>>>> +pmd_t kasan_zero_pmd[PTRS_PER_PMD] __page_aligned_bss;
>>>> +#endif
>>>> +pte_t kasan_zero_pte[PTRS_PER_PTE] __page_aligned_bss;
>>>> +
>>>> +static void __init kasan_early_pmd_populate(unsigned long start,
>>>> +					unsigned long end, pud_t *pud)
>>>> +{
>>>> +	unsigned long addr;
>>>> +	unsigned long next;
>>>> +	pmd_t *pmd;
>>>> +
>>>> +	pmd = pmd_offset(pud, start);
>>>> +	for (addr = start; addr < end; addr = next, pmd++) {
>>>> +		pmd_populate_kernel(&init_mm, pmd, kasan_zero_pte);
>>>> +		next = pmd_addr_end(addr, end);
>>>> +	}
>>>> +}
>>>> +
>>>> +static void __init kasan_early_pud_populate(unsigned long start,
>>>> +					unsigned long end, pgd_t *pgd)
>>>> +{
>>>> +	unsigned long addr;
>>>> +	unsigned long next;
>>>> +	pud_t *pud;
>>>> +
>>>> +	pud = pud_offset(pgd, start);
>>>> +	for (addr = start; addr < end; addr = next, pud++) {
>>>> +		pud_populate(&init_mm, pud, kasan_zero_pmd);
>>>> +		next = pud_addr_end(addr, end);
>>>> +		kasan_early_pmd_populate(addr, next, pud);
>>>> +	}
>>>> +}
>>>> +
>>>> +static void __init kasan_map_early_shadow(pgd_t *pgdp)
>>>> +{
>>>> +	int i;
>>>> +	unsigned long start = KASAN_SHADOW_START;
>>>> +	unsigned long end = KASAN_SHADOW_END;
>>>> +	unsigned long addr;
>>>> +	unsigned long next;
>>>> +	pgd_t *pgd;
>>>> +
>>>> +	for (i = 0; i < PTRS_PER_PTE; i++)
>>>> +		set_pte(&kasan_zero_pte[i], pfn_pte(
>>>> +				virt_to_pfn(kasan_zero_page), PAGE_KERNEL));
>>>> +
>>>> +	pgd = pgd_offset_k(start);
>>>> +	for (addr = start; addr < end; addr = next, pgd++) {
>>>> +		pgd_populate(&init_mm, pgd, kasan_zero_pud);
>>>> +		next = pgd_addr_end(addr, end);
>>>> +		kasan_early_pud_populate(addr, next, pgd);
>>>> +	}
>>>
>>> I prefer to use "do ... while" constructs similar to __create_mapping()
>>> (or zero_{pgd,pud,pmd}_populate as you are more familiar with them).
>>>
>>> But what I don't get here is that you repopulate the pud page for every
>>> pgd (and so on for pmd). You don't need this recursive call all the way
>>> to kasan_early_pmd_populate() but just sequential:
>>
>> This repopulation needed for 3,2 level page tables configurations.
>>
>> E.g. for 3-level page tables we need to call pud_populate(&init_mm,
>> pud, kasan_zero_pmd) for each pud in [KASAN_SHADOW_START,
>> KASAN_SHADOW_END] range, this causes repopopulation for 4-level page
>> tables, since we need to pud_populate() only [KASAN_SHADOW_START,
>> KASAN_SHADOW_START + PGDIR_SIZE] range.
> 
> I'm referring to writing the same information multiple times over the
> same entry. kasan_map_early_shadow() goes over each pgd entry and writes
> the address of kasan_zero_pud. That's fine so far. However, in the same
> loop you call kasan_early_pud_populate(). The latter retrieves the pud
> page via pud_offset(pgd, start) which would always be kasan_zero_pud

Not always. E.g. if we have 3-level page tables pud = pgd, pgd_populate() is nop, and
pud_populate in fact populates pgd.

pud_offset(pgd, start) will return (swapper_pg_dir + pgd_index(start)) and pud_populate()
will fill that entry with the address of kasan_zero_pmd. So we need to pud_populate() for
each pgd.

> because that's what you wrote via pgd_populate() in each pgd entry. So
> for each pgd entry, you keep populating the same kasan_zero_pud page
> with pointers to kasan_zero_pmd. And so on for the pmd.
> 

Yes, I'm perfectly understand that. And this was done intentionally since I don't
see the way to make this work for all possible CONFIG_PGTABLE_LEVELS without rewrites
or without #ifdefs (and you didn't like them in v1).


>>> 	kasan_early_pte_populate();
>>> 	kasan_early_pmd_populate(..., pte);
>>> 	kasan_early_pud_populate(..., pmd);
>>> 	kasan_early_pgd_populate(..., pud);
>>>
>>> (or in reverse order)
>>
>> Unless, I'm missing something, this will either work only with 4-level
>> page tables. We could do this without repopulation by using
>> CONFIG_PGTABLE_LEVELS ifdefs.
> 
> Or you could move kasan_early_*_populate outside the loop. You already
> do this for the pte at the beginning of the kasan_map_early_shadow()
> function (and it probably makes more sense to create a separate
> kasan_early_pte_populate).
> 


Ok, let's try to implement that.
And for example, let's consider CONFIG_PGTABLE_LEVELS=3 case:

 * pgd_populate() is nop, so kasan_early_pgd_populate() won't do anything.

 * pud_populate() in kasan_early_pud_populate() actually will setup pgd entries in swapper_pg_dir,
   so pud_populate() should be called for the whole shadow range: [KASAN_SHADOW_START, KASAN_SHADOW_END]
	IOW: kasan_early_pud_populate(KASAN_SHADOW_START, KASAN_SHADOW_END, kasan_zero_pmd);
	
	We will need to slightly change kasan_early_pud_populate() implementation for that
	(Current implementation implies that [start, end) addresses belong to one pgd)

	void kasan_early_pud_populate(unsigned long start, unsigned long end, pmd_t *pmd)
	{
		unsigned long addr;
		long next;

		for (addr = start; addr < end; addr = next) {
			pud_t *pud = pud_offset(pgd_offset_k(addr), addr);
			pud_populate(&init_mm, pud, pmd);
			next = pud_addr_end(addr, pgd_addr_end(addr, end));
		}
	}

	But, wait! In 4-level page tables case this will be the same repopulation as we had before!

See? The problem here is that pud_populate() but not pgd_populate() populates pgds (3-level page tables case).
So I still don't see the way to avoid repopulation without ifdefs.
Did I miss anything?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1185988

On 07/15/2015 07:37 PM, Catalin Marinas wrote:
> Ok, so simply taking the call out of the loop won't work unless we
> conditionally define these functions (wouldn't be too bad since we have
> some #if CONFIG_PGTABLE_LEVELS already introduced by this patch but it
> would be nicer without).
> 
> Anyway, I think we can keep the current iterations but exit early if
> !pud_none() because it means we already populated it (reworked to match
> other such patterns throughout the kernel with pgd_populate called from
> the pud function; and untested):
> 
> void kasan_early_pmd_populate(pud_t *pud, unsigned long addr, unsigned long end)
> {
> 	pmd_t *pmd;
> 	unsigned long next;
> 
> 	if (pud_none(*pud))
> 		pud_populate(&init_mm, pud, kasan_zero_pmd);
> 
> 	pmd = pmd_offset(pud, addr);
> 	do {
> 		next = pmd_addr_end(addr, end);
> 		kasan_early_pte_populate(pmd, addr, next);
> 	} while (pmd++, addr = next, addr != end && pmd_none(*pmd));
> }
> 
> void kasan_early_pud_populate(pgd_t *pgd, unsigned long addr, unsigned long end)
> {
> 	pud_t *pud;
> 	unsigned long next;
> 
> 	if (pgd_none(*pgd))
> 		pgd_populate(&init_mm, pgd, kasan_zero_pud);
> 
> 	pud = pud_offset(pgd, addr);
> 	do {
> 		next = pud_addr_end(addr, end);
> 		kasan_early_pmd_populate(pud, addr, next);
> 	} while (pud++, addr = next, addr != end && pud_none(*pud));
> }
> 
> Given that we check pud_none() after the first iterations, it covers the
> lower levels if needed.
> 

I think this may work, if pud_none(*pud) will be replaced with !pud_val(*pud).
We can't use pud_none() because with 2-level page tables it's always false, so
we will never go down to pmd level where swapper_pg_dir populated.

But you gave me another idea how we could use p?d_none() and avoid rewriting table entries:


void kasan_early_pmd_populate(unsigned long start, unsigned long end, pte_t *pte)
{
	unsigned long addr = start;
	long next;

	do {
		pgd_t *pgd = pgd_offset_k(addr);
		pud_t *pud = pud_offset(pgd, addr);
		pmd_t *pmd = pmd_offset(pud, addr);

		if (!pmd_none(*pmd))
			break;

		pmd_populate_kernel(&init_mm, pmd, pte);
		next = pgd_addr_end(addr, end);
		next = pud_addr_end(addr, next)
		next = pmd_addr_end(addr, next);
	} while(addr = next, addr != end);
}

void kasan_early_pud_populate(unsigned long start, unsigned long end, pmd_t *pmd)
{
	unsigned long addr = start;
	long next;

	do {
		pgd_t *pgd = pgd_offset_k(addr);
		pud_t *pud = pud_offset(pgd, addr);

		if (!pud_none(*pud))
			break;

		pud_populate(&init_mm, pud, pmd);
		next = pud_addr_end(addr, pgd_addr_end(addr, end));
	} while(addr = next, addr != end);
}


void kasan_early_pgd_populate(...)
{
	//something similar to above
	....
}

static void __init kasan_map_early_shadow(void)
{
	kasan_early_pgd_populate(KASAN_SHADOW_START, KASAN_SHADOW_END, kasan_zero_pud);
	kasan_early_pud_populate(KASAN_SHADOW_START, KASAN_SHADOW_END, kasan_zero_pmd);
	kasan_early_pmd_populate(KASAN_SHADOW_START, KASAN_SHADOW_END, kasan_zero_pte);
	kasan_early_pte_populate();
}





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186040

On Thu, Jul 16, 2015 at 06:30:11PM +0300, Andrey Ryabinin wrote:
> On 07/15/2015 07:37 PM, Catalin Marinas wrote:
> > Ok, so simply taking the call out of the loop won't work unless we
> > conditionally define these functions (wouldn't be too bad since we have
> > some #if CONFIG_PGTABLE_LEVELS already introduced by this patch but it
> > would be nicer without).
> > 
> > Anyway, I think we can keep the current iterations but exit early if
> > !pud_none() because it means we already populated it (reworked to match
> > other such patterns throughout the kernel with pgd_populate called from
> > the pud function; and untested):
> > 
> > void kasan_early_pmd_populate(pud_t *pud, unsigned long addr, unsigned long end)
> > {
> > 	pmd_t *pmd;
> > 	unsigned long next;
> > 
> > 	if (pud_none(*pud))
> > 		pud_populate(&init_mm, pud, kasan_zero_pmd);
> > 
> > 	pmd = pmd_offset(pud, addr);
> > 	do {
> > 		next = pmd_addr_end(addr, end);
> > 		kasan_early_pte_populate(pmd, addr, next);
> > 	} while (pmd++, addr = next, addr != end && pmd_none(*pmd));
> > }
> > 
> > void kasan_early_pud_populate(pgd_t *pgd, unsigned long addr, unsigned long end)
> > {
> > 	pud_t *pud;
> > 	unsigned long next;
> > 
> > 	if (pgd_none(*pgd))
> > 		pgd_populate(&init_mm, pgd, kasan_zero_pud);
> > 
> > 	pud = pud_offset(pgd, addr);
> > 	do {
> > 		next = pud_addr_end(addr, end);
> > 		kasan_early_pmd_populate(pud, addr, next);
> > 	} while (pud++, addr = next, addr != end && pud_none(*pud));
> > }
> > 
> > Given that we check pud_none() after the first iterations, it covers the
> > lower levels if needed.
> 
> I think this may work, if pud_none(*pud) will be replaced with !pud_val(*pud).
> We can't use pud_none() because with 2-level page tables it's always false, so
> we will never go down to pmd level where swapper_pg_dir populated.

The reason I used "do ... while" vs "while" or "for" is so that it gets
down to the pmd level. The iteration over pgd is always done in the top
loop via pgd_addr_end while the loops for missing levels (nopud, nopmd)
are always a single iteration whether we check for pud_none or not. But
when the level is present, we avoid looping when !pud_none().

> But you gave me another idea how we could use p?d_none() and avoid rewriting table entries:
> 
> 
> void kasan_early_pmd_populate(unsigned long start, unsigned long end, pte_t *pte)
> {
> 	unsigned long addr = start;
> 	long next;
> 
> 	do {
> 		pgd_t *pgd = pgd_offset_k(addr);
> 		pud_t *pud = pud_offset(pgd, addr);
> 		pmd_t *pmd = pmd_offset(pud, addr);
> 
> 		if (!pmd_none(*pmd))
> 			break;
> 
> 		pmd_populate_kernel(&init_mm, pmd, pte);
> 		next = pgd_addr_end(addr, end);
> 		next = pud_addr_end(addr, next)
> 		next = pmd_addr_end(addr, next);
> 	} while(addr = next, addr != end);
> }
> 
> void kasan_early_pud_populate(unsigned long start, unsigned long end, pmd_t *pmd)
> {
> 	unsigned long addr = start;
> 	long next;
> 
> 	do {
> 		pgd_t *pgd = pgd_offset_k(addr);
> 		pud_t *pud = pud_offset(pgd, addr);
> 
> 		if (!pud_none(*pud))
> 			break;
> 
> 		pud_populate(&init_mm, pud, pmd);
> 		next = pud_addr_end(addr, pgd_addr_end(addr, end));
> 	} while(addr = next, addr != end);
> }
> 
> 
> void kasan_early_pgd_populate(...)
> {
> 	//something similar to above
> 	....
> }
> 
> static void __init kasan_map_early_shadow(void)
> {
> 	kasan_early_pgd_populate(KASAN_SHADOW_START, KASAN_SHADOW_END, kasan_zero_pud);
> 	kasan_early_pud_populate(KASAN_SHADOW_START, KASAN_SHADOW_END, kasan_zero_pmd);
> 	kasan_early_pmd_populate(KASAN_SHADOW_START, KASAN_SHADOW_END, kasan_zero_pte);
> 	kasan_early_pte_populate();
> }

While this would probably work, you still need #ifdef's since
kasan_zero_pud is not defined with 2 and 3 levels. That's what I
initially thought we should do but since you didn't like the #ifdef's, I
came up with another proposal.

So, I still prefer my suggestion above unless you find a problem with
it.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186816

On 07/16/2015 07:03 PM, Catalin Marinas wrote:
> On Thu, Jul 16, 2015 at 06:30:11PM +0300, Andrey Ryabinin wrote:
>>
>> I think this may work, if pud_none(*pud) will be replaced with !pud_val(*pud).
>> We can't use pud_none() because with 2-level page tables it's always false, so
>> we will never go down to pmd level where swapper_pg_dir populated.
> 
> The reason I used "do ... while" vs "while" or "for" is so that it gets
> down to the pmd level. The iteration over pgd is always done in the top
> loop via pgd_addr_end while the loops for missing levels (nopud, nopmd)
> are always a single iteration whether we check for pud_none or not. But
> when the level is present, we avoid looping when !pud_none().
> 

Right, dunno what I was thinking.
It seems to work. Lightly tested with every possible CONFIG_PGTABLE_LEVELS.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web