diff mbox series

[RFC,2/3] lib/vsprintf.c: make %pD print full path for file

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

Commit Message

Justin He May 8, 2021, 12:25 p.m. UTC
We have '%pD' for printing a filename. 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 "file_dentry_name()" simply shouldn't use "dentry_name()" at all.
Despite that shared code origin, and despite that similar letter
choice (lower-vs-upper case), a dentry and a file really are very
different from a name standpoint.

[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                            | 12 ++++++++++--
 2 files changed, 13 insertions(+), 4 deletions(-)

Comments

Sergey Senozhatsky May 10, 2021, 3:46 a.m. UTC | #1
On (21/05/08 20:25), Jia He wrote:
> +++ 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>
> @@ -923,10 +924,17 @@ static noinline_for_stack
>  char *file_dentry_name(char *buf, char *end, const struct file *f,
>  			struct printf_spec spec, const char *fmt)
>  {
> +	const struct path *path = &f->f_path;
> +	char *p;
> +	char tmp[128];

This doesn't look ideal. Why 128 bytes and why on the stack?
Petr Mladek May 10, 2021, 1:04 p.m. UTC | #2
On Sat 2021-05-08 20:25:29, Jia He wrote:
> We have '%pD' for printing a filename. 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 "file_dentry_name()" simply shouldn't use "dentry_name()" at all.
> Despite that shared code origin, and despite that similar letter
> choice (lower-vs-upper case), a dentry and a file really are very
> different from a name standpoint.
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index f0c35d9b65bf..8220ab1411c5 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>
> @@ -923,10 +924,17 @@ static noinline_for_stack
>  char *file_dentry_name(char *buf, char *end, const struct file *f,
>  			struct printf_spec spec, const char *fmt)
>  {
> +	const struct path *path = &f->f_path;

This dereferences @f before it is checked by check_pointer().

> +	char *p;
> +	char tmp[128];
> +
>  	if (check_pointer(&buf, end, f, spec))
>  		return buf;
>  
> -	return dentry_name(buf, end, f->f_path.dentry, spec, fmt);
> +	p = d_path_fast(path, (char *)tmp, 128);
> +	buf = string(buf, end, p, spec);

Is 128 a limit of the path or just a compromise, please?

d_path_fast() limits the size of the buffer so we could use @buf
directly. We basically need to imitate what string_nocheck() does:

     + the length is limited by min(spec.precision, end-buf);
     + the string need to get shifted by widen_string()

We already do similar thing in dentry_name(). It might look like:

char *file_dentry_name(char *buf, char *end, const struct file *f,
			struct printf_spec spec, const char *fmt)
{
	const struct path *path;
	int lim, len;
	char *p;

	if (check_pointer(&buf, end, f, spec))
		return buf;

	path = &f->f_path;
	if (check_pointer(&buf, end, path, spec))
		return buf;

	lim = min(spec.precision, end - buf);
	p = d_path_fast(path, buf, lim);
	if (IS_ERR(p))
		return err_ptr(buf, end, p, spec);

	len = strlen(buf);
	return widen_string(buf + len, len, end, spec);
}

Note that the code is _not_ even compile tested. It might include
some ugly mistake.

> +
> +	return buf;
>  }
>  #ifdef CONFIG_BLOCK
>  static noinline_for_stack
> @@ -2296,7 +2304,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' Same as 'd' but for a struct file

It is not really the same. We should make it clear that it prints
the full path:

+   * - 'D' Same as 'd' but for a struct file; prints full path with
+       the mount-related parenthood

>   * - 'g' For block_device name (gendisk + partition number)
>   * - 't[RT][dt][r]' For time and date as represented by:
>   *      R    struct rtc_time
> -- 
> 2.17.1

Best Regards,
Petr
Justin He May 10, 2021, 2:25 p.m. UTC | #3
Hi Petr

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Monday, May 10, 2021 9:05 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>; Al Viro <viro@ftp.linux.org.uk>; Heiko Carstens
> <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-s390@vger.kernel.org; linux-
> fsdevel@vger.kernel.org
> Subject: Re: [PATCH RFC 2/3] lib/vsprintf.c: make %pD print full path for
> file
>
> On Sat 2021-05-08 20:25:29, Jia He wrote:
> > We have '%pD' for printing a filename. 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 "file_dentry_name()" simply shouldn't use "dentry_name()" at all.
> > Despite that shared code origin, and despite that similar letter
> > choice (lower-vs-upper case), a dentry and a file really are very
> > different from a name standpoint.
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index f0c35d9b65bf..8220ab1411c5 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>
> > @@ -923,10 +924,17 @@ static noinline_for_stack
> >  char *file_dentry_name(char *buf, char *end, const struct file *f,
> >                     struct printf_spec spec, const char *fmt)
> >  {
> > +   const struct path *path = &f->f_path;
>
> This dereferences @f before it is checked by check_pointer().

Okay

>
> > +   char *p;
> > +   char tmp[128];
> > +
> >     if (check_pointer(&buf, end, f, spec))
> >             return buf;
> >
> > -   return dentry_name(buf, end, f->f_path.dentry, spec, fmt);
> > +   p = d_path_fast(path, (char *)tmp, 128);
> > +   buf = string(buf, end, p, spec);
>
> Is 128 a limit of the path or just a compromise, please?

It is just a compromise.

>
> d_path_fast() limits the size of the buffer so we could use @buf
> directly. We basically need to imitate what string_nocheck() does:
>
>      + the length is limited by min(spec.precision, end-buf);
>      + the string need to get shifted by widen_string()
>
> We already do similar thing in dentry_name(). It might look like:
>
> char *file_dentry_name(char *buf, char *end, const struct file *f,
>                       struct printf_spec spec, const char *fmt)
> {
>       const struct path *path;
>       int lim, len;
>       char *p;
>
>       if (check_pointer(&buf, end, f, spec))
>               return buf;
>
>       path = &f->f_path;
>       if (check_pointer(&buf, end, path, spec))
>               return buf;
>
>       lim = min(spec.precision, end - buf);
>       p = d_path_fast(path, buf, lim);
>       if (IS_ERR(p))
>               return err_ptr(buf, end, p, spec);
>
>       len = strlen(buf);
>       return widen_string(buf + len, len, end, spec);
> }
>
> Note that the code is _not_ even compile tested. It might include
> some ugly mistake.

Okay, let me try it together with Linus's prepend_entries().
Thanks for the suggestion.
>
> > +
> > +   return buf;
> >  }
> >  #ifdef CONFIG_BLOCK
> >  static noinline_for_stack
> > @@ -2296,7 +2304,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' Same as 'd' but for a struct file
>
> It is not really the same. We should make it clear that it prints
> the full path:

Okay


--
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.
Justin He May 27, 2021, 7:20 a.m. UTC | #4
Hi Petr

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Monday, May 10, 2021 9:05 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>; Al Viro <viro@ftp.linux.org.uk>; Heiko Carstens
> <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-s390@vger.kernel.org; linux-
> fsdevel@vger.kernel.org
> Subject: Re: [PATCH RFC 2/3] lib/vsprintf.c: make %pD print full path for
> file
> 
> On Sat 2021-05-08 20:25:29, Jia He wrote:
> > We have '%pD' for printing a filename. 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 "file_dentry_name()" simply shouldn't use "dentry_name()" at all.
> > Despite that shared code origin, and despite that similar letter
> > choice (lower-vs-upper case), a dentry and a file really are very
> > different from a name standpoint.
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index f0c35d9b65bf..8220ab1411c5 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>
> > @@ -923,10 +924,17 @@ static noinline_for_stack
> >  char *file_dentry_name(char *buf, char *end, const struct file *f,
> >  			struct printf_spec spec, const char *fmt)
> >  {
> > +	const struct path *path = &f->f_path;
> 
> This dereferences @f before it is checked by check_pointer().
> 
> > +	char *p;
> > +	char tmp[128];
> > +
> >  	if (check_pointer(&buf, end, f, spec))
> >  		return buf;
> >
> > -	return dentry_name(buf, end, f->f_path.dentry, spec, fmt);
> > +	p = d_path_fast(path, (char *)tmp, 128);
> > +	buf = string(buf, end, p, spec);
> 
> Is 128 a limit of the path or just a compromise, please?
> 
> d_path_fast() limits the size of the buffer so we could use @buf
> directly. We basically need to imitate what string_nocheck() does:
> 
>      + the length is limited by min(spec.precision, end-buf);
>      + the string need to get shifted by widen_string()
> 
> We already do similar thing in dentry_name(). It might look like:
> 
> char *file_dentry_name(char *buf, char *end, const struct file *f,
> 			struct printf_spec spec, const char *fmt)
> {
> 	const struct path *path;
> 	int lim, len;
> 	char *p;
> 
> 	if (check_pointer(&buf, end, f, spec))
> 		return buf;
> 
> 	path = &f->f_path;
> 	if (check_pointer(&buf, end, path, spec))
> 		return buf;
> 
> 	lim = min(spec.precision, end - buf);
> 	p = d_path_fast(path, buf, lim);

After further think about it, I prefer to choose pass stack space instead of _buf_.

vsnprintf() should return the size it requires after formatting the string.
vprintk_store() will invoke 1st vsnprintf() will 8 bytes to get the reserve_size.
Then invoke 2nd printk_sprint()->vscnprintf()->vsnprintf() to fill the space.

Hence end-buf is <0 in the 1st vsnprintf case.

If I call d_path_fast(path, buf, lim) with _buf_ instead of stack space, the
logic in prepend_name should be changed a lot. 

What do you think of it?

---
Cheers,
Justin (Jia He)
Petr Mladek May 27, 2021, 9:14 a.m. UTC | #5
On Thu 2021-05-27 07:20:55, Justin He wrote:
> > > @@ -923,10 +924,17 @@ static noinline_for_stack
> > >  char *file_dentry_name(char *buf, char *end, const struct file *f,
> > >  			struct printf_spec spec, const char *fmt)
> > >  {
> > > +	const struct path *path = &f->f_path;
> > 
> > This dereferences @f before it is checked by check_pointer().
> > 
> > > +	char *p;
> > > +	char tmp[128];
> > > +
> > >  	if (check_pointer(&buf, end, f, spec))
> > >  		return buf;
> > >
> > > -	return dentry_name(buf, end, f->f_path.dentry, spec, fmt);
> > > +	p = d_path_fast(path, (char *)tmp, 128);
> > > +	buf = string(buf, end, p, spec);
> > 
> > Is 128 a limit of the path or just a compromise, please?
> > 
> > d_path_fast() limits the size of the buffer so we could use @buf
> > directly. We basically need to imitate what string_nocheck() does:
> > 
> >      + the length is limited by min(spec.precision, end-buf);
> >      + the string need to get shifted by widen_string()
> > 
> > We already do similar thing in dentry_name(). It might look like:
> > 
> > char *file_dentry_name(char *buf, char *end, const struct file *f,
> > 			struct printf_spec spec, const char *fmt)
> > {
> > 	const struct path *path;
> > 	int lim, len;
> > 	char *p;
> > 
> > 	if (check_pointer(&buf, end, f, spec))
> > 		return buf;
> > 
> > 	path = &f->f_path;
> > 	if (check_pointer(&buf, end, path, spec))
> > 		return buf;
> > 
> > 	lim = min(spec.precision, end - buf);
> > 	p = d_path_fast(path, buf, lim);
> 
> After further think about it, I prefer to choose pass stack space instead of _buf_.
> 
> vsnprintf() should return the size it requires after formatting the string.
> vprintk_store() will invoke 1st vsnprintf() will 8 bytes to get the reserve_size.
> Then invoke 2nd printk_sprint()->vscnprintf()->vsnprintf() to fill the space.
> 
> Hence end-buf is <0 in the 1st vsnprintf case.

Grr, you are right, I have completely missed this. I felt that there
must had been a catch but I did not see it.

> If I call d_path_fast(path, buf, lim) with _buf_ instead of stack space, the
> logic in prepend_name should be changed a lot. 
> 
> What do you think of it?

I wonder if vsprintf() could pass a bigger static buffer
when (str >= end). I would be safe if the dentry API only writes
to the buffer and does not depend on reading what has already
been written there. Then it will not matter that it is shared
between more vsprintf() callers.

It is a dirty hack. I do not have a good feeling about it. Of course,
a better solution would be when some dentry API just returns
the required size in this case.

Anyway, the buffer on stack would be more safe. It looks like a good
compromise. We could always improve it when it is not good enough in
the real life.

Best Regards,
Petr
diff mbox series

Patch

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index f063a384c7c8..6770130d32c8 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 does the same thing for struct file and prints full
+file path together with mount-related parenthood.
 
 Passed by reference.
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f0c35d9b65bf..8220ab1411c5 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>
@@ -923,10 +924,17 @@  static noinline_for_stack
 char *file_dentry_name(char *buf, char *end, const struct file *f,
 			struct printf_spec spec, const char *fmt)
 {
+	const struct path *path = &f->f_path;
+	char *p;
+	char tmp[128];
+
 	if (check_pointer(&buf, end, f, spec))
 		return buf;
 
-	return dentry_name(buf, end, f->f_path.dentry, spec, fmt);
+	p = d_path_fast(path, (char *)tmp, 128);
+	buf = string(buf, end, p, spec);
+
+	return buf;
 }
 #ifdef CONFIG_BLOCK
 static noinline_for_stack
@@ -2296,7 +2304,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' Same as 'd' but for 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