Date:   Thu, 27 May 2021 07:20:55 +0000
From:   Justin He <Justin.He@....com>
To:     Petr Mladek <pmladek@...e.com>
CC:     Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Jonathan Corbet <corbet@....net>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Al Viro <viro@....linux.org.uk>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Ira Weiny <ira.weiny@...el.com>,
        Eric Biggers <ebiggers@...gle.com>, nd <nd@....com>,
        "Ahmed S. Darwish" <a.darwish@...utronix.de>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>
Subject: RE: [PATCH RFC 2/3] lib/vsprintf.c: make %pD print full path for file

Hi Petr

> -----Original Message-----
> From: Petr Mladek <pmladek@...e.com>
> Sent: Monday, May 10, 2021 9:05 PM
> To: Justin He <Justin.He@....com>
> Cc: Steven Rostedt <rostedt@...dmis.org>; Sergey Senozhatsky
> <senozhatsky@...omium.org>; Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com>; Rasmus Villemoes
> <linux@...musvillemoes.dk>; Jonathan Corbet <corbet@....net>; Alexander
> Viro <viro@...iv.linux.org.uk>; Linus Torvalds <torvalds@...ux-
> foundation.org>; Al Viro <viro@....linux.org.uk>; Heiko Carstens
> <hca@...ux.ibm.com>; Vasily Gorbik <gor@...ux.ibm.com>; Christian
> Borntraeger <borntraeger@...ibm.com>; Eric W . Biederman
> <ebiederm@...ssion.com>; Darrick J. Wong <darrick.wong@...cle.com>; Peter
> Zijlstra (Intel) <peterz@...radead.org>; Ira Weiny <ira.weiny@...el.com>;
> Eric Biggers <ebiggers@...gle.com>; Ahmed S. Darwish
> <a.darwish@...utronix.de>; linux-doc@...r.kernel.org; linux-
> kernel@...r.kernel.org; linux-s390@...r.kernel.org; linux-
> fsdevel@...r.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)

Powered by blists - more mailing lists