Re: [PATCH v4 2/2] vmalloc: Avoid rare case of flushing tlb with weird arguements

[Peter Zijlstra]

On Tue, May 21, 2019 at 01:51:37PM -0700, Rick Edgecombe wrote:
> In a rare case, flush_tlb_kernel_range() could be called with a start
> higher than the end. Most architectures should be fine with with this, but
> some may not like it, so avoid doing this.
> 
> In vm_remove_mappings(), in case page_address() returns 0 for all pages,
> _vm_unmap_aliases() will be called with start = ULONG_MAX, end = 0 and
> flush = 1.
> 
> If at the same time, the vmalloc purge operation is triggered by something
> else while the current operation is between remove_vm_area() and
> _vm_unmap_aliases(), then the vm mapping just removed will be already
> purged. In this case the call of vm_unmap_aliases() may not find any other
> mappings to flush and so end up flushing start = ULONG_MAX, end = 0. So
> only set flush = true if we find something in the direct mapping that we
> need to flush, and this way this can't happen.
> 
> Fixes: 868b104d7379 ("mm/vmalloc: Add flag for freeing of special permsissions")
> Cc: Meelis Roos <mroos@xxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Nadav Amit <namit@xxxxxxxxxx>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> ---
>  mm/vmalloc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 836888ae01f6..537d1134b40e 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2125,6 +2125,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages)
>  	unsigned long addr = (unsigned long)area->addr;
>  	unsigned long start = ULONG_MAX, end = 0;
>  	int flush_reset = area->flags & VM_FLUSH_RESET_PERMS;
> +	int flush_dmap = 0;
>  	int i;
>  
>  	/*
> @@ -2163,6 +2164,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages)
>  		if (addr) {
>  			start = min(addr, start);
>  			end = max(addr + PAGE_SIZE, end);
> +			flush_dmap = 1;
>  		}
>  	}
>  
> @@ -2172,7 +2174,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages)
>  	 * reset the direct map permissions to the default.
>  	 */
>  	set_area_direct_map(area, set_direct_map_invalid_noflush);
> -	_vm_unmap_aliases(start, end, 1);
> +	_vm_unmap_aliases(start, end, flush_dmap);
>  	set_area_direct_map(area, set_direct_map_default_noflush);
>  }

Hurmph.. another clue that this range flushing is crap I feel. The phys
addrs of the page array can be scattered all over the place, a range
doesn't properly represent things.

But yes, this seems like a minimal fix in spirit with the existing code.