[RFC patch] vsprintf: Add %pav extension for print_vma_addr

[RFC patch] vsprintf: Add %pav extension for print_vma_addr

[Joe Perches]

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(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 2ec6d84..962f82c 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -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]
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 8243e2f..f4956c1 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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,
+	       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);
+
+	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);
+
+		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;
+}
+
+static noinline_for_stack
 char *address_val(char *buf, char *end, const void *addr,
 		  struct printf_spec spec, const char *fmt)
 {
 	unsigned long long num;
 
-	spec.flags |= SPECIAL | SMALL | ZEROPAD;
-	spec.base = 16;
-
 	switch (fmt[1]) {
+	case 'v':
+		return vma_addr(buf, end, addr, spec, fmt);
 	case 'd':
-		num = *(const dma_addr_t *)addr;
+		spec.flags |= SPECIAL | SMALL | ZEROPAD;
+		spec.base = 16;
 		spec.field_width = sizeof(dma_addr_t) * 2 + 2;
+		num = *(const dma_addr_t *)addr;
 		break;
 	case 'p':
 	default:
-		num = *(const phys_addr_t *)addr;
+		spec.flags |= SPECIAL | SMALL | ZEROPAD;
+		spec.base = 16;
 		spec.field_width = sizeof(phys_addr_t) * 2 + 2;
+		num = *(const phys_addr_t *)addr;
 		break;
 	}
 
@@ -1453,7 +1500,10 @@ int kptr_restrict __read_mostly;
  *              N no separator
  *            The maximum supported length is 64 bytes of the input. Consider
  *            to use print_hex_dump() for the larger input.
- * - 'a[pd]' For address types [p] phys_addr_t, [d] dma_addr_t and derivatives
+ * - 'a[pdv]' For address types:
+ *            [p] phys_addr_t and derivatives (resource_size_t)
+ *            [d] dma_addr_t
+ *            [v] vma_addr
  *           (default assumed to be phys_addr_t, passed by reference)
  * - 'd[234]' For a dentry name (optionally 2-4 last components)
  * - 'D[234]' Same as 'd' but for a struct file

Re: [RFC patch] vsprintf: Add %pav extension for print_vma_addr

[Andrew Morton]

On Tue, 26 May 2015 16:05:04 -0700 Joe Perches <joe@perches.com> 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:
> 
> ...
>
> @@ -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,
> +	       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.

> +	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.

> +		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;
> +}
>
> ...
>

Re: [RFC patch] vsprintf: Add %pav extension for print_vma_addr

[Joe Perches]

On Wed, 2015-05-27 at 14:04 -0700, Andrew Morton wrote:
> On Tue, 26 May 2015 16:05:04 -0700 Joe Perches <joe@perches.com> 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.
[]
> >  static noinline_for_stack
> > +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.
> 
> > +	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?

Re: [RFC patch] vsprintf: Add %pav extension for print_vma_addr

[Andrew Morton]

On Wed, 27 May 2015 14:09:31 -0700 Joe Perches <joe@perches.com> wrote:

> > Problems when CONFIG_PREEMPT=n.
> > 
> > > +	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?

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.

Re: [RFC patch] vsprintf: Add %pav extension for print_vma_addr

[Joe Perches]

On Wed, 2015-05-27 at 14:19 -0700, Andrew Morton wrote:
> On Wed, 27 May 2015 14:09:31 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > > Problems when CONFIG_PREEMPT=n.
> > > 
> > > > +	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?
> 
> 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(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 2ec6d84..962f82c 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -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]
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 8243e2f..bb4fa63 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1313,23 +1313,74 @@ 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,
+	       struct printf_spec spec, const char *fmt)
+{
+	struct mm_struct *mm = current->mm;
+	const char *errmsg;
+	struct vm_area_struct *vma;
+	char *page;
+	char *path;
+
+	/* if we are in atomic contexts (in exception stacks, etc.) */
+	if (preempt_count()) {
+		errmsg = "(atomic context)";
+		goto err_string;
+	}
+
+	down_read(&mm->mmap_sem);
+	vma = find_vma(mm, *(unsigned long *)addr);
+
+	if (!vma || !vma->vm_file) {
+		errmsg = "(find_vma failed)";
+		goto err_up_read;
+	}
+
+	page = (char *)__get_free_page(GFP_ATOMIC | __GFP_NOWARN);
+	if (!page) {
+		errmsg = "(__get_free_page failed)";
+		goto err_up_read;
+	}
+
+	path = d_path(&vma->vm_file->f_path, page, PAGE_SIZE);
+	if (IS_ERR(path))
+		path = "?";
+
+	buf += snprintf(buf, end > buf ? end - buf : 0, "%s[%lx+%lx]",
+			kbasename(path),
+			vma->vm_start, vma->vm_end - vma->vm_start);
+
+	free_page((unsigned long)page);
+	up_read(&mm->mmap_sem);
+	return buf;
+
+err_up_read:
+	up_read(&mm->mmap_sem);
+err_string:
+	return string(buf, end, errmsg, spec);
+}
+
+static noinline_for_stack
 char *address_val(char *buf, char *end, const void *addr,
 		  struct printf_spec spec, const char *fmt)
 {
 	unsigned long long num;
 
-	spec.flags |= SPECIAL | SMALL | ZEROPAD;
-	spec.base = 16;
-
 	switch (fmt[1]) {
+	case 'v':
+		return vma_addr(buf, end, addr, spec, fmt);
 	case 'd':
-		num = *(const dma_addr_t *)addr;
+		spec.flags |= SPECIAL | SMALL | ZEROPAD;
+		spec.base = 16;
 		spec.field_width = sizeof(dma_addr_t) * 2 + 2;
+		num = *(const dma_addr_t *)addr;
 		break;
 	case 'p':
 	default:
-		num = *(const phys_addr_t *)addr;
+		spec.flags |= SPECIAL | SMALL | ZEROPAD;
+		spec.base = 16;
 		spec.field_width = sizeof(phys_addr_t) * 2 + 2;
+		num = *(const phys_addr_t *)addr;
 		break;
 	}
 
@@ -1453,7 +1504,10 @@ int kptr_restrict __read_mostly;
  *              N no separator
  *            The maximum supported length is 64 bytes of the input. Consider
  *            to use print_hex_dump() for the larger input.
- * - 'a[pd]' For address types [p] phys_addr_t, [d] dma_addr_t and derivatives
+ * - 'a[pdv]' For address types:
+ *            [p] phys_addr_t and derivatives (resource_size_t)
+ *            [d] dma_addr_t
+ *            [v] vma_addr
  *           (default assumed to be phys_addr_t, passed by reference)
  * - 'd[234]' For a dentry name (optionally 2-4 last components)
  * - 'D[234]' Same as 'd' but for a struct file

Re: [RFC patch] vsprintf: Add %pav extension for print_vma_addr

[Andrew Morton]

On Sun, 31 May 2015 10:51:17 -0700 Joe Perches <joe@perches.com> wrote:

> On Wed, 2015-05-27 at 14:19 -0700, Andrew Morton wrote:
> > On Wed, 27 May 2015 14:09:31 -0700 Joe Perches <joe@perches.com> wrote:
> > 
> > > > Problems when CONFIG_PREEMPT=n.
> > > > 
> > > > > +	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?
> > 
> > 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.