Re: [RFC PATCH] vsprintf: Add %pv extension replacement for print_vma_addr
On Mon, 2020-08-17 at 14:44 +0300, Andy Shevchenko wrote: > On Fri, Aug 14, 2020 at 10:53:03AM -0700, Joe Perches wrote: > > There is a print_vma_addr function used several places > > that requires KERN_CONT use. > > > > Add a %pv mechanism to avoid the need for KERN_CONT. > > I like the idea, but I would accent the selling point to make code > (in the user call sites) nicer. [] > > +#else > > + buf = string_nocheck(buf, end, "CONFIG_MMU=n", default_str_spec); > > +#endif > > Can we avoid this spammy message? Really it's quite long and fills valuable > space in kernel buffer. I would rather print the hashed pointer as it's done > for the rest of %pXXX. Don't see why not. I believe it'd just use buf = ptr_to_id(buf, end, ip, spec); instead
Re: [RFC PATCH] vsprintf: Add %pv extension replacement for print_vma_addr
On Fri, Aug 14, 2020 at 10:53:03AM -0700, Joe Perches wrote: > There is a print_vma_addr function used several places > that requires KERN_CONT use. > > Add a %pv mechanism to avoid the need for KERN_CONT. I like the idea, but I would accent the selling point to make code (in the user call sites) nicer. > An example conversion is arch/x86/kernel/signal.c > > from: > if (show_unhandled_signals && printk_ratelimit()) { > printk("%s" > "%s[%d] bad frame in %s frame:%p ip:%lx sp:%lx orax:%lx", > task_pid_nr(current) > 1 ? KERN_INFO : KERN_EMERG, > me->comm, me->pid, where, frame, > regs->ip, regs->sp, regs->orig_ax); > print_vma_addr(KERN_CONT " in ", regs->ip); > pr_cont("\n"); > to: > printk("%s" > "%s[%d] bad frame in %s frame:%p ip:%lx sp:%lx orax:%lx > in %pv\n", > task_pid_nr(current) > 1 ? KERN_INFO : KERN_EMERG, > me->comm, me->pid, where, frame, > regs->ip, regs->sp, regs->orig_ax, (void *)regs->ip); ... > +#else > + buf = string_nocheck(buf, end, "CONFIG_MMU=n", default_str_spec); > +#endif Can we avoid this spammy message? Really it's quite long and fills valuable space in kernel buffer. I would rather print the hashed pointer as it's done for the rest of %pXXX. -- With Best Regards, Andy Shevchenko
Re: [RFC PATCH] vsprintf: Add %pv extension replacement for print_vma_addr
On Sat, 2020-08-15 at 12:52 +0900, Sergey Senozhatsky wrote: > Cc-ing John > > On (20/08/14 10:53), Joe Perches wrote: > [..] > > In general, the idea looks nice. > > > +static noinline_for_stack > > +char *vma_addr(char *buf, char *end, void *ip, > > + struct printf_spec spec, const char *fmt) > > +{ > > +#ifdef CONFIG_MMU > > + struct mm_struct *mm = current->mm; > > + struct vm_area_struct *vma; > > + > > + /* > > +* we might be running from an atomic context so we cannot sleep > > +*/ > > + if (!mmap_read_trylock(mm)) > > + return buf; > > + > > + vma = find_vma(mm, (unsigned long)ip); > > + if (vma && vma->vm_file) { > > + char *p; > > + struct file *f = vma->vm_file; > > + char *page = (char *)__get_free_page(GFP_NOWAIT); > > Hmm, this is huge. For the time being this is going to introduce lock > inversion chains: > > PRINTK -> printk_locks -> MM -> mm_locks > > vs > MM -> mm_locks -> PRINTK -> printk_locks > > Another thing to mention is > > PRINTK -> printk_locks -> MM (WANR_ON/etc) -> PRINTK > > we are in printk_safe, currently, but that's going to change. > > We might not be ready to take this as of now, but things can change > once we drop printk_locks. > > > + if (page) { > > + p = file_path(f, page, PAGE_SIZE); > > + if (IS_ERR(p)) > > + p = "?"; > > + buf = string(buf, end, kbasename(p), default_str_spec); > > + buf = string_nocheck(buf, end, "[", default_str_spec); > > + buf = pointer_string(buf, end, > > +(void *)vma->vm_start, > > +default_hex_spec); > > + buf = string_nocheck(buf, end, "+", default_str_spec); > > + buf = pointer_string(buf, end, > > +(void *)(vma->vm_end - > > vma->vm_start), > > +default_hex_spec); > > + buf = string_nocheck(buf, end, "]", default_str_spec); > > + free_page((unsigned long)page); > > + } > > + } > > + mmap_read_unlock(mm); > > +#else > > + buf = string_nocheck(buf, end, "CONFIG_MMU=n", default_str_spec); > > Hmm, we don't usually do that CONFIG_FOO=n thing, I think we just need > to skip %pv and print nothing. I don't. Right now, include/linux/mm.h has #ifdef CONFIG_MMU void print_vma_addr(char *prefix, unsigned long rip); #else static inline void print_vma_addr(char *prefix, unsigned long rip) { } #endif and the 'v' can't be #ifdef'd in vsprintf/pointer() otherwise %pv would fallthrough to return ptr_to_id(buf, end, ptr, spec);
Re: [RFC PATCH] vsprintf: Add %pv extension replacement for print_vma_addr
Cc-ing John On (20/08/14 10:53), Joe Perches wrote: [..] In general, the idea looks nice. > +static noinline_for_stack > +char *vma_addr(char *buf, char *end, void *ip, > +struct printf_spec spec, const char *fmt) > +{ > +#ifdef CONFIG_MMU > + struct mm_struct *mm = current->mm; > + struct vm_area_struct *vma; > + > + /* > + * we might be running from an atomic context so we cannot sleep > + */ > + if (!mmap_read_trylock(mm)) > + return buf; > + > + vma = find_vma(mm, (unsigned long)ip); > + if (vma && vma->vm_file) { > + char *p; > + struct file *f = vma->vm_file; > + char *page = (char *)__get_free_page(GFP_NOWAIT); Hmm, this is huge. For the time being this is going to introduce lock inversion chains: PRINTK -> printk_locks -> MM -> mm_locks vs MM -> mm_locks -> PRINTK -> printk_locks Another thing to mention is PRINTK -> printk_locks -> MM (WANR_ON/etc) -> PRINTK we are in printk_safe, currently, but that's going to change. We might not be ready to take this as of now, but things can change once we drop printk_locks. > + if (page) { > + p = file_path(f, page, PAGE_SIZE); > + if (IS_ERR(p)) > + p = "?"; > + buf = string(buf, end, kbasename(p), default_str_spec); > + buf = string_nocheck(buf, end, "[", default_str_spec); > + buf = pointer_string(buf, end, > + (void *)vma->vm_start, > + default_hex_spec); > + buf = string_nocheck(buf, end, "+", default_str_spec); > + buf = pointer_string(buf, end, > + (void *)(vma->vm_end - > vma->vm_start), > + default_hex_spec); > + buf = string_nocheck(buf, end, "]", default_str_spec); > + free_page((unsigned long)page); > + } > + } > + mmap_read_unlock(mm); > +#else > + buf = string_nocheck(buf, end, "CONFIG_MMU=n", default_str_spec); Hmm, we don't usually do that CONFIG_FOO=n thing, I think we just need to skip %pv and print nothing. Otherwise on !CONFIG_MMU systems the logbuf may contain a number of CONFIG_MMU=n messages, which are hardly useful. > +#endif > + return buf; > +} > + > /* > * Show a '%p' thing. A kernel extension is that the '%p' is followed > * by an extra set of alphanumeric characters that are extended format > @@ -2254,6 +2304,8 @@ char *pointer(const char *fmt, char *buf, char *end, > void *ptr, > return uuid_string(buf, end, ptr, spec, fmt); > case 'V': > return va_format(buf, end, ptr, spec, fmt); + #ifdef CONFIG_MMU > + case 'v': > + return vma_addr(buf, end, ptr, spec, fmt); + #endif > case 'K': > return restricted_pointer(buf, end, ptr, spec); > case 'N': -ss
Re: [RFC PATCH] vsprintf: Add %pv extension replacement for print_vma_addr
On Fri, 2020-08-14 at 14:38 -0400, Steven Rostedt wrote: > On Fri, 14 Aug 2020 10:53:03 -0700 > Joe Perches wrote: > I'm fine with all his, but I feel more comfortable if this patch > created a single copy of the code. Perhaps add: [] > diff --git a/mm/memory.c b/mm/memory.c [] > @@ -4771,32 +4771,7 @@ EXPORT_SYMBOL_GPL(access_process_vm); > */ > void print_vma_addr(char *prefix, unsigned long ip) > { > - struct mm_struct *mm = current->mm; > - struct vm_area_struct *vma; > - > - /* > - * we might be running from an atomic context so we cannot sleep > - */ > - if (!mmap_read_trylock(mm)) > - return; > - > - vma = find_vma(mm, ip); > - if (vma && vma->vm_file) { > - struct file *f = vma->vm_file; > - char *buf = (char *)__get_free_page(GFP_NOWAIT); > - if (buf) { > - char *p; > - > - p = file_path(f, buf, PAGE_SIZE); > - if (IS_ERR(p)) > - p = "?"; > - printk("%s%s[%lx+%lx]", prefix, kbasename(p), > - vma->vm_start, > - vma->vm_end - vma->vm_start); > - free_page((unsigned long)buf); > - } > - } > - mmap_read_unlock(mm); > + printk("%s%pv", prefix, ip); > } I'd just convert all of them and remove this function instead. Something like (uncompiled/untested): --- arch/arc/kernel/troubleshoot.c | 2 +- arch/arm64/kernel/traps.c | 16 +++-- arch/csky/kernel/traps.c | 11 - arch/mips/mm/fault.c | 14 +-- arch/parisc/mm/fault.c | 15 +--- arch/powerpc/kernel/traps.c| 8 ++- arch/riscv/kernel/traps.c | 11 - arch/s390/mm/fault.c | 7 +++--- arch/sparc/mm/fault_32.c | 8 ++- arch/sparc/mm/fault_64.c | 8 ++- arch/um/kernel/trap.c | 13 --- arch/x86/kernel/signal.c | 6 ++--- arch/x86/kernel/traps.c| 6 ++--- arch/x86/mm/fault.c| 53 +++--- include/linux/mm.h | 7 -- mm/memory.c| 33 -- 16 files changed, 74 insertions(+), 144 deletions(-) diff --git a/arch/arc/kernel/troubleshoot.c b/arch/arc/kernel/troubleshoot.c index 28e8bf04b253..26ecd59f0057 100644 --- a/arch/arc/kernel/troubleshoot.c +++ b/arch/arc/kernel/troubleshoot.c @@ -86,7 +86,7 @@ static void show_faulting_vma(unsigned long address) struct vm_area_struct *vma; struct mm_struct *active_mm = current->active_mm; - /* can't use print_vma_addr() yet as it doesn't check for + /* can't use %pv yet as it doesn't check for * non-inclusive vma */ mmap_read_lock(active_mm); diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index 13ebd5ca2070..294ed81f67d8 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -224,13 +224,15 @@ static void arm64_show_signal(int signo, const char *str) !__ratelimit()) return; - pr_info("%s[%d]: unhandled exception: ", tsk->comm, task_pid_nr(tsk)); - if (esr) - pr_cont("%s, ESR 0x%08x, ", esr_get_class_string(esr), esr); - - pr_cont("%s", str); - print_vma_addr(KERN_CONT " in ", regs->pc); - pr_cont("\n"); + if (esr) { + pr_info("%s[%d]: unhandled exception: %s, ESR 0x%08x, %s in %pv\n", + tsk->comm, task_pid_nr(tsk), + esr_get_class_string(esr), esr, + str, (void *)regs->pc); + } else { + pr_info("%s[%d]: unhandled exception: %s in %pv\n", + tsk->comm, task_pid_nr(tsk), str, (void *)regs->pc); + } __show_regs(regs); } diff --git a/arch/csky/kernel/traps.c b/arch/csky/kernel/traps.c index 959a917c989d..bdf1577237df 100644 --- a/arch/csky/kernel/traps.c +++ b/arch/csky/kernel/traps.c @@ -118,12 +118,11 @@ void do_trap(struct pt_regs *regs, int signo, int code, unsigned long addr) { struct task_struct *tsk = current; - if (show_unhandled_signals && unhandled_signal(tsk, signo) - && printk_ratelimit()) { - pr_info("%s[%d]: unhandled signal %d code 0x%x at 0x%08lx", - tsk->comm, task_pid_nr(tsk), signo, code, addr); - print_vma_addr(KERN_CONT " in ", instruction_pointer(regs)); - pr_cont("\n"); + if (show_unhandled_signals && unhandled_signal(tsk, signo) && + printk_ratelimit()) { + pr_info("%s[%d]: unhandled signal %d code 0x%x at 0x%08lx in %pv\n", + tsk->comm, task_pid_nr(tsk), signo, code, addr, + (void *)instruction_pointer(regs)); show_regs(regs); } diff --git
Re: [RFC PATCH] vsprintf: Add %pv extension replacement for print_vma_addr
On Fri, 14 Aug 2020 10:53:03 -0700 Joe Perches wrote: > There is a print_vma_addr function used several places > that requires KERN_CONT use. > > Add a %pv mechanism to avoid the need for KERN_CONT. > > An example conversion is arch/x86/kernel/signal.c > > from: > if (show_unhandled_signals && printk_ratelimit()) { > printk("%s" > "%s[%d] bad frame in %s frame:%p ip:%lx sp:%lx orax:%lx", > task_pid_nr(current) > 1 ? KERN_INFO : KERN_EMERG, > me->comm, me->pid, where, frame, > regs->ip, regs->sp, regs->orig_ax); > print_vma_addr(KERN_CONT " in ", regs->ip); > pr_cont("\n"); > to: > printk("%s" > "%s[%d] bad frame in %s frame:%p ip:%lx sp:%lx orax:%lx > in %pv\n", > task_pid_nr(current) > 1 ? KERN_INFO : KERN_EMERG, > me->comm, me->pid, where, frame, > regs->ip, regs->sp, regs->orig_ax, (void *)regs->ip); > --- > lib/vsprintf.c | 52 > 1 file changed, 52 insertions(+) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index c155769559ab..654402c43f8d 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -995,6 +995,12 @@ static const struct printf_spec default_dec_spec = { > .precision = -1, > }; > > +static const struct printf_spec default_hex_spec = { > + .base = 16, > + .precision = -1, > + .flags = SMALL, > +}; > + > static const struct printf_spec default_dec02_spec = { > .base = 10, > .field_width = 2, > @@ -2089,6 +2095,50 @@ char *fwnode_string(char *buf, char *end, struct > fwnode_handle *fwnode, > return widen_string(buf, buf - buf_start, end, spec); > } > > +static noinline_for_stack > +char *vma_addr(char *buf, char *end, void *ip, > +struct printf_spec spec, const char *fmt) > +{ > +#ifdef CONFIG_MMU > + struct mm_struct *mm = current->mm; > + struct vm_area_struct *vma; > + > + /* > + * we might be running from an atomic context so we cannot sleep > + */ > + if (!mmap_read_trylock(mm)) > + return buf; > + > + vma = find_vma(mm, (unsigned long)ip); > + if (vma && vma->vm_file) { > + char *p; > + struct file *f = vma->vm_file; > + char *page = (char *)__get_free_page(GFP_NOWAIT); > + > + if (page) { > + p = file_path(f, page, PAGE_SIZE); > + if (IS_ERR(p)) > + p = "?"; > + buf = string(buf, end, kbasename(p), default_str_spec); > + buf = string_nocheck(buf, end, "[", default_str_spec); > + buf = pointer_string(buf, end, > + (void *)vma->vm_start, > + default_hex_spec); > + buf = string_nocheck(buf, end, "+", default_str_spec); > + buf = pointer_string(buf, end, > + (void *)(vma->vm_end - > vma->vm_start), > + default_hex_spec); > + buf = string_nocheck(buf, end, "]", default_str_spec); > + free_page((unsigned long)page); > + } > + } > + mmap_read_unlock(mm); > +#else > + buf = string_nocheck(buf, end, "CONFIG_MMU=n", default_str_spec); > +#endif I'm fine with all his, but I feel more comfortable if this patch created a single copy of the code. Perhaps add: diff --git a/mm/memory.c b/mm/memory.c index 87ec87cdc1ff..795a4db4d83d 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -4771,32 +4771,7 @@ EXPORT_SYMBOL_GPL(access_process_vm); */ void print_vma_addr(char *prefix, unsigned long ip) { - struct mm_struct *mm = current->mm; - struct vm_area_struct *vma; - - /* -* we might be running from an atomic context so we cannot sleep -*/ - if (!mmap_read_trylock(mm)) - return; - - vma = find_vma(mm, ip); - if (vma && vma->vm_file) { - struct file *f = vma->vm_file; - char *buf = (char *)__get_free_page(GFP_NOWAIT); - if (buf) { - char *p; - - p = file_path(f, buf, PAGE_SIZE); - if (IS_ERR(p)) - p = "?"; - printk("%s%s[%lx+%lx]", prefix, kbasename(p), - vma->vm_start, - vma->vm_end - vma->vm_start); - free_page((unsigned long)buf); - } - } - mmap_read_unlock(mm); + printk("%s%pv", prefix, ip); } #if defined(CONFIG_PROVE_LOCKING) || defined(CONFIG_DEBUG_ATOMIC_SLEEP) And of course this would need for documentation. --
[RFC PATCH] vsprintf: Add %pv extension replacement for print_vma_addr
There is a print_vma_addr function used several places that requires KERN_CONT use. Add a %pv mechanism to avoid the need for KERN_CONT. An example conversion is arch/x86/kernel/signal.c from: if (show_unhandled_signals && printk_ratelimit()) { printk("%s" "%s[%d] bad frame in %s frame:%p ip:%lx sp:%lx orax:%lx", task_pid_nr(current) > 1 ? KERN_INFO : KERN_EMERG, me->comm, me->pid, where, frame, regs->ip, regs->sp, regs->orig_ax); print_vma_addr(KERN_CONT " in ", regs->ip); pr_cont("\n"); to: printk("%s" "%s[%d] bad frame in %s frame:%p ip:%lx sp:%lx orax:%lx in %pv\n", task_pid_nr(current) > 1 ? KERN_INFO : KERN_EMERG, me->comm, me->pid, where, frame, regs->ip, regs->sp, regs->orig_ax, (void *)regs->ip); --- lib/vsprintf.c | 52 1 file changed, 52 insertions(+) diff --git a/lib/vsprintf.c b/lib/vsprintf.c index c155769559ab..654402c43f8d 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -995,6 +995,12 @@ static const struct printf_spec default_dec_spec = { .precision = -1, }; +static const struct printf_spec default_hex_spec = { + .base = 16, + .precision = -1, + .flags = SMALL, +}; + static const struct printf_spec default_dec02_spec = { .base = 10, .field_width = 2, @@ -2089,6 +2095,50 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode, return widen_string(buf, buf - buf_start, end, spec); } +static noinline_for_stack +char *vma_addr(char *buf, char *end, void *ip, + struct printf_spec spec, const char *fmt) +{ +#ifdef CONFIG_MMU + struct mm_struct *mm = current->mm; + struct vm_area_struct *vma; + + /* +* we might be running from an atomic context so we cannot sleep +*/ + if (!mmap_read_trylock(mm)) + return buf; + + vma = find_vma(mm, (unsigned long)ip); + if (vma && vma->vm_file) { + char *p; + struct file *f = vma->vm_file; + char *page = (char *)__get_free_page(GFP_NOWAIT); + + if (page) { + p = file_path(f, page, PAGE_SIZE); + if (IS_ERR(p)) + p = "?"; + buf = string(buf, end, kbasename(p), default_str_spec); + buf = string_nocheck(buf, end, "[", default_str_spec); + buf = pointer_string(buf, end, +(void *)vma->vm_start, +default_hex_spec); + buf = string_nocheck(buf, end, "+", default_str_spec); + buf = pointer_string(buf, end, +(void *)(vma->vm_end - vma->vm_start), +default_hex_spec); + buf = string_nocheck(buf, end, "]", default_str_spec); + free_page((unsigned long)page); + } + } + mmap_read_unlock(mm); +#else + buf = string_nocheck(buf, end, "CONFIG_MMU=n", default_str_spec); +#endif + return buf; +} + /* * Show a '%p' thing. A kernel extension is that the '%p' is followed * by an extra set of alphanumeric characters that are extended format @@ -2254,6 +2304,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, return uuid_string(buf, end, ptr, spec, fmt); case 'V': return va_format(buf, end, ptr, spec, fmt); + case 'v': + return vma_addr(buf, end, ptr, spec, fmt); case 'K': return restricted_pointer(buf, end, ptr, spec); case 'N':