-----Original Message-----
From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Sent: Saturday, June 12, 2021 5:28 AM
To: Justin He <redacted>; Petr Mladek <pmladek@suse.com>; Steven
Rostedt [off-list ref]; Sergey Senozhatsky
[off-list ref]; Andy Shevchenko
[off-list ref]; Jonathan Corbet [off-list ref];
Alexander Viro [off-list ref]; Linus Torvalds <torvalds@linux-
foundation.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>; Eric Biggers
[off-list ref]; Ahmed S. Darwish [off-list ref]; linux-
doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
fsdevel@vger.kernel.org
Subject: Re: [PATCH RFCv3 2/3] lib/vsprintf.c: make %pD print full path for
file

On 11/06/2021 17.59, 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 change the behavior of '%pD' to print full path of that file.

Things become more complicated when spec.precision and spec.field_width
is added in. string_truncate() is to handle the small space case for
'%pD' precision and field_width.

[1] https://lore.kernel.org/lkml/CAHk-=wimsMqGdzik187YWLb-

ru+iktb4MYbMQG1rnZ81dXYFVg@mail.gmail.com/

quoted

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jia He <redacted>
---
 Documentation/core-api/printk-formats.rst |  5 ++-
 lib/vsprintf.c                            | 47 +++++++++++++++++++++--
 2 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst

b/Documentation/core-api/printk-formats.rst

quoted

index f063a384c7c8..95ba14dc529b 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

quoted

 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 prints full file path together with mount-related
+parenthood.

 Passed by reference.

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f0c35d9b65bf..317b65280252 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>

@@ -601,6 +602,20 @@ char *widen_string(char *buf, int n, char *end,

struct printf_spec spec)

quoted

 }

 /* Handle string from a well known address. */
+static char *string_truncate(char *buf, char *end, const char *s,
+                        u32 full_len, struct printf_spec spec)
+{
+   int lim = 0;
+
+   if (buf < end) {

See below, I think the sole caller guarantees this,

quoted

+           if (spec.precision >= 0)
+                   lim = strlen(s) - min_t(int, spec.precision, strlen(s));
+
+           return widen_string(buf + full_len, full_len, end - lim, spec);
+   }
+
+   return buf;

which is good because this would almost certainly be wrong (violating
the "always forward buf appropriately regardless of whether you wrote
something" rule).

Sorry, I don't quite understand why it violates the rules here.

After removing the precision consideration, the codes should look like:
static char *string_truncate(char *buf, char *end, const char *s,
                                    u32 full_len, struct printf_spec spec)
{
        return widen_string(buf + full_len, full_len, end, spec);
}

Please note that in the case of small space with long string name,
The _buf_ had been filled with full path name:
e.g."/dev/testfile"
But the string might be truncated by the small space size.
e.g. "/dev/testf"
So we can't use the original string_nocheck here

Actually it doesn't backward buf here

--
Cheers,
Justin (Jia He)

quoted

+}
 static char *string_nocheck(char *buf, char *end, const char *s,
                        struct printf_spec spec)
 {

@@ -920,13 +935,37 @@ char *dentry_name(char *buf, char *end, const

struct dentry *d, struct printf_sp

quoted

 }

 static noinline_for_stack
-char *file_dentry_name(char *buf, char *end, const struct file *f,
+char *file_d_path_name(char *buf, char *end, const struct file *f,
                    struct printf_spec spec, const char *fmt)
 {
+   const struct path *path;
+   char *p;
+   int prepend_len, reserved_size, dpath_len;
+
    if (check_pointer(&buf, end, f, spec))
            return buf;

-   return dentry_name(buf, end, f->f_path.dentry, spec, fmt);
+   path = &f->f_path;
+   if (check_pointer(&buf, end, path, spec))
+           return buf;
+
+   p = d_path_unsafe(path, buf, end - buf, &prepend_len);

If I'm reading this right, you're using buf as scratch space to write
however much of the path fits. Then [*]

quoted

+   /* Minus 1 byte for '\0' */
+   dpath_len = end - buf - prepend_len - 1;
+
+   reserved_size = max_t(int, dpath_len, spec.field_width);
+
+   /* no filling space at all */
+   if (buf >= end || !buf)
+           return buf + reserved_size;

Why the !buf check? The only way we can have that is the snprintf(NULL,
0, ...) case of asking how much space we'd need to malloc, right? In
which case end would be NULL+0 == NULL, so buf >= end automatically,
regardless of how much have been "printed" before %pD.

quoted

+
+   /* small space for long name */
+   if (buf < end && prepend_len < 0)

So if we did an early return for buf >= end, we now know buf < end and
hence the first part here is redundant.

Anyway, as for [*]:

quoted

+           return string_truncate(buf, end, p, dpath_len, spec);
+
+   /* space is enough */
+   return string_nocheck(buf, end, p, spec);

Now you're passing p to string_truncate or string_nocheck, while p
points somewhere into buf itself. I can't convince myself that would be
safe. At the very least, it deserves a couple of comments.

Rasmus

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.