Skip to contentSkip to conversation

vsprintf: Add %pav extension for print_vma_addr

RFC PATCH In review Joe Perches 2 participants 6 messages May 26, 2015 – Jun 1, 2015

Subsystems (3)
Documentation Library code Vsprintf
lore
Share & tools
lore

Joe Perches

author
#0 lore raw

RFC PATCH 1/1 vsprintf: Add %pav extension for print_vma_addr

print_vma_addr is another function to emit useful data similar to print_symbol. The print_symbol functionality has been added via %p[fFsS] vsprintf extensions.

Perhaps it's appropriate to add vma_addr address decoding to vsprintf too.

This would allow code conversions where the very unlikely interleaving of messages from multiple threads might occur.

like arch/x86/kernel/traps.c:

from:

        if (show_unhandled_signals && unhandled_signal(tsk, signr) &&
            printk_ratelimit()) {
                pr_info("%s[%d] trap %s ip:%lx sp:%lx error:%lx",
                        tsk->comm, tsk->pid, str,
                        regs->ip, regs->sp, error_code);
                print_vma_addr(" in ", regs->ip);
                pr_cont("\n");
to:
                pr_info("%s[%d] trap %s ip:%lx sp:%lx error:%lx in %pav\n",
                        regs->ip, regs->sp, error_code, &regs->ip);
        }

Something like:

---

 Documentation/printk-formats.txt |  5 ++++
 lib/vsprintf.c                   | 62 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 61 insertions(+), 6 deletions(-)
Documentation/printk-formats.txt +5 -0
index 2ec6d84..962f82c 100644
@@ -86,6 +86,11 @@ DMA addresses types dma_addr_t:
 	For printing a dma_addr_t type which can vary based on build options,
 	regardless of the width of the CPU data path. Passed by reference.
 
+VMA addresses: where the content of an unsigned long * is used in find_vma()
+	%pav	"%s[%lx+%lx]",
+	        kbasename(d_path(vma->vm_file)),
+		vma->vm_start, vma->vm_end - vma->vm_start)
+
 Raw buffer as an escaped string:
 
 	%*pE[achnops]
lib/vsprintf.c +56 -6

86 lines not on this page · raw message

maintainer
#1 lore raw
Quoted 36 lines · On Tue, 26 May 2015 16:05:04 -0700 Joe Perches <[email protected]> wrote:

print_vma_addr is another function to emit useful data similar to print_symbol. The print_symbol functionality has been added via %p[fFsS] vsprintf extensions.

Perhaps it's appropriate to add vma_addr address decoding to vsprintf too.

This would allow code conversions where the very unlikely interleaving of messages from multiple threads might occur.

like arch/x86/kernel/traps.c:

from:

        if (show_unhandled_signals && unhandled_signal(tsk, signr) &&
            printk_ratelimit()) {
                pr_info("%s[%d] trap %s ip:%lx sp:%lx error:%lx",
                        tsk->comm, tsk->pid, str,
                        regs->ip, regs->sp, error_code);
                print_vma_addr(" in ", regs->ip);
                pr_cont("\n");
to:
                pr_info("%s[%d] trap %s ip:%lx sp:%lx error:%lx in %pav\n",
                        regs->ip, regs->sp, error_code, &regs->ip);
        }

Something like:

...

lib/vsprintf.c +1 -0
@@ -1313,23 +1313,70 @@ char *netdev_feature_string(char *buf, char *end, const u8 *addr,
 }
 
 static noinline_for_stack
+char *vma_addr(char *buf, char *end, const void *addr,

lib/vsprintf.c char *netdev_feature_string(char *buf, char *end, const u8 *addr, · line 1326

Quoted 10 lines
lib/vsprintf.c +10 -0
+	       struct printf_spec spec, const char *fmt)
+{
+	struct mm_struct *mm = current->mm;
+	struct vm_area_struct *vma;
+	unsigned long ip = *(unsigned long *)addr;
+	char *rtn;
+
+	/* if we are in atomic contexts (in exception stacks, etc.) */
+	if (preempt_count())
+		return string(buf, end, "(atomic context)", spec);

Problems when CONFIG_PREEMPT=n.

lib/vsprintf.c char *netdev_feature_string(char *buf, char *end, const u8 *addr, · line 1332

Quoted 5 lines
lib/vsprintf.c +5 -0
+	down_read(&mm->mmap_sem);
+	vma = find_vma(mm, ip);
+	if (vma && vma->vm_file) {
+		struct file *f = vma->vm_file;
+		char *gfp_buf = (char *)__get_free_page(GFP_KERNEL);

We shouldn't assume we can use GFP_KERNEL here. Even if the preempt_count() worked, we might be in a context which requires GFP_NOFS or GFP_NOIO.

Quoted 27 lines
lib/vsprintf.c +24 -0
+		if (gfp_buf) {
+			char *p = d_path(&f->f_path, gfp_buf, PAGE_SIZE);
+
+			if (IS_ERR(p))
+				p = "?";
+
+			rtn = buf + snprintf(buf, end > buf ? end - buf : 0,
+					     "%s[%lx+%lx]",
+					     kbasename(p),
+					     vma->vm_start,
+					     vma->vm_end - vma->vm_start);
+
+			free_page((unsigned long)gfp_buf);
+		} else {
+			rtn = string(buf, end, "(__get_free_page failed)", spec);
+		}
+	} else {
+		rtn = string(buf, end, "(find_vma failed)", spec);
+	}
+
+	up_read(&mm->mmap_sem);
+
+	return rtn;
+}
 
...
author
#2 lore raw
Quoted 8 lines · Andrew Morton
Quoted 7 lines · On Tue, 26 May 2015 16:05:04 -0700 Joe Perches <[email protected]> wrote:

print_vma_addr is another function to emit useful data similar to print_symbol. The print_symbol functionality has been added via %p[fFsS] vsprintf extensions.

Perhaps it's appropriate to add vma_addr address decoding to vsprintf too.

[]

Quoted 15 lines

lib/vsprintf.c char *netdev_feature_string(char *buf, char *end, const u8 *addr, · line 1326

Quoted 12 lines

static noinline_for_stack

lib/vsprintf.c +11 -0
+char *vma_addr(char *buf, char *end, const void *addr,
+	       struct printf_spec spec, const char *fmt)
+{
+	struct mm_struct *mm = current->mm;
+	struct vm_area_struct *vma;
+	unsigned long ip = *(unsigned long *)addr;
+	char *rtn;
+
+	/* if we are in atomic contexts (in exception stacks, etc.) */
+	if (preempt_count())
+		return string(buf, end, "(atomic context)", spec);

Problems when CONFIG_PREEMPT=n.

Quoted 9 lines

lib/vsprintf.c char *netdev_feature_string(char *buf, char *end, const u8 *addr, · line 1332

Quoted 5 lines
lib/vsprintf.c +5 -0
+	down_read(&mm->mmap_sem);
+	vma = find_vma(mm, ip);
+	if (vma && vma->vm_file) {
+		struct file *f = vma->vm_file;
+		char *gfp_buf = (char *)__get_free_page(GFP_KERNEL);

We shouldn't assume we can use GFP_KERNEL here. Even if the preempt_count() worked, we might be in a context which requires GFP_NOFS or GFP_NOIO.

This code is basically a copy of the existing print_vma_addr() so is that true for all the existing uses too?

maintainer
#3 lore raw
Quoted 12 lines · On Wed, 27 May 2015 14:09:31 -0700 Joe Perches <[email protected]> wrote:
Quoted 11 lines

Problems when CONFIG_PREEMPT=n.

lib/vsprintf.c char *netdev_feature_string(char *buf, char *end, const u8 *addr, · line 1332

Quoted 5 lines
lib/vsprintf.c +5 -0
+	down_read(&mm->mmap_sem);
+	vma = find_vma(mm, ip);
+	if (vma && vma->vm_file) {
+		struct file *f = vma->vm_file;
+		char *gfp_buf = (char *)__get_free_page(GFP_KERNEL);

We shouldn't assume we can use GFP_KERNEL here. Even if the preempt_count() worked, we might be in a context which requires GFP_NOFS or GFP_NOIO.

Quoted 2 lines

This code is basically a copy of the existing print_vma_addr() so is that true for all the existing uses too?

Yeah, the current code is pretty junky. But normally print_vma_addr() should never be called so nobody noticed...

In e8bff74a Ingo did a fiddle to preempt_conditional_sti() which looks like it will address the CONFIG_PREEMPT=n issue, but only on x86.

author
#4 lore raw
Quoted 14 lines · Andrew Morton
Quoted 12 lines · On Wed, 27 May 2015 14:09:31 -0700 Joe Perches <[email protected]> wrote:
Quoted 11 lines

Problems when CONFIG_PREEMPT=n.

Review

Quoted 5 lines
(quoted diff) +5 -0
+	down_read(&mm->mmap_sem);
+	vma = find_vma(mm, ip);
+	if (vma && vma->vm_file) {
+		struct file *f = vma->vm_file;
+		char *gfp_buf = (char *)__get_free_page(GFP_KERNEL);

We shouldn't assume we can use GFP_KERNEL here. Even if the preempt_count() worked, we might be in a context which requires GFP_NOFS or GFP_NOIO.

Quoted 8 lines
Quoted 2 lines

This code is basically a copy of the existing print_vma_addr() so is that true for all the existing uses too?

Yeah, the current code is pretty junky. But normally print_vma_addr() should never be called so nobody noticed...

In e8bff74a Ingo did a fiddle to preempt_conditional_sti() which looks like it will address the CONFIG_PREEMPT=n issue, but only on x86.

Maybe this? (using GFP_ATOMIC and __GFP_NOWARN)

 Documentation/printk-formats.txt |  5 +++
 lib/vsprintf.c                   | 66 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 65 insertions(+), 6 deletions(-)
Documentation/printk-formats.txt +5 -0
index 2ec6d84..962f82c 100644
@@ -86,6 +86,11 @@ DMA addresses types dma_addr_t:
 	For printing a dma_addr_t type which can vary based on build options,
 	regardless of the width of the CPU data path. Passed by reference.
 
+VMA addresses: where the content of an unsigned long * is used in find_vma()
+	%pav	"%s[%lx+%lx]",
+	        kbasename(d_path(vma->vm_file)),
+		vma->vm_start, vma->vm_end - vma->vm_start)
+
 Raw buffer as an escaped string:
 
 	%*pE[achnops]
lib/vsprintf.c +60 -6

90 lines not on this page · raw message

maintainer
#5 lore raw
Quoted 18 lines · On Sun, 31 May 2015 10:51:17 -0700 Joe Perches <[email protected]> wrote:
Quoted 17 lines · Andrew Morton
Quoted 14 lines · On Wed, 27 May 2015 14:09:31 -0700 Joe Perches <[email protected]> wrote:
Quoted 11 lines

Problems when CONFIG_PREEMPT=n.

Review

Quoted 5 lines
(quoted diff) +5 -0
+	down_read(&mm->mmap_sem);
+	vma = find_vma(mm, ip);
+	if (vma && vma->vm_file) {
+		struct file *f = vma->vm_file;
+		char *gfp_buf = (char *)__get_free_page(GFP_KERNEL);

We shouldn't assume we can use GFP_KERNEL here. Even if the preempt_count() worked, we might be in a context which requires GFP_NOFS or GFP_NOIO.

This code is basically a copy of the existing print_vma_addr() so is that true for all the existing uses too?

Quoted 7 lines
Quoted 5 lines

Yeah, the current code is pretty junky. But normally print_vma_addr() should never be called so nobody noticed...

In e8bff74a Ingo did a fiddle to preempt_conditional_sti() which looks like it will address the CONFIG_PREEMPT=n issue, but only on x86.

Maybe this? (using GFP_ATOMIC and __GFP_NOWARN)

(tries to remember what this is about)

Please do include the (updated) changelog each time.

This is intended to replace open-coded calls to print_vma_addr(), yes?

We shouldn't really need the memory allocation at all - we could poke these characters one at a time into the log buffer. That would require quite some hackery with d_path which would be rather pointless. Maybe for this purpose it's sufficient to just grab vma->vm_file->f_path->dentry->d_name. It's not as if this is terribly important code.

But we still have the down_read(mmap_sem) in there. I don't immediately see any deadlocks in the current callsites but it isn't obvious.

I dunno, I just don't think it's a nice idea to be allocating memory and taking mmap_sem and doing rcu_read_lock (in d_path) all within core vsprintf/printk. printk() is supposed to be callable from virtually any context, not "any context as long as you don't use features X Y and Z".

If we can make print_vma_addr() robust then OK. Perhaps we just don't try to print the vma's filename. People can look up the hex address in /proc/pid/maps if it's really needed.