Hi Petr
> -----Original Message-----
> From: Petr Mladek <pmladek@xxxxxxxx>
> Sent: Monday, May 10, 2021 9:05 PM
> To: Justin He <Justin.He@xxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>; Sergey Senozhatsky
> <senozhatsky@xxxxxxxxxxxx>; Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx>; Rasmus Villemoes
> <linux@xxxxxxxxxxxxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>; Alexander
> Viro <viro@xxxxxxxxxxxxxxxxxx>; Linus Torvalds <torvalds@linux-
> foundation.org>; Al Viro <viro@xxxxxxxxxxxxxxxx>; Heiko Carstens
> <hca@xxxxxxxxxxxxx>; Vasily Gorbik <gor@xxxxxxxxxxxxx>; Christian
> Borntraeger <borntraeger@xxxxxxxxxx>; Eric W . Biederman
> <ebiederm@xxxxxxxxxxxx>; Darrick J. Wong <darrick.wong@xxxxxxxxxx>; Peter
> Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>; Ira Weiny <ira.weiny@xxxxxxxxx>;
> Eric Biggers <ebiggers@xxxxxxxxxx>; Ahmed S. Darwish
> <a.darwish@xxxxxxxxxxxxx>; linux-doc@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-s390@xxxxxxxxxxxxxxx; linux-
> fsdevel@xxxxxxxxxxxxxxx
> 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)
