| Message ID | 20171219133236.GE13680@bombadil.infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue 19-12-17 05:32:36, Matthew Wilcox wrote: > > From: Matthew Wilcox <mawilcox@microsoft.com> > > With the recent addition of hashed kernel pointers, places which need > to produce useful debug output have to specify %px, not %p. This patch > fixes all the VM debug to use %px. This is appropriate because it's > debug output that the user should never be able to trigger, and kernel > developers need to see the actual pointers. Agreed. This is essentially a BUG_ON so we shouldn't hide information. I am just wondering why %px rather than %lx (like __show_regs e.g.)? > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com> Acked-by: Michal Hocko <mhocko@suse.com> > diff --git a/mm/debug.c b/mm/debug.c > index d947f3e03b0d..56e2d9125ea5 100644 > --- a/mm/debug.c > +++ b/mm/debug.c > @@ -50,7 +50,7 @@ void __dump_page(struct page *page, const char *reason) > */ > int mapcount = PageSlab(page) ? 0 : page_mapcount(page); > > - pr_emerg("page:%p count:%d mapcount:%d mapping:%p index:%#lx", > + pr_emerg("page:%px count:%d mapcount:%d mapping:%px index:%#lx", > page, page_ref_count(page), mapcount, > page->mapping, page_to_pgoff(page)); > if (PageCompound(page)) > @@ -69,7 +69,7 @@ void __dump_page(struct page *page, const char *reason) > > #ifdef CONFIG_MEMCG > if (page->mem_cgroup) > - pr_alert("page->mem_cgroup:%p\n", page->mem_cgroup); > + pr_alert("page->mem_cgroup:%px\n", page->mem_cgroup); > #endif > } > > @@ -84,10 +84,10 @@ EXPORT_SYMBOL(dump_page); > > void dump_vma(const struct vm_area_struct *vma) > { > - pr_emerg("vma %p start %p end %p\n" > - "next %p prev %p mm %p\n" > - "prot %lx anon_vma %p vm_ops %p\n" > - "pgoff %lx file %p private_data %p\n" > + pr_emerg("vma %px start %px end %px\n" > + "next %px prev %px mm %px\n" > + "prot %lx anon_vma %px vm_ops %px\n" > + "pgoff %lx file %px private_data %px\n" > "flags: %#lx(%pGv)\n", > vma, (void *)vma->vm_start, (void *)vma->vm_end, vma->vm_next, > vma->vm_prev, vma->vm_mm, > @@ -100,27 +100,27 @@ EXPORT_SYMBOL(dump_vma); > > void dump_mm(const struct mm_struct *mm) > { > - pr_emerg("mm %p mmap %p seqnum %d task_size %lu\n" > + pr_emerg("mm %px mmap %px seqnum %d task_size %lu\n" > #ifdef CONFIG_MMU > - "get_unmapped_area %p\n" > + "get_unmapped_area %px\n" > #endif > "mmap_base %lu mmap_legacy_base %lu highest_vm_end %lu\n" > - "pgd %p mm_users %d mm_count %d pgtables_bytes %lu map_count %d\n" > + "pgd %px mm_users %d mm_count %d pgtables_bytes %lu map_count %d\n" > "hiwater_rss %lx hiwater_vm %lx total_vm %lx locked_vm %lx\n" > "pinned_vm %lx data_vm %lx exec_vm %lx stack_vm %lx\n" > "start_code %lx end_code %lx start_data %lx end_data %lx\n" > "start_brk %lx brk %lx start_stack %lx\n" > "arg_start %lx arg_end %lx env_start %lx env_end %lx\n" > - "binfmt %p flags %lx core_state %p\n" > + "binfmt %px flags %lx core_state %px\n" > #ifdef CONFIG_AIO > - "ioctx_table %p\n" > + "ioctx_table %px\n" > #endif > #ifdef CONFIG_MEMCG > - "owner %p " > + "owner %px " > #endif > - "exe_file %p\n" > + "exe_file %px\n" > #ifdef CONFIG_MMU_NOTIFIER > - "mmu_notifier_mm %p\n" > + "mmu_notifier_mm %px\n" > #endif > #ifdef CONFIG_NUMA_BALANCING > "numa_next_scan %lu numa_scan_offset %lu numa_scan_seq %d\n" > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
On Tue, Dec 19, 2017 at 03:42:11PM +0100, Michal Hocko wrote: > On Tue 19-12-17 05:32:36, Matthew Wilcox wrote: > > > > From: Matthew Wilcox <mawilcox@microsoft.com> > > > > With the recent addition of hashed kernel pointers, places which need > > to produce useful debug output have to specify %px, not %p. This patch > > fixes all the VM debug to use %px. This is appropriate because it's > > debug output that the user should never be able to trigger, and kernel > > developers need to see the actual pointers. > > Agreed. This is essentially a BUG_ON so we shouldn't hide information. > I am just wondering why %px rather than %lx (like __show_regs e.g.)? commit 7b1924a1d930eb27fc79c4e4e2a6c1c970623e68 Author: Tobin C. Harding <me@tobin.cc> Date: Thu Nov 23 10:59:45 2017 +1100 vsprintf: add printk specifier %px printk specifier %p now hashes all addresses before printing. Sometimes we need to see the actual unmodified address. This can be achieved using %lx but then we face the risk that if in future we want to change the way the Kernel handles printing of pointers we will have to grep through the already existent 50 000 %lx call sites. Let's add specifier %px as a clear, opt-in, way to print a pointer and maintain some level of isolation from all the other hex integer output within the Kernel. Add printk specifier %px to print the actual unmodified address. Signed-off-by: Tobin C. Harding <me@tobin.cc> > > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com> > > Acked-by: Michal Hocko <mhocko@suse.com> Thanks. Andrew, will you take this, or does it go through the hardening tree?
On Tue, 19 Dec 2017 07:02:12 -0800 Matthew Wilcox <willy@infradead.org> wrote: > > > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com> > > > > Acked-by: Michal Hocko <mhocko@suse.com> > > Thanks. Andrew, will you take this, or does it go through the hardening tree? I've queued it for 4.15.
diff --git a/mm/debug.c b/mm/debug.c index d947f3e03b0d..56e2d9125ea5 100644 --- a/mm/debug.c +++ b/mm/debug.c @@ -50,7 +50,7 @@ void __dump_page(struct page *page, const char *reason) */ int mapcount = PageSlab(page) ? 0 : page_mapcount(page); - pr_emerg("page:%p count:%d mapcount:%d mapping:%p index:%#lx", + pr_emerg("page:%px count:%d mapcount:%d mapping:%px index:%#lx", page, page_ref_count(page), mapcount, page->mapping, page_to_pgoff(page)); if (PageCompound(page)) @@ -69,7 +69,7 @@ void __dump_page(struct page *page, const char *reason) #ifdef CONFIG_MEMCG if (page->mem_cgroup) - pr_alert("page->mem_cgroup:%p\n", page->mem_cgroup); + pr_alert("page->mem_cgroup:%px\n", page->mem_cgroup); #endif } @@ -84,10 +84,10 @@ EXPORT_SYMBOL(dump_page); void dump_vma(const struct vm_area_struct *vma) { - pr_emerg("vma %p start %p end %p\n" - "next %p prev %p mm %p\n" - "prot %lx anon_vma %p vm_ops %p\n" - "pgoff %lx file %p private_data %p\n" + pr_emerg("vma %px start %px end %px\n" + "next %px prev %px mm %px\n" + "prot %lx anon_vma %px vm_ops %px\n" + "pgoff %lx file %px private_data %px\n" "flags: %#lx(%pGv)\n", vma, (void *)vma->vm_start, (void *)vma->vm_end, vma->vm_next, vma->vm_prev, vma->vm_mm, @@ -100,27 +100,27 @@ EXPORT_SYMBOL(dump_vma); void dump_mm(const struct mm_struct *mm) { - pr_emerg("mm %p mmap %p seqnum %d task_size %lu\n" + pr_emerg("mm %px mmap %px seqnum %d task_size %lu\n" #ifdef CONFIG_MMU - "get_unmapped_area %p\n" + "get_unmapped_area %px\n" #endif "mmap_base %lu mmap_legacy_base %lu highest_vm_end %lu\n" - "pgd %p mm_users %d mm_count %d pgtables_bytes %lu map_count %d\n" + "pgd %px mm_users %d mm_count %d pgtables_bytes %lu map_count %d\n" "hiwater_rss %lx hiwater_vm %lx total_vm %lx locked_vm %lx\n" "pinned_vm %lx data_vm %lx exec_vm %lx stack_vm %lx\n" "start_code %lx end_code %lx start_data %lx end_data %lx\n" "start_brk %lx brk %lx start_stack %lx\n" "arg_start %lx arg_end %lx env_start %lx env_end %lx\n" - "binfmt %p flags %lx core_state %p\n" + "binfmt %px flags %lx core_state %px\n" #ifdef CONFIG_AIO - "ioctx_table %p\n" + "ioctx_table %px\n" #endif #ifdef CONFIG_MEMCG - "owner %p " + "owner %px " #endif - "exe_file %p\n" + "exe_file %px\n" #ifdef CONFIG_MMU_NOTIFIER - "mmu_notifier_mm %p\n" + "mmu_notifier_mm %px\n" #endif #ifdef CONFIG_NUMA_BALANCING "numa_next_scan %lu numa_scan_offset %lu numa_scan_seq %d\n"