diff mbox series

[RFCv4,2/4] lib/vsprintf.c: make '%pD' print full path for file

Message ID 20210615154952.2744-3-justin.he@arm.com (mailing list archive)
State New
Headers show
Series make '%pD' print full path for file | expand

Commit Message

Jia He June 15, 2021, 3:49 p.m. UTC
Previously, the specifier '%pD' is for printing dentry name of struct
file. It may not be perfect (by default it only prints one component.)

As suggested by Linus at [1]:
A dentry has a parent, but at the same time, a dentry really does
inherently have "one name" (and given just the dentry pointers, you
can't show mount-related parenthood, so in many ways the "show just
one name" makes sense for "%pd" in ways it doesn't necessarily for
"%pD"). But while a dentry arguably has that "one primary component",
a _file_ is certainly not exclusively about that last component.

Hence change the behavior of '%pD' to print full path of that file.

Precision is never going to be used with %p (or any of its kernel
extensions) if -Wformat is turned on.
.

[1] https://lore.kernel.org/lkml/CAHk-=wimsMqGdzik187YWLb-ru+iktb4MYbMQG1rnZ81dXYFVg@mail.gmail.com/

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jia He <justin.he@arm.com>
---
 Documentation/core-api/printk-formats.rst |  5 +--
 lib/vsprintf.c                            | 37 ++++++++++++++++++++---
 2 files changed, 36 insertions(+), 6 deletions(-)

Comments

Andy Shevchenko June 15, 2021, 8:44 p.m. UTC | #1
On Tue, Jun 15, 2021 at 6:54 PM Jia He <justin.he@arm.com> wrote:
>
> Previously, the specifier '%pD' is for printing dentry name of struct
> file. It may not be perfect (by default it only prints one component.)
>
> As suggested by Linus at [1]:
> A dentry has a parent, but at the same time, a dentry really does
> inherently have "one name" (and given just the dentry pointers, you
> can't show mount-related parenthood, so in many ways the "show just
> one name" makes sense for "%pd" in ways it doesn't necessarily for
> "%pD"). But while a dentry arguably has that "one primary component",
> a _file_ is certainly not exclusively about that last component.
>
> Hence change the behavior of '%pD' to print full path of that file.

print the full

>
> Precision is never going to be used with %p (or any of its kernel
> extensions) if -Wformat is turned on.
> .
>
> [1] https://lore.kernel.org/lkml/CAHk-=wimsMqGdzik187YWLb-ru+iktb4MYbMQG1rnZ81dXYFVg@mail.gmail.com/

Put it as a Link: tag?

>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Jia He <justin.he@arm.com>
> ---
>  Documentation/core-api/printk-formats.rst |  5 +--
>  lib/vsprintf.c                            | 37 ++++++++++++++++++++---
>  2 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index f063a384c7c8..95ba14dc529b 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -408,12 +408,13 @@ dentry names
>  ::
>
>         %pd{,2,3,4}
> -       %pD{,2,3,4}
> +       %pD
>
>  For printing dentry name; if we race with :c:func:`d_move`, the name might
>  be a mix of old and new ones, but it won't oops.  %pd dentry is a safer
>  equivalent of %s dentry->d_name.name we used to use, %pd<n> prints ``n``
> -last components.  %pD does the same thing for struct file.
> +last components.  %pD prints full file path together with mount-related
> +parenthood.
>
>  Passed by reference.
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index f0c35d9b65bf..9d3166332726 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -27,6 +27,7 @@
>  #include <linux/string.h>
>  #include <linux/ctype.h>
>  #include <linux/kernel.h>

> +#include <linux/dcache.h>

I know that this is an arbitrary order, but can you keep it after ctype.h?

>  #include <linux/kallsyms.h>
>  #include <linux/math64.h>
>  #include <linux/uaccess.h>
> @@ -920,13 +921,41 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
>  }
>
>  static noinline_for_stack
> -char *file_dentry_name(char *buf, char *end, const struct file *f,
> +char *file_d_path_name(char *buf, char *end, const struct file *f,
>                         struct printf_spec spec, const char *fmt)
>  {
> +       const struct path *path;
> +       char *p;
> +       int prepend_len, reserved_size, dpath_len;
> +
>         if (check_pointer(&buf, end, f, spec))
>                 return buf;
>
> -       return dentry_name(buf, end, f->f_path.dentry, spec, fmt);
> +       path = &f->f_path;
> +       if (check_pointer(&buf, end, path, spec))
> +               return buf;
> +
> +       p = d_path_unsafe(path, buf, end - buf, &prepend_len);
> +
> +       /* Calculate the full d_path length, ignoring the tail '\0' */
> +       dpath_len = end - buf - prepend_len - 1;
> +
> +       reserved_size = max_t(int, dpath_len, spec.field_width);
> +
> +       /* case 1: no space at all, forward the buf with reserved size */

Case 1:

> +       if (buf >= end)
> +               return buf + reserved_size;
> +
> +       /*
> +        * case 2: small scratch space for long d_path name. The space

Case 2:

> +        * [buf,end] has been filled with truncated string. Hence use the
> +        * full dpath_len for further string widening.
> +        */
> +       if (prepend_len < 0)
> +               return widen_string(buf + dpath_len, dpath_len, end, spec);
> +
> +       /* case3: space is big enough */

Case 3:

> +       return string_nocheck(buf, end, p, spec);
>  }
>  #ifdef CONFIG_BLOCK
>  static noinline_for_stack
> @@ -2296,7 +2325,7 @@ early_param("no_hash_pointers", no_hash_pointers_enable);
>   * - 'a[pd]' For address types [p] phys_addr_t, [d] dma_addr_t and derivatives
>   *           (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
> + * - 'D' For full path name of a struct file
>   * - 'g' For block_device name (gendisk + partition number)
>   * - 't[RT][dt][r]' For time and date as represented by:
>   *      R    struct rtc_time
> @@ -2395,7 +2424,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>         case 'C':
>                 return clock(buf, end, ptr, spec, fmt);
>         case 'D':
> -               return file_dentry_name(buf, end, ptr, spec, fmt);
> +               return file_d_path_name(buf, end, ptr, spec, fmt);
>  #ifdef CONFIG_BLOCK
>         case 'g':
>                 return bdev_name(buf, end, ptr, spec, fmt);
> --
> 2.17.1
>
Petr Mladek June 17, 2021, 2:09 p.m. UTC | #2
On Tue 2021-06-15 23:49:50, Jia He wrote:
> Previously, the specifier '%pD' is for printing dentry name of struct
> file. It may not be perfect (by default it only prints one component.)
> 
> As suggested by Linus at [1]:
> A dentry has a parent, but at the same time, a dentry really does
> inherently have "one name" (and given just the dentry pointers, you
> can't show mount-related parenthood, so in many ways the "show just
> one name" makes sense for "%pd" in ways it doesn't necessarily for
> "%pD"). But while a dentry arguably has that "one primary component",
> a _file_ is certainly not exclusively about that last component.
> 
> Hence change the behavior of '%pD' to print full path of that file.
> 
> Precision is never going to be used with %p (or any of its kernel
> extensions) if -Wformat is turned on.
> .
> 
> [1] https://lore.kernel.org/lkml/CAHk-=wimsMqGdzik187YWLb-ru+iktb4MYbMQG1rnZ81dXYFVg@mail.gmail.com/
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Jia He <justin.he@arm.com>

> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -920,13 +921,41 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
>  }
>  
>  static noinline_for_stack
> -char *file_dentry_name(char *buf, char *end, const struct file *f,
> +char *file_d_path_name(char *buf, char *end, const struct file *f,
>  			struct printf_spec spec, const char *fmt)
>  {
> +	const struct path *path;
> +	char *p;
> +	int prepend_len, reserved_size, dpath_len;
> +
>  	if (check_pointer(&buf, end, f, spec))
>  		return buf;
>  
> -	return dentry_name(buf, end, f->f_path.dentry, spec, fmt);
> +	path = &f->f_path;
> +	if (check_pointer(&buf, end, path, spec))
> +		return buf;
> +
> +	p = d_path_unsafe(path, buf, end - buf, &prepend_len);
> +
> +	/* Calculate the full d_path length, ignoring the tail '\0' */
> +	dpath_len = end - buf - prepend_len - 1;
> +
> +	reserved_size = max_t(int, dpath_len, spec.field_width);

"reserved_size" is kind of confusing. "dpath_widen_len" or just "widen_len"
look much more obvious.

The below comments are not bad. But they still made me thing about it
more than I wanted ;-) I wonder if it following is better:

> +	/* case 1: no space at all, forward the buf with reserved size */
> +	if (buf >= end)
> +		return buf + reserved_size;

	/* Case 1: Already started past the buffer. Just forward @buf. */
	if (buf >= end)
		return buf + widen_len;

> +
> +	/*
> +	 * case 2: small scratch space for long d_path name. The space
> +	 * [buf,end] has been filled with truncated string. Hence use the
> +	 * full dpath_len for further string widening.
> +	 */
> +	if (prepend_len < 0)
> +		return widen_string(buf + dpath_len, dpath_len, end, spec);

	/*
	 * Case 2: The entire remaining space of the buffer filled by
	 * the truncated path. Still need to get moved right when
	 * the filed width is greather than the full path length.
	 */
	if (prepend_len < 0)
		return widen_string(buf + dpath_len, dpath_len, end, spec);

> +	/* case3: space is big enough */
> +	return string_nocheck(buf, end, p, spec);

	/*
	 * Case 3: The full path is printed at the end of the buffer.
	 * Print it at the right location in the same buffer.
	 */
	return string_nocheck(buf, end, p, spec);
>  }
>  #ifdef CONFIG_BLOCK
>  static noinline_for_stack

In each case, I am happy that it was possible to simplify the logic.
I got lost several times in the previous version.

Best Regards,
Petr
Jia He June 22, 2021, 2:20 a.m. UTC | #3
Hi Petr

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Thursday, June 17, 2021 10:10 PM
> To: Justin He <Justin.He@arm.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>; Sergey Senozhatsky
> <senozhatsky@chromium.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Rasmus Villemoes
> <linux@rasmusvillemoes.dk>; Jonathan Corbet <corbet@lwn.net>; Alexander
> Viro <viro@zeniv.linux.org.uk>; Linus Torvalds <torvalds@linux-
> foundation.org>; Peter Zijlstra (Intel) <peterz@infradead.org>; Eric
> Biggers <ebiggers@google.com>; Ahmed S. Darwish <a.darwish@linutronix.de>;
> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> fsdevel@vger.kernel.org; Matthew Wilcox <willy@infradead.org>
> Subject: Re: [PATCH RFCv4 2/4] lib/vsprintf.c: make '%pD' print full path
> for file
>
> On Tue 2021-06-15 23:49:50, Jia He wrote:
> > Previously, the specifier '%pD' is for printing dentry name of struct
> > file. It may not be perfect (by default it only prints one component.)
> >
> > As suggested by Linus at [1]:
> > A dentry has a parent, but at the same time, a dentry really does
> > inherently have "one name" (and given just the dentry pointers, you
> > can't show mount-related parenthood, so in many ways the "show just
> > one name" makes sense for "%pd" in ways it doesn't necessarily for
> > "%pD"). But while a dentry arguably has that "one primary component",
> > a _file_ is certainly not exclusively about that last component.
> >
> > Hence change the behavior of '%pD' to print full path of that file.
> >
> > Precision is never going to be used with %p (or any of its kernel
> > extensions) if -Wformat is turned on.
> > .
> >
> > [1] https://lore.kernel.org/lkml/CAHk-=wimsMqGdzik187YWLb-
> ru+iktb4MYbMQG1rnZ81dXYFVg@mail.gmail.com/
> >
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Jia He <justin.he@arm.com>
>
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -920,13 +921,41 @@ char *dentry_name(char *buf, char *end, const
> struct dentry *d, struct printf_sp
> >  }
> >
> >  static noinline_for_stack
> > -char *file_dentry_name(char *buf, char *end, const struct file *f,
> > +char *file_d_path_name(char *buf, char *end, const struct file *f,
> >                     struct printf_spec spec, const char *fmt)
> >  {
> > +   const struct path *path;
> > +   char *p;
> > +   int prepend_len, reserved_size, dpath_len;
> > +
> >     if (check_pointer(&buf, end, f, spec))
> >             return buf;
> >
> > -   return dentry_name(buf, end, f->f_path.dentry, spec, fmt);
> > +   path = &f->f_path;
> > +   if (check_pointer(&buf, end, path, spec))
> > +           return buf;
> > +
> > +   p = d_path_unsafe(path, buf, end - buf, &prepend_len);
> > +
> > +   /* Calculate the full d_path length, ignoring the tail '\0' */
> > +   dpath_len = end - buf - prepend_len - 1;
> > +
> > +   reserved_size = max_t(int, dpath_len, spec.field_width);
>
> "reserved_size" is kind of confusing. "dpath_widen_len" or just "widen_len"
> look much more obvious.

Okay

>
> The below comments are not bad. But they still made me thing about it
> more than I wanted ;-) I wonder if it following is better:
>
> > +   /* case 1: no space at all, forward the buf with reserved size */
> > +   if (buf >= end)
> > +           return buf + reserved_size;
>
>       /* Case 1: Already started past the buffer. Just forward @buf. */
>       if (buf >= end)
>               return buf + widen_len;
>
Okay
> > +
> > +   /*
> > +    * case 2: small scratch space for long d_path name. The space
> > +    * [buf,end] has been filled with truncated string. Hence use the
> > +    * full dpath_len for further string widening.
> > +    */
> > +   if (prepend_len < 0)
> > +           return widen_string(buf + dpath_len, dpath_len, end, spec);
>
>       /*
>        * Case 2: The entire remaining space of the buffer filled by
>        * the truncated path. Still need to get moved right when
>        * the filed width is greather than the full path length.
>        */
>       if (prepend_len < 0)
>               return widen_string(buf + dpath_len, dpath_len, end, spec);
>
Okay
> > +   /* case3: space is big enough */
> > +   return string_nocheck(buf, end, p, spec);
>
>       /*
>        * Case 3: The full path is printed at the end of the buffer.
>        * Print it at the right location in the same buffer.
>        */
>       return string_nocheck(buf, end, p, spec);
Okay
> >  }
> >  #ifdef CONFIG_BLOCK
> >  static noinline_for_stack
>
> In each case, I am happy that it was possible to simplify the logic.
> I got lost several times in the previous version.

Indeed, the cases can be much simpler if we don't consider spec.precision.
More than that, maybe we could remove the spec.precision consideration for
'%pd' or other pointer related specifiers also


--
Cheers,
Justin (Jia He)


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
diff mbox series

Patch

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index f063a384c7c8..95ba14dc529b 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -408,12 +408,13 @@  dentry names
 ::
 
 	%pd{,2,3,4}
-	%pD{,2,3,4}
+	%pD
 
 For printing dentry name; if we race with :c:func:`d_move`, the name might
 be a mix of old and new ones, but it won't oops.  %pd dentry is a safer
 equivalent of %s dentry->d_name.name we used to use, %pd<n> prints ``n``
-last components.  %pD does the same thing for struct file.
+last components.  %pD prints full file path together with mount-related
+parenthood.
 
 Passed by reference.
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f0c35d9b65bf..9d3166332726 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -27,6 +27,7 @@ 
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/kernel.h>
+#include <linux/dcache.h>
 #include <linux/kallsyms.h>
 #include <linux/math64.h>
 #include <linux/uaccess.h>
@@ -920,13 +921,41 @@  char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 }
 
 static noinline_for_stack
-char *file_dentry_name(char *buf, char *end, const struct file *f,
+char *file_d_path_name(char *buf, char *end, const struct file *f,
 			struct printf_spec spec, const char *fmt)
 {
+	const struct path *path;
+	char *p;
+	int prepend_len, reserved_size, dpath_len;
+
 	if (check_pointer(&buf, end, f, spec))
 		return buf;
 
-	return dentry_name(buf, end, f->f_path.dentry, spec, fmt);
+	path = &f->f_path;
+	if (check_pointer(&buf, end, path, spec))
+		return buf;
+
+	p = d_path_unsafe(path, buf, end - buf, &prepend_len);
+
+	/* Calculate the full d_path length, ignoring the tail '\0' */
+	dpath_len = end - buf - prepend_len - 1;
+
+	reserved_size = max_t(int, dpath_len, spec.field_width);
+
+	/* case 1: no space at all, forward the buf with reserved size */
+	if (buf >= end)
+		return buf + reserved_size;
+
+	/*
+	 * case 2: small scratch space for long d_path name. The space
+	 * [buf,end] has been filled with truncated string. Hence use the
+	 * full dpath_len for further string widening.
+	 */
+	if (prepend_len < 0)
+		return widen_string(buf + dpath_len, dpath_len, end, spec);
+
+	/* case3: space is big enough */
+	return string_nocheck(buf, end, p, spec);
 }
 #ifdef CONFIG_BLOCK
 static noinline_for_stack
@@ -2296,7 +2325,7 @@  early_param("no_hash_pointers", no_hash_pointers_enable);
  * - 'a[pd]' For address types [p] phys_addr_t, [d] dma_addr_t and derivatives
  *           (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
+ * - 'D' For full path name of a struct file
  * - 'g' For block_device name (gendisk + partition number)
  * - 't[RT][dt][r]' For time and date as represented by:
  *      R    struct rtc_time
@@ -2395,7 +2424,7 @@  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'C':
 		return clock(buf, end, ptr, spec, fmt);
 	case 'D':
-		return file_dentry_name(buf, end, ptr, spec, fmt);
+		return file_d_path_name(buf, end, ptr, spec, fmt);
 #ifdef CONFIG_BLOCK
 	case 'g':
 		return bdev_name(buf, end, ptr, spec, fmt);