Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Daniel Axtens <dja@axtens.net>
To:     Andrey Ryabinin <aryabinin@virtuozzo.com>,
        kasan-dev@googlegroups.com, linux-mm@kvack.org, x86@kernel.org,
        glider@google.com, luto@kernel.org, linux-kernel@vger.kernel.org,
        mark.rutland@arm.com, dvyukov@google.com, christophe.leroy@c-s.fr
Cc:     linuxppc-dev@lists.ozlabs.org, gor@linux.ibm.com
Subject: Re: [PATCH v8 1/5] kasan: support backing vmalloc space with real shadow memory
In-Reply-To: <352cb4fa-2e57-7e3b-23af-898e113bbe22@virtuozzo.com>
References: <20191001065834.8880-1-dja@axtens.net> <20191001065834.8880-2-dja@axtens.net> <352cb4fa-2e57-7e3b-23af-898e113bbe22@virtuozzo.com>
Date:   Tue, 15 Oct 2019 00:57:44 +1100
Message-ID: <87ftjvtoo7.fsf@dja-thinkpad.axtens.net>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Andrey,


>> +	/*
>> +	 * Ensure poisoning is visible before the shadow is made visible
>> +	 * to other CPUs.
>> +	 */
>> +	smp_wmb();
>
> I'm not quite understand what this barrier do and why it needed.
> And if it's really needed there should be a pairing barrier
> on the other side which I don't see.

Mark might be better able to answer this, but my understanding is that
we want to make sure that we never have a situation where the writes are
reordered so that PTE is installed before all the poisioning is written
out. I think it follows the logic in __pte_alloc() in mm/memory.c:

	/*
	 * Ensure all pte setup (eg. pte page lock and page clearing) are
	 * visible before the pte is made visible to other CPUs by being
	 * put into page tables.
	 *
	 * The other side of the story is the pointer chasing in the page
	 * table walking code (when walking the page table without locking;
	 * ie. most of the time). Fortunately, these data accesses consist
	 * of a chain of data-dependent loads, meaning most CPUs (alpha
	 * being the notable exception) will already guarantee loads are
	 * seen in-order. See the alpha page table accessors for the
	 * smp_read_barrier_depends() barriers in page table walking code.
	 */
	smp_wmb(); /* Could be smp_wmb__xxx(before|after)_spin_lock */

I can clarify the comment.

>> +
>> +	spin_lock(&init_mm.page_table_lock);
>> +	if (likely(pte_none(*ptep))) {
>> +		set_pte_at(&init_mm, addr, ptep, pte);
>> +		page = 0;
>> +	}
>> +	spin_unlock(&init_mm.page_table_lock);
>> +	if (page)
>> +		free_page(page);
>> +	return 0;
>> +}
>> +
>
>
> ...
>
>> @@ -754,6 +769,8 @@ merge_or_add_vmap_area(struct vmap_area *va,
>>  	}
>>  
>>  insert:
>> +	kasan_release_vmalloc(orig_start, orig_end, va->va_start, va->va_end);
>> +
>>  	if (!merged) {
>>  		link_va(va, root, parent, link, head);
>>  		augment_tree_propagate_from(va);
>> @@ -2068,6 +2085,22 @@ static struct vm_struct *__get_vm_area_node(unsigned long size,
>>  
>>  	setup_vmalloc_vm(area, va, flags, caller);
>>  
>> +	/*
>> +	 * For KASAN, if we are in vmalloc space, we need to cover the shadow
>> +	 * area with real memory. If we come here through VM_ALLOC, this is
>> +	 * done by a higher level function that has access to the true size,
>> +	 * which might not be a full page.
>> +	 *
>> +	 * We assume module space comes via VM_ALLOC path.
>> +	 */
>> +	if (is_vmalloc_addr(area->addr) && !(area->flags & VM_ALLOC)) {
>> +		if (kasan_populate_vmalloc(area->size, area)) {
>> +			unmap_vmap_area(va);
>> +			kfree(area);
>> +			return NULL;
>> +		}
>> +	}
>> +
>>  	return area;
>>  }
>>  
>> @@ -2245,6 +2278,9 @@ static void __vunmap(const void *addr, int deallocate_pages)
>>  	debug_check_no_locks_freed(area->addr, get_vm_area_size(area));
>>  	debug_check_no_obj_freed(area->addr, get_vm_area_size(area));
>>  
>> +	if (area->flags & VM_KASAN)
>> +		kasan_poison_vmalloc(area->addr, area->size);
>> +
>>  	vm_remove_mappings(area, deallocate_pages);
>>  
>>  	if (deallocate_pages) {
>> @@ -2497,6 +2533,9 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
>>  	if (!addr)
>>  		return NULL;
>>  
>> +	if (kasan_populate_vmalloc(real_size, area))
>> +		return NULL;
>> +
>
> KASAN itself uses __vmalloc_node_range() to allocate and map shadow in memory online callback.
> So we should either skip non-vmalloc and non-module addresses here or teach kasan's memory online/offline
> callbacks to not use __vmalloc_node_range() (do something similar to kasan_populate_vmalloc() perhaps?). 

Ah, right you are. I haven't been testing that.

I am a bit nervous about further restricting kasan_populate_vmalloc: I
seem to remember having problems with code using the vmalloc family of
functions to map memory that doesn't lie within vmalloc space but which
still has instrumented accesses.

On the other hand, I'm not keen on rewriting any of the memory
on/offline code if I can avoid it!

I'll have a look and get back you as soon as I can.

Thanks for catching this.

Kind regards,
Daniel

>
> -- 
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/352cb4fa-2e57-7e3b-23af-898e113bbe22%40virtuozzo.com.