Re: [patch 1/6] mm/vmalloc: Prevent stale TLBs in fully utilized blocks

From: Baoquan He <bhe@redhat.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
	Christoph Hellwig <hch@lst.de>,
	Uladzislau Rezki <urezki@gmail.com>,
	Lorenzo Stoakes <lstoakes@gmail.com>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [patch 1/6] mm/vmalloc: Prevent stale TLBs in fully utilized blocks
Date: Wed, 24 May 2023 17:32:01 +0800	[thread overview]
Message-ID: <ZG3ZkUrQK4LwnN9p@MiWiFi-R3L-srv> (raw)
In-Reply-To: <20230523140002.575854344@linutronix.de>

On 05/23/23 at 04:02pm, Thomas Gleixner wrote:
> _vm_unmap_aliases() is used to ensure that no unflushed TLB entries for a
> page are left in the system. This is required due to the lazy TLB flush
> mechanism in vmalloc.
> 
> This is tried to achieve by walking the per CPU free lists, but those do
> not contain fully utilized vmap blocks because they are removed from the
> free list once the blocks free space became zero.
> 
> So the per CPU list iteration does not find the block and if the page was
> mapped via such a block and the TLB has not yet been flushed, the guarantee
> of _vm_unmap_aliases() that there are no stale TLBs after returning is
> broken:
> 
> x = vb_alloc() // Removes vmap_block from free list because vb->free became 0
> vb_free(x)     // Unmaps page and marks in dirty_min/max range
> 
> // Page is reused
> vm_unmap_aliases() // Can't find vmap block with the dirty space -> FAIL
> 
> So instead of walking the per CPU free lists, walk the per CPU xarrays
> which hold pointers to _all_ active blocks in the system including those
> removed from the free lists.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  mm/vmalloc.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2236,9 +2236,10 @@ static void _vm_unmap_aliases(unsigned l
>  	for_each_possible_cpu(cpu) {
>  		struct vmap_block_queue *vbq = &per_cpu(vmap_block_queue, cpu);
>  		struct vmap_block *vb;
> +		unsigned long idx;
>  
>  		rcu_read_lock();

Do we need to remove this rcu_read_xx() pair since it marks the RCU
read-side critical section on vbq-free list?

> -		list_for_each_entry_rcu(vb, &vbq->free, free_list) {
> +		xa_for_each(&vbq->vmap_blocks, idx, vb) {
>  			spin_lock(&vb->lock);
>  			if (vb->dirty && vb->dirty != VMAP_BBMAP_BITS) {
>  				unsigned long va_start = vb->va->va_start;
> 