Re: /proc/kcore reads 0's for vmap_block

[Stephen Brennan <stephen.s.brennan@oracle.com>]

Hi Baoquan,

Thanks for taking a look and coming up with a patch!

(responses inline)

Baoquan He <bhe@redhat.com> writes:
> On 10/26/22 at 04:15pm, Stephen Brennan wrote:
>> Hi all,
>> 
>> The /proc/kcore interface uses vread() to read memory addresses which
>> are in the vmalloc region, and it seems that vread() doesn't support
>> vmap_areas which are associated with a vmap_block. As far as I can tell,
>> those have vmap_area.vm == NULL, and get skipped during vread()'s
>> iteration. So the end result is that the read simply returns 0's.
>
> Hmm, with my understanding, it should be vm_map_ram() which is called
> without a struct vm_struct associated, and it's the only interface do
> to so. vmap_block is the optimization way based on percpu to reduce vmap
> lock race.
>
>> 
>> This impacts live debuggers like gdb and drgn, which is how I stumbled
>> upon it[1]. It looks like crash avoids the issue by doing a page table
>> walk and reading the physical address.
>> 
>> I'm wondering if there's any rationale for this omission from vread():
>> is it a simple oversight, or was it omitted due to the difficulty? Is
>> it possible for /proc/kcore to simply take the page faults when it reads
>> unmapped memory and handle them? (I'm sure that's already discussed or
>> is obviously infeasible for some reason beyond me.)
>
> From git history, the vmlist iterating was taken in vread() at the
> first place. Later, in below commit, when people changed to iterate
> vmap_area_list instead, they just inherited the old code logic. I guess
> that's why vmap with NULL ->vm is skipped.
>
> commit e81ce85f960c ("mm, vmalloc: iterate vmap_area_list, instead of vmlist in vread/vwrite()")
>
>> 
>> Ideally, I'm just looking for a way forward that allows the debugger to
>> *work* as expected, meaning either that /proc/kcore always reads the
>> correct data, or that the debugger can know ahead of time that it will
>> need to do some processing (like a page table walk) first. 
>
> I think we can adjust vread() to allow those vmap_area with NULL ->vm
> being read out? Made a draft patch at below, please feel free to have a
> test. Not sure if there's any risk.

I tested this out and it fully resolved the problem! I didn't encounter
any issues with the debuggers using /proc/kcore. My test kernel module
is here:
https://github.com/brenns10/kernel_stuff/blob/master/drgn_vmalloc_test/drgn_vmalloc_test.c

I was able to read the full contents of the mapped area correctly with
drgn thanks to this patch.

>
> From 9f1b786730f3ee0a8d5b48a94dbefa674102d7b9 Mon Sep 17 00:00:00 2001
> From: Baoquan He <bhe@redhat.com>
> Date: Thu, 27 Oct 2022 16:20:26 +0800
> Subject: [PATCH] mm/vmalloc.c: allow to read out vm_map_ram() areas in vread()
> Content-type: text/plain
>
> Currently, vread can read out vmalloc areas who is associated with
> a vm_struct. While this doesn't work for areas created by vm_map_ram()
> interface because it doesn't allocate a vm_struct. Then in vread(),
> these areas will be skipped.
>
> Pages are passed into vm_map_ram() and mapped onto frea vmap area,
> it should be safe to read them out. Change code to allow to read
> out these vm_map_ram() areas in vread().

I see, we don't even need to worry about page faults because
vm_map_ram() is guaranteed to have mapped them. That does make things
easier :)

>
> Signed-off-by: Baoquan He <bhe@redhat.com>

Reviewed-by: Stephen Brennan <stephen.s.brennan@oracle.com>

(or Tested-by if you prefer)

> ---
>  mm/vmalloc.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index ccaa461998f3..f899ab784671 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -3526,7 +3526,7 @@ long vread(char *buf, char *addr, unsigned long count)
>  	struct vm_struct *vm;
>  	char *vaddr, *buf_start = buf;
>  	unsigned long buflen = count;
> -	unsigned long n;
> +	unsigned long n, size;
>  
>  	addr = kasan_reset_tag(addr);
>  
> @@ -3547,12 +3547,11 @@ long vread(char *buf, char *addr, unsigned long count)
>  		if (!count)
>  			break;
>  
> -		if (!va->vm)
> -			continue;
> -
>  		vm = va->vm;
> -		vaddr = (char *) vm->addr;
> -		if (addr >= vaddr + get_vm_area_size(vm))
> +		vaddr = (char *) va->va_start;
> +		size = vm ? get_vm_area_size(vm) : va_size(va);
> +
> +		if (addr >= vaddr + size)
>  			continue;
>  		while (addr < vaddr) {
>  			if (count == 0)
> @@ -3562,10 +3561,10 @@ long vread(char *buf, char *addr, unsigned long count)
>  			addr++;
>  			count--;
>  		}
> -		n = vaddr + get_vm_area_size(vm) - addr;
> +		n = vaddr + size - addr;
>  		if (n > count)
>  			n = count;
> -		if (!(vm->flags & VM_IOREMAP))
> +		if (!vm || !(vm->flags & VM_IOREMAP))
>  			aligned_vread(buf, addr, n);
>  		else /* IOREMAP area is treated as memory hole */
>  			memset(buf, 0, n);
> -- 
> 2.34.1