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

[Justin He <Justin.He@arm.com>]

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)