On 11/10/22 at 10:48am, Stephen Brennan wrote: > Baoquan He <bhe@xxxxxxxxxx> writes: > > > On 11/09/22 at 04:59pm, Stephen Brennan wrote: > > ...... > >> > @@ -3569,12 +3609,14 @@ long vread(char *buf, char *addr, unsigned long count) > >> > if (!count) > >> > break; > >> > > >> > - if (!va->vm) > >> > + if (!(va->flags & VMAP_RAM) && !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); > >> > >> Hi Baoquan, > >> > >> Thanks for working on this. I tested your patches out by using drgn to > >> debug /proc/kcore. I have a kernel module[1] to do a vm_map_ram() call > >> and print the virtual address to the kernel log so I can try to read > >> that memory address in drgn. When I did this test, I got a panic on the > >> above line of code. > > ...... > >> Since flags is in a union, it shadows "vm" and causes the condition to > >> be true, and then get_vm_area_size() tries to follow the pointer defined > >> by flags. I'm not sure if the fix is to have flags be a separate field > >> inside vmap_area, or to have more careful handling in the vread path. > > > > Sorry, my bad. Thanks for testing this and catching the error, Stephen. > > > > About the fix, both way are fine to me. I made a draft fix based on the > > current patchset. To me, adding flags in a separate field makes code > > easier, but cost extra memory. I will see what other people say about > > this, firstly if the solution is acceptable, then reusing the union > > field or adding anohter flags. > > > > Could you try below code to see if it works? > > I tried the patch below and it worked for me: reading from vm_map_ram() > regions in drgn was fine, gave me the correct values, and I also tested > reads which overlapped the beginning and end of the region. Thanks a lot for the testing. > > That said (and I don't know the vmalloc code well at all), I wonder > whether you can be confident that the lower 2 bits of the va->vm pointer > are always clear? It looks like it comes from kmalloc, and so it should > be aligned, but can slab red zones mess up that alignment? Hmm, it should be OK. I am also worried about the other places of va->vm checking. I will check code again to see if there's risk in the case you mentioned. I may change to add another ->flags field into vmap_area in v2 post. > > I also tested out this patch on top of yours, to be a bit more cautious. > I think we can be confident that the remaining bits are zero when used > as flags, and non-zero when used as a pointer, so you can test them to > avoid any overlap. But it's probably too cautious. > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 78cae59170d8..911974f32b23 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -3613,7 +3613,7 @@ long vread(char *buf, char *addr, unsigned long count) > if (!va->vm) > continue; > > - flags = va->flags & VMAP_FLAGS_MASK; > + flags = (va->flags & ~VMAP_FLAGS_MASK) ? 0 : (va->flags & VMAP_FLAGS_MASK); > vm = va->vm; > > vaddr = (char *) va->va_start; >
