* mmap for /proc/vmcore broken since 3.12-rc1
@ 2013-10-02 12:03 Michael Holzheu
2013-10-03 6:12 ` HATAYAMA Daisuke
0 siblings, 1 reply; 7+ messages in thread
From: Michael Holzheu @ 2013-10-02 12:03 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: David S. Miller, Vivek Goyal, HATAYAMA Daisuke, Jan Willeke,
linux-kernel, kexec
Hello Alexey,
Looks like the following commit broke mmap for /proc/vmcore:
commit c4fe24485729fc2cbff324c111e67a1cc2f9adea
Author: Alexey Dobriyan <adobriyan@gmail.com>
Date: Tue Aug 20 22:17:24 2013 +0300
sparc: fix PCI device proc file mmap(2)
Because /proc/vmcore (fs/proc/vmcore.c) does not implement the
get_unmapped_area() fops function mmap now always returns EIO.
Michael
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: mmap for /proc/vmcore broken since 3.12-rc1 2013-10-02 12:03 mmap for /proc/vmcore broken since 3.12-rc1 Michael Holzheu @ 2013-10-03 6:12 ` HATAYAMA Daisuke 2013-10-07 2:42 ` HATAYAMA Daisuke 0 siblings, 1 reply; 7+ messages in thread From: HATAYAMA Daisuke @ 2013-10-03 6:12 UTC (permalink / raw) To: Michael Holzheu, Alexey Dobriyan Cc: David S. Miller, Vivek Goyal, Jan Willeke, linux-kernel, kexec (2013/10/02 21:03), Michael Holzheu wrote: > Hello Alexey, > > Looks like the following commit broke mmap for /proc/vmcore: > > commit c4fe24485729fc2cbff324c111e67a1cc2f9adea > Author: Alexey Dobriyan <adobriyan@gmail.com> > Date: Tue Aug 20 22:17:24 2013 +0300 > > sparc: fix PCI device proc file mmap(2) > > Because /proc/vmcore (fs/proc/vmcore.c) does not implement the > get_unmapped_area() fops function mmap now always returns EIO. > > Michael > I confirmed the bug on v3.12-rc3. According to makedumpfile's log, mmap failed on /proc/vmcore. mem_map (271) mem_map : ffffea001da40000 pfn_start : 878000 pfn_end : 880000 Kernel can't mmap vmcore, using reads. STEP [Excluding unnecessary pages] : 1.268799 seconds STEP [Excluding unnecessary pages] : 1.268756 seconds STEP [Copying data ] : 44.847924 seconds Writing erase info... I'll post a patch later. -- Thanks. HATAYAMA, Daisuke ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: mmap for /proc/vmcore broken since 3.12-rc1 2013-10-03 6:12 ` HATAYAMA Daisuke @ 2013-10-07 2:42 ` HATAYAMA Daisuke 2013-10-08 12:49 ` Alexey Dobriyan 0 siblings, 1 reply; 7+ messages in thread From: HATAYAMA Daisuke @ 2013-10-07 2:42 UTC (permalink / raw) To: Michael Holzheu, Alexey Dobriyan Cc: David S. Miller, Vivek Goyal, Jan Willeke, linux-kernel, kexec (2013/10/03 15:12), HATAYAMA Daisuke wrote: > (2013/10/02 21:03), Michael Holzheu wrote: >> Hello Alexey, >> >> Looks like the following commit broke mmap for /proc/vmcore: >> >> commit c4fe24485729fc2cbff324c111e67a1cc2f9adea >> Author: Alexey Dobriyan <adobriyan@gmail.com> >> Date: Tue Aug 20 22:17:24 2013 +0300 >> >> sparc: fix PCI device proc file mmap(2) >> >> Because /proc/vmcore (fs/proc/vmcore.c) does not implement the >> get_unmapped_area() fops function mmap now always returns EIO. >> >> Michael >> > > I confirmed the bug on v3.12-rc3. According to makedumpfile's log, > mmap failed on /proc/vmcore. > > mem_map (271) > mem_map : ffffea001da40000 > pfn_start : 878000 > pfn_end : 880000 > Kernel can't mmap vmcore, using reads. > STEP [Excluding unnecessary pages] : 1.268799 seconds > STEP [Excluding unnecessary pages] : 1.268756 seconds > STEP [Copying data ] : 44.847924 seconds > Writing erase info... > > I'll post a patch later. > I've not completed this. I thought it was short task but after I tried to fix, makedumpfile became frequently failing with -ENOMEM and I'm not sure why even now. Here's current progress. First, on v3.12-rc3 mmap() on /proc/vmcore fails while returning -EIO. This is due to the commit c4fe24485729fc2cbff324c111e67a1cc2f9adea, just as reported by Holzheu, where proc_reg_get_unmapped_area was newly added to proc_reg_file_ops_no_compat file operations as get_unmapped_area method. Looking at get_unmapped_area function, it calls current->mm->get_unmapped_area at default, but calls f_ops->get_unmapped_area_function if it's assigned. get_area = current->mm->get_unmapped_area; if (file && file->f_op && file->f_op->get_unmapped_area) get_area = file->f_op->get_unmapped_area; addr = get_area(file, addr, len, pgoff, flags); if (IS_ERR_VALUE(addr)) return addr; For regular files in procfs, proc_reg_file_ops_no_compat is used first and then this behaves as wrapper. static unsigned long proc_reg_get_unmapped_area(struct file *file, unsigned long orig_addr, unsigned long len, unsigned long pgoff, unsigned long flags) { struct proc_dir_entry *pde = PDE(file_inode(file)); int rv = -EIO; unsigned long (*get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long); if (use_pde(pde)) { get_unmapped_area = pde->proc_fops->get_unmapped_area; if (get_unmapped_area) rv = get_unmapped_area(file, orig_addr, len, pgoff, flags); unuse_pde(pde); } return rv; } Since this was added in proc_reg_file_ops_no_compat, proc_reg_get_unmapped_area is used in get_unmapped_area now and it always returns -EIO since proc_vmcore_operations has no get_unmapped_area method now. So, immediate fix idea is to define get_unmapped_area method in proc_vmcore_operations and to design it so that it just calls current->mm->get_unmapped_area. --- fs/proc/vmcore.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c index 9100d69..9583419 100644 --- a/fs/proc/vmcore.c +++ b/fs/proc/vmcore.c @@ -412,10 +412,23 @@ static int mmap_vmcore(struct file *file, struct vm_area_struct *vma) } #endif +static unsigned long +get_unmapped_area_vmcore(struct file *filp, unsigned long addr, + unsigned long len, unsigned long pgoff, + unsigned long flags) +{ +#ifdef CONFIG_MMU + return current->mm->get_unmapped_area(filp, addr, len, pgoff, flags); +#else + return -EIO; +#endif +} + static const struct file_operations proc_vmcore_operations = { .read = read_vmcore, .llseek = default_llseek, .mmap = mmap_vmcore, + .get_unmapped_area = get_unmapped_area_vmcore, }; static struct vmcore* __init get_new_element(void) -- 1.8.3.1 However, after applying this patch, makedumpfile now somehow fails returning -ENOMEM frequently. It's about 50/128 on my box. Searching for where to return -ENOMEM in mmap path by printk debug, I found instance of get_unmapped_area returns kernel-space address: get_area = current->mm->get_unmapped_area; if (file && file->f_op && file->f_op->get_unmapped_area) get_area = file->f_op->get_unmapped_area; addr = get_area(file, addr, len, pgoff, flags); if (IS_ERR_VALUE(addr)) return addr; if (addr > TASK_SIZE - len) <---- Here return -ENOMEM; The log is: kdump:/# cd /mnt/ kdump:/mnt# for ((i=0; i<128; ++i)) ; do > makedumpfile -f -p -d 31 /proc/vmcore vmcore-pd31 > done The kernel version is not supported. The created dumpfile may be incomplete. cyclic buffer size has been changed: 65535 => 64512 [ 49.462536] addr: 0xffffffff8ef28000 [ 49.463686] TASK_SIZE: 0x007ffffffff000 [ 49.464952] len: 0x00000000400000 Note that makedumpfile tries to mmap some area in 4MiB size here, get_unmapped_area tries to find some area to cover such requested 4MiB size within user-space address space limit but it returns somehow kernel-space address. The actual instance of current->mm->get_unmapped_area on my environment is arch_get_unmapped_area_topdown. Finally, note: I tried to run makedumpfile using mmap on /proc/vmcore 128-times. Then, - v3.12-rc3 returns -EIO in every case (trivial), - v3.12-rc3 with the above patch applied returns -ENOMEM at 50/128, - v3.12-rc3 with commit c4fe24485729fc2cbff324c111e67a1cc2f9adea reverted works well in every case, and - v3.11.1-201.fc19.x86_64 works well in every case. So, I suspect procfs wrapper affects arch_get_unmapped_region_topdown? Any comments are helpful. -- Thanks. HATAYAMA, Daisuke ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: mmap for /proc/vmcore broken since 3.12-rc1 2013-10-07 2:42 ` HATAYAMA Daisuke @ 2013-10-08 12:49 ` Alexey Dobriyan 2013-10-09 10:14 ` HATAYAMA Daisuke 0 siblings, 1 reply; 7+ messages in thread From: Alexey Dobriyan @ 2013-10-08 12:49 UTC (permalink / raw) To: HATAYAMA Daisuke Cc: Michael Holzheu, David S. Miller, Vivek Goyal, Jan Willeke, Linux Kernel, kexec On Mon, Oct 7, 2013 at 5:42 AM, HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote: > +static unsigned long > +get_unmapped_area_vmcore(struct file *filp, unsigned long addr, > + unsigned long len, unsigned long pgoff, > + unsigned long flags) > +{ > +#ifdef CONFIG_MMU > + return current->mm->get_unmapped_area(filp, addr, len, pgoff, > flags); > +#else > + return -EIO; > +#endif > +} > + > static const struct file_operations proc_vmcore_operations = { > .read = read_vmcore, > .llseek = default_llseek, > .mmap = mmap_vmcore, > + .get_unmapped_area = get_unmapped_area_vmcore, I think current->mm->get_unmapped_area should be used by core proc code. ENOMEM bug looks unrelated though. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: mmap for /proc/vmcore broken since 3.12-rc1 2013-10-08 12:49 ` Alexey Dobriyan @ 2013-10-09 10:14 ` HATAYAMA Daisuke 2013-10-12 20:32 ` Alexey Dobriyan 0 siblings, 1 reply; 7+ messages in thread From: HATAYAMA Daisuke @ 2013-10-09 10:14 UTC (permalink / raw) To: Alexey Dobriyan Cc: Michael Holzheu, David S. Miller, Vivek Goyal, Jan Willeke, Linux Kernel, kexec Hello, (2013/10/08 21:49), Alexey Dobriyan wrote: > On Mon, Oct 7, 2013 at 5:42 AM, HATAYAMA Daisuke > <d.hatayama@jp.fujitsu.com> wrote: > >> +static unsigned long >> +get_unmapped_area_vmcore(struct file *filp, unsigned long addr, >> + unsigned long len, unsigned long pgoff, >> + unsigned long flags) >> +{ >> +#ifdef CONFIG_MMU >> + return current->mm->get_unmapped_area(filp, addr, len, pgoff, >> flags); >> +#else >> + return -EIO; >> +#endif >> +} >> + >> static const struct file_operations proc_vmcore_operations = { >> .read = read_vmcore, >> .llseek = default_llseek, >> .mmap = mmap_vmcore, >> + .get_unmapped_area = get_unmapped_area_vmcore, > > I think current->mm->get_unmapped_area should be used by core proc code. What do you actually suggest here? You mean moving this code in proc code? I don't think you suggest so. > ENOMEM bug looks unrelated though. > Next step I'll do is to look into vm_unmapped_region() that looks for for a region fit to a given mmap request and returns its address. In particular, I'll focus on when vm_unmapped_region() could return kernel-space address. -- Thanks. HATAYAMA, Daisuke ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: mmap for /proc/vmcore broken since 3.12-rc1 2013-10-09 10:14 ` HATAYAMA Daisuke @ 2013-10-12 20:32 ` Alexey Dobriyan 2013-10-14 4:52 ` HATAYAMA Daisuke 0 siblings, 1 reply; 7+ messages in thread From: Alexey Dobriyan @ 2013-10-12 20:32 UTC (permalink / raw) To: HATAYAMA Daisuke Cc: Michael Holzheu, David S. Miller, Vivek Goyal, Jan Willeke, Linux Kernel, kexec On Wed, Oct 09, 2013 at 07:14:55PM +0900, HATAYAMA Daisuke wrote: > Hello, > > (2013/10/08 21:49), Alexey Dobriyan wrote: > > On Mon, Oct 7, 2013 at 5:42 AM, HATAYAMA Daisuke > > <d.hatayama@jp.fujitsu.com> wrote: > > > >> +static unsigned long > >> +get_unmapped_area_vmcore(struct file *filp, unsigned long addr, > >> + unsigned long len, unsigned long pgoff, > >> + unsigned long flags) > >> +{ > >> +#ifdef CONFIG_MMU > >> + return current->mm->get_unmapped_area(filp, addr, len, pgoff, > >> flags); > >> +#else > >> + return -EIO; > >> +#endif > >> +} > >> + > >> static const struct file_operations proc_vmcore_operations = { > >> .read = read_vmcore, > >> .llseek = default_llseek, > >> .mmap = mmap_vmcore, > >> + .get_unmapped_area = get_unmapped_area_vmcore, > > > > I think current->mm->get_unmapped_area should be used by core proc code. > > What do you actually suggest here? You mean moving this code in proc code? > I don't think you suggest so. Please, try this patch, I don't have kexec setup handy. --- a/fs/proc/inode.c +++ b/fs/proc/inode.c @@ -291,7 +291,11 @@ static unsigned long proc_reg_get_unmapped_area(struct file *file, unsigned long int rv = -EIO; unsigned long (*get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long); if (use_pde(pde)) { - get_unmapped_area = pde->proc_fops->get_unmapped_area; + get_unmapped_area = current->mm->get_unmapped_area; +#ifdef CONFIG_MMU + if (pde->proc_fops->get_unmapped_area) + get_unmapped_area = pde->proc_fops->get_unmapped_area; +#endif if (get_unmapped_area) rv = get_unmapped_area(file, orig_addr, len, pgoff, flags); unuse_pde(pde); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: mmap for /proc/vmcore broken since 3.12-rc1 2013-10-12 20:32 ` Alexey Dobriyan @ 2013-10-14 4:52 ` HATAYAMA Daisuke 0 siblings, 0 replies; 7+ messages in thread From: HATAYAMA Daisuke @ 2013-10-14 4:52 UTC (permalink / raw) To: Alexey Dobriyan Cc: Michael Holzheu, David S. Miller, Vivek Goyal, Jan Willeke, Linux Kernel, kexec (2013/10/13 5:32), Alexey Dobriyan wrote: > On Wed, Oct 09, 2013 at 07:14:55PM +0900, HATAYAMA Daisuke wrote: >> Hello, >> >> (2013/10/08 21:49), Alexey Dobriyan wrote: >>> On Mon, Oct 7, 2013 at 5:42 AM, HATAYAMA Daisuke >>> <d.hatayama@jp.fujitsu.com> wrote: >>> >>>> +static unsigned long >>>> +get_unmapped_area_vmcore(struct file *filp, unsigned long addr, >>>> + unsigned long len, unsigned long pgoff, >>>> + unsigned long flags) >>>> +{ >>>> +#ifdef CONFIG_MMU >>>> + return current->mm->get_unmapped_area(filp, addr, len, pgoff, >>>> flags); >>>> +#else >>>> + return -EIO; >>>> +#endif >>>> +} >>>> + >>>> static const struct file_operations proc_vmcore_operations = { >>>> .read = read_vmcore, >>>> .llseek = default_llseek, >>>> .mmap = mmap_vmcore, >>>> + .get_unmapped_area = get_unmapped_area_vmcore, >>> >>> I think current->mm->get_unmapped_area should be used by core proc code. >> >> What do you actually suggest here? You mean moving this code in proc code? >> I don't think you suggest so. > > Please, try this patch, I don't have kexec setup handy. > > --- a/fs/proc/inode.c > +++ b/fs/proc/inode.c > @@ -291,7 +291,11 @@ static unsigned long proc_reg_get_unmapped_area(struct file *file, unsigned long > int rv = -EIO; > unsigned long (*get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long); > if (use_pde(pde)) { > - get_unmapped_area = pde->proc_fops->get_unmapped_area; > + get_unmapped_area = current->mm->get_unmapped_area; > +#ifdef CONFIG_MMU > + if (pde->proc_fops->get_unmapped_area) > + get_unmapped_area = pde->proc_fops->get_unmapped_area; > +#endif > if (get_unmapped_area) > rv = get_unmapped_area(file, orig_addr, len, pgoff, flags); > unuse_pde(pde); > Slight modification to #ifdef ... get_unmapped_area = NULL; #ifdef CONFIG_MMU get_unmapped_area = current->mm->get_unmapped_area #endif if (pde->proc_fops->get_unmapped_area) get_unmapped_area = pde->proc_fops->get_unmapped_area; And, I found the bug. The variable rv should have been defined as unsigned long. sizeof(int) is 4 bytes but sizeof(long) is 8 bytes at least on x86_64. The reason why returned value looked like kernel virtual address was due to signed extension performed during conversion from negative 32-bit signed integer to 64-bit unsigned long integer. Hmm, I first checked signature of related functions but overlooked... Anyway, I'll post fixing patch soon. -- Thanks. HATAYAMA, Daisuke ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-10-14 4:57 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-02 12:03 mmap for /proc/vmcore broken since 3.12-rc1 Michael Holzheu 2013-10-03 6:12 ` HATAYAMA Daisuke 2013-10-07 2:42 ` HATAYAMA Daisuke 2013-10-08 12:49 ` Alexey Dobriyan 2013-10-09 10:14 ` HATAYAMA Daisuke 2013-10-12 20:32 ` Alexey Dobriyan 2013-10-14 4:52 ` HATAYAMA Daisuke
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox