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

Hi Petr

-----Original Message-----
From: Petr Mladek <pmladek@suse.com>
Sent: Monday, May 10, 2021 9:05 PM
To: Justin He <redacted>
Cc: Steven Rostedt <rostedt@goodmis.org>; Sergey Senozhatsky
[off-list ref]; Andy Shevchenko
[off-list ref]; Rasmus Villemoes
[off-list ref]; Jonathan Corbet [off-list ref]; Alexander
Viro [off-list ref]; Linus Torvalds <torvalds@linux-
foundation.org>; Al Viro [off-list ref]; Heiko Carstens
[off-list ref]; Vasily Gorbik [off-list ref]; Christian
Borntraeger [off-list ref]; Eric W . Biederman
[off-list ref]; Darrick J. Wong [off-list ref]; Peter
Zijlstra (Intel) [off-list ref]; Ira Weiny [off-list ref];
Eric Biggers [off-list ref]; Ahmed S. Darwish
[off-list ref]; 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:

quoted

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().

quoted

+	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)