[PATCH RFCv4 0/4] make '%pD' print full path for file

[PATCH RFCv4 0/4] make '%pD' print full path for file

[Jia He]

Background
==========
Linus suggested printing full path for file instead of printing
the components as '%pd'.

Typically, there is no need for printk specifiers to take any real locks
(ie mount_lock or rename_lock). So I introduce a new helper d_path_fast
which is similar to d_path except it doesn't take any seqlock/spinlock.

This series is based on Al Viro's d_path cleanup patches [1] which
lifted the inner lockless loop into a new helper. 

[1] https://lkml.org/lkml/2021/5/18/1260

Test
====
The cases I tested:
1. print '%pD' with full path of ext4 file
2. mount a ext4 filesystem upon a ext4 filesystem, and print the file
   with '%pD'
3. all test_print selftests, including the new '%14pD' '%-14pD'
4. kasnprintf
   
Changelog
=========
v4:
- don't support spec.precision anymore for '%pD'
- add Rasmus's patch into this series
 
v3:
- implement new d_path_unsafe to use [buf, end] instead of stack space for
  filling bytes (by Matthew)
- add new test cases for '%pD'
- drop patch "hmcdrv: remove the redundant directory path" before removing rfc.

v2: 
- implement new d_path_fast based on Al Viro's patches
- add check_pointer check (by Petr)
- change the max full path size to 256 in stack space
v1: https://lkml.org/lkml/2021/5/8/122

Jia He (4):
  fs: introduce helper d_path_unsafe()
  lib/vsprintf.c: make '%pD' print full path for file
  lib/test_printf.c: split write-beyond-buffer check in two
  lib/test_printf.c: add test cases for '%pD'

 Documentation/core-api/printk-formats.rst |  5 +-
 fs/d_path.c                               | 83 ++++++++++++++++++++++-
 include/linux/dcache.h                    |  1 +
 lib/test_printf.c                         | 31 ++++++++-
 lib/vsprintf.c                            | 37 ++++++++--
 5 files changed, 148 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH RFCv4 1/4] fs: introduce helper d_path_unsafe()

[Jia He]

This helper is similar to d_path except that it doesn't take any
seqlock/spinlock. It is typical for debugging purpose. Besides,
an additional return value *prenpend_len* is used to get the full
path length of the dentry.

prepend_name_with_len() enhances the behavior of prepend_name().
Previously it will skip the loop at once in __prepen_path() when the
space is not enough. __prepend_path() gets the full length of dentry
together with the parent recusively.

Besides, if someone invokes snprintf with small but positive space,
prepend_name_with() needs to move and copy the string partially.

More than that, kasnprintf will pass NULL _buf_ and _end_, hence
it returns at the very beginning with false in this case;

Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Jia He <justin.he@arm.com>
---
 fs/d_path.c            | 83 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/dcache.h |  1 +
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/fs/d_path.c b/fs/d_path.c
index 23a53f7b5c71..4fc224eadf58 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -68,9 +68,66 @@ static bool prepend_name(struct prepend_buffer *p, const struct qstr *name)
 	return true;
 }
 
+static bool prepend_name_with_len(struct prepend_buffer *p, const struct qstr *name,
+			 int orig_buflen)
+{
+	const char *dname = smp_load_acquire(&name->name); /* ^^^ */
+	int dlen = READ_ONCE(name->len);
+	char *s;
+	int last_len = p->len;
+
+	p->len -= dlen + 1;
+
+	if (unlikely(!p->buf))
+		return false;
+
+	if (orig_buflen <= 0)
+		return false;
+	/*
+	 * The first time we overflow the buffer. Then fill the string
+	 * partially from the beginning
+	 */
+	if (unlikely(p->len < 0)) {
+		int buflen = strlen(p->buf);
+
+		s = p->buf;
+
+		/* Still have small space to fill partially */
+		if (last_len > 0) {
+			p->buf -= last_len;
+			buflen += last_len;
+		}
+
+		if (buflen > dlen + 1) {
+			/* This dentry name can be fully filled */
+			memmove(p->buf + dlen + 1, s, buflen - dlen - 1);
+			p->buf[0] = '/';
+			memcpy(p->buf + 1, dname, dlen);
+		} else if (buflen > 0) {
+			/* Partially filled, and drop last dentry name */
+			p->buf[0] = '/';
+			memcpy(p->buf + 1, dname, buflen - 1);
+		}
+
+		return false;
+	}
+
+	s = p->buf -= dlen + 1;
+	*s++ = '/';
+	while (dlen--) {
+		char c = *dname++;
+
+		if (!c)
+			break;
+		*s++ = c;
+	}
+	return true;
+}
 static int __prepend_path(const struct dentry *dentry, const struct mount *mnt,
 			  const struct path *root, struct prepend_buffer *p)
 {
+	int orig_buflen = p->len;
+
 	while (dentry != root->dentry || &mnt->mnt != root->mnt) {
 		const struct dentry *parent = READ_ONCE(dentry->d_parent);
 
@@ -97,8 +154,7 @@ static int __prepend_path(const struct dentry *dentry, const struct mount *mnt,
 			return 3;
 
 		prefetch(parent);
-		if (!prepend_name(p, &dentry->d_name))
-			break;
+		prepend_name_with_len(p, &dentry->d_name, orig_buflen);
 		dentry = parent;
 	}
 	return 0;
@@ -263,6 +319,29 @@ char *d_path(const struct path *path, char *buf, int buflen)
 }
 EXPORT_SYMBOL(d_path);
 
+/**
+ * d_path_unsafe - fast return the full path of a dentry without taking
+ * any seqlock/spinlock. This helper is typical for debugging purpose.
+ */
+char *d_path_unsafe(const struct path *path, char *buf, int buflen,
+		    int *prepend_len)
+{
+	struct path root;
+	struct mount *mnt = real_mount(path->mnt);
+	DECLARE_BUFFER(b, buf, buflen);
+
+	rcu_read_lock();
+	get_fs_root_rcu(current->fs, &root);
+
+	prepend(&b, "", 1);
+	__prepend_path(path->dentry, mnt, &root, &b);
+	rcu_read_unlock();
+
+	*prepend_len = b.len;
+
+	return b.buf;
+}
+
 /*
  * Helper function for dentry_operations.d_dname() members
  */
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 9e23d33bb6f1..ec118b684055 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -301,6 +301,7 @@ char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
 extern char *__d_path(const struct path *, const struct path *, char *, int);
 extern char *d_absolute_path(const struct path *, char *, int);
 extern char *d_path(const struct path *, char *, int);
+extern char *d_path_unsafe(const struct path *, char *, int, int*);
 extern char *dentry_path_raw(const struct dentry *, char *, int);
 extern char *dentry_path(const struct dentry *, char *, int);
 
-- 
2.17.1

[PATCH RFCv4 2/4] lib/vsprintf.c: make '%pD' print full path for file

[Jia He]

Previously, the specifier '%pD' is for printing dentry name of struct
file. 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.

Precision is never going to be used with %p (or any of its kernel
extensions) if -Wformat is turned on.
.

[1] https://lore.kernel.org/lkml/CAHk-=wimsMqGdzik187YWLb-ru+iktb4MYbMQG1rnZ81dXYFVg@mail.gmail.com/

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

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
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
 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..9d3166332726 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>
@@ -920,13 +921,41 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 }
 
 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);
+
+	/* Calculate the full d_path length, ignoring the tail '\0' */
+	dpath_len = end - buf - prepend_len - 1;
+
+	reserved_size = max_t(int, dpath_len, spec.field_width);
+
+	/* case 1: no space at all, forward the buf with reserved size */
+	if (buf >= end)
+		return buf + reserved_size;
+
+	/*
+	 * case 2: small scratch space for long d_path name. The space
+	 * [buf,end] has been filled with truncated string. Hence use the
+	 * full dpath_len for further string widening.
+	 */
+	if (prepend_len < 0)
+		return widen_string(buf + dpath_len, dpath_len, end, spec);
+
+	/* case3: space is big enough */
+	return string_nocheck(buf, end, p, spec);
 }
 #ifdef CONFIG_BLOCK
 static noinline_for_stack
@@ -2296,7 +2325,7 @@ early_param("no_hash_pointers", no_hash_pointers_enable);
  * - 'a[pd]' For address types [p] phys_addr_t, [d] dma_addr_t and derivatives
  *           (default assumed to be phys_addr_t, passed by reference)
  * - 'd[234]' For a dentry name (optionally 2-4 last components)
- * - 'D[234]' Same as 'd' but for a struct file
+ * - 'D' For full path name of a struct file
  * - 'g' For block_device name (gendisk + partition number)
  * - 't[RT][dt][r]' For time and date as represented by:
  *      R    struct rtc_time
@@ -2395,7 +2424,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'C':
 		return clock(buf, end, ptr, spec, fmt);
 	case 'D':
-		return file_dentry_name(buf, end, ptr, spec, fmt);
+		return file_d_path_name(buf, end, ptr, spec, fmt);
 #ifdef CONFIG_BLOCK
 	case 'g':
 		return bdev_name(buf, end, ptr, spec, fmt);
-- 
2.17.1

[PATCH RFCv4 3/4] lib/test_printf.c: split write-beyond-buffer check in two

[Jia He]

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Before each invocation of vsnprintf(), do_test() memsets the entire
allocated buffer to a sentinel value. That buffer includes leading and
trailing padding which is never included in the buffer area handed to
vsnprintf (spaces merely for clarity):

  pad  test_buffer      pad
  **** **************** ****

Then vsnprintf() is invoked with a bufsize argument <=
BUF_SIZE. Suppose bufsize=10, then we'd have e.g.

 |pad |   test_buffer    |pad |
  **** pizza0 **** ****** ****
 A    B      C    D           E

where vsnprintf() was given the area from B to D.

It is obviously a bug for vsnprintf to touch anything between A and B
or between D and E. The former is checked for as one would expect. But
for the latter, we are actually a little stricter in that we check the
area between C and E.

Split that check in two, providing a clearer error message in case it
was a genuine buffer overrun and not merely a write within the
provided buffer, but after the end of the generated string.

So far, no part of the vsnprintf() implementation has had any use for
using the whole buffer as scratch space, but it's not unreasonable to
allow that, as long as the result is properly nul-terminated and the
return value is the right one. However, it is somewhat unusual, and
most %<something> won't need this, so keep the [C,D] check, but make
it easy for a later patch to make that part opt-out for certain tests.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Tested-by: Jia He <justin.he@arm.com>
Signed-off-by: Jia He <justin.he@arm.com>
---
 lib/test_printf.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index ec0d5976bb69..d1d2f898ebae 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -78,12 +78,17 @@ do_test(int bufsize, const char *expect, int elen,
 		return 1;
 	}
 
-	if (memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1))) {
+	if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) {
 		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n",
 			bufsize, fmt);
 		return 1;
 	}
 
+	if (memchr_inv(test_buffer + bufsize, FILL_CHAR, BUF_SIZE + PAD_SIZE - bufsize)) {
+		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond buffer\n", bufsize, fmt);
+		return 1;
+	}
+
 	if (memcmp(test_buffer, expect, written)) {
 		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'\n",
 			bufsize, fmt, test_buffer, written, expect);
-- 
2.17.1

[PATCH RFCv4 4/4] lib/test_printf.c: add test cases for '%pD'

[Jia He]

After the behaviour of specifier '%pD' is changed to print full path
of struct file, the related test cases are also updated.

Given the string of '%pD' is prepended from the end of the buffer, the
check of "wrote beyond the nul-terminator" should be skipped.

Signed-off-by: Jia He <justin.he@arm.com>
---
 lib/test_printf.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index d1d2f898ebae..9f851a82b3af 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -16,6 +16,7 @@
 
 #include <linux/bitmap.h>
 #include <linux/dcache.h>
+#include <linux/fs.h>
 #include <linux/socket.h>
 #include <linux/in.h>
 
@@ -34,6 +35,7 @@ KSTM_MODULE_GLOBALS();
 
 static char *test_buffer __initdata;
 static char *alloced_buffer __initdata;
+static bool is_prepended_buf __initdata;
 
 extern bool no_hash_pointers;
 
@@ -78,7 +80,7 @@ do_test(int bufsize, const char *expect, int elen,
 		return 1;
 	}
 
-	if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) {
+	if (!is_prepended_buf && memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) {
 		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n",
 			bufsize, fmt);
 		return 1;
@@ -501,6 +503,27 @@ dentry(void)
 	test("  bravo/alfa|  bravo/alfa", "%12pd2|%*pd2", &test_dentry[2], 12, &test_dentry[2]);
 }
 
+static struct vfsmount test_vfsmnt __initdata = {};
+
+static struct file test_file __initdata = {
+	.f_path = { .dentry = &test_dentry[2],
+		    .mnt = &test_vfsmnt,
+	},
+};
+
+static void __init
+f_d_path(void)
+{
+	test("(null)", "%pD", NULL);
+	test("(efault)", "%pD", PTR_INVALID);
+
+	is_prepended_buf = true;
+	test("/bravo/alfa   |/bravo/alfa   ", "%-14pD|%*pD", &test_file, -14, &test_file);
+	test("   /bravo/alfa|   /bravo/alfa", "%14pD|%*pD", &test_file, 14, &test_file);
+	test("   /bravo/alfa|/bravo/alfa   ", "%14pD|%-14pD", &test_file, &test_file);
+	is_prepended_buf = false;
+}
+
 static void __init
 struct_va_format(void)
 {
@@ -784,6 +807,7 @@ test_pointer(void)
 	ip();
 	uuid();
 	dentry();
+	f_d_path();
 	struct_va_format();
 	time_and_date();
 	struct_clk();
-- 
2.17.1

Re: [PATCH RFCv4 1/4] fs: introduce helper d_path_unsafe()

[Andy Shevchenko]

On Tue, Jun 15, 2021 at 6:56 PM Jia He <justin.he@arm.com> wrote:
>
> This helper is similar to d_path except that it doesn't take any
> seqlock/spinlock. It is typical for debugging purpose. Besides,

purposes

> an additional return value *prenpend_len* is used to get the full
> path length of the dentry.
>
> prepend_name_with_len() enhances the behavior of prepend_name().
> Previously it will skip the loop at once in __prepen_path() when the
> space is not enough. __prepend_path() gets the full length of dentry
> together with the parent recusively.

recursively

>
> Besides, if someone invokes snprintf with small but positive space,
> prepend_name_with() needs to move and copy the string partially.
>
> More than that, kasnprintf will pass NULL _buf_ and _end_, hence

kasprintf()

> it returns at the very beginning with false in this case;
>
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Jia He <justin.he@arm.com>
> ---
>  fs/d_path.c            | 83 +++++++++++++++++++++++++++++++++++++++++-
>  include/linux/dcache.h |  1 +
>  2 files changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/fs/d_path.c b/fs/d_path.c
> index 23a53f7b5c71..4fc224eadf58 100644
> --- a/fs/d_path.c
> +++ b/fs/d_path.c
> @@ -68,9 +68,66 @@ static bool prepend_name(struct prepend_buffer *p, const struct qstr *name)
>         return true;
>  }
>
> +static bool prepend_name_with_len(struct prepend_buffer *p, const struct qstr *name,
> +                        int orig_buflen)
> +{
> +       const char *dname = smp_load_acquire(&name->name); /* ^^^ */

What does this funny comment mean?

> +       int dlen = READ_ONCE(name->len);
> +       char *s;
> +       int last_len = p->len;
> +
> +       p->len -= dlen + 1;
> +
> +       if (unlikely(!p->buf))
> +               return false;
> +
> +       if (orig_buflen <= 0)
> +               return false;
> +       /*
> +        * The first time we overflow the buffer. Then fill the string
> +        * partially from the beginning
> +        */
> +       if (unlikely(p->len < 0)) {
> +               int buflen = strlen(p->buf);
> +
> +               s = p->buf;
> +
> +               /* Still have small space to fill partially */
> +               if (last_len > 0) {
> +                       p->buf -= last_len;
> +                       buflen += last_len;
> +               }
> +
> +               if (buflen > dlen + 1) {
> +                       /* This dentry name can be fully filled */
> +                       memmove(p->buf + dlen + 1, s, buflen - dlen - 1);
> +                       p->buf[0] = '/';
> +                       memcpy(p->buf + 1, dname, dlen);
> +               } else if (buflen > 0) {
> +                       /* Partially filled, and drop last dentry name */
> +                       p->buf[0] = '/';
> +                       memcpy(p->buf + 1, dname, buflen - 1);
> +               }
> +
> +               return false;
> +       }
> +
> +       s = p->buf -= dlen + 1;
> +       *s++ = '/';

> +       while (dlen--) {
> +               char c = *dname++;
> +
> +               if (!c)
> +                       break;
> +               *s++ = c;

I'm wondering why can't memcpy() be used here as well.

> +       }
> +       return true;
> +}
>  static int __prepend_path(const struct dentry *dentry, const struct mount *mnt,
>                           const struct path *root, struct prepend_buffer *p)
>  {
> +       int orig_buflen = p->len;
> +
>         while (dentry != root->dentry || &mnt->mnt != root->mnt) {
>                 const struct dentry *parent = READ_ONCE(dentry->d_parent);
>
> @@ -97,8 +154,7 @@ static int __prepend_path(const struct dentry *dentry, const struct mount *mnt,
>                         return 3;
>
>                 prefetch(parent);
> -               if (!prepend_name(p, &dentry->d_name))
> -                       break;
> +               prepend_name_with_len(p, &dentry->d_name, orig_buflen);
>                 dentry = parent;
>         }
>         return 0;
> @@ -263,6 +319,29 @@ char *d_path(const struct path *path, char *buf, int buflen)
>  }
>  EXPORT_SYMBOL(d_path);
>
> +/**
> + * d_path_unsafe - fast return the full path of a dentry without taking
> + * any seqlock/spinlock. This helper is typical for debugging purpose.

purposes

Haven't you got kernel doc validation warnings? Please, describe
parameters as well.

> + */
> +char *d_path_unsafe(const struct path *path, char *buf, int buflen,
> +                   int *prepend_len)
> +{
> +       struct path root;
> +       struct mount *mnt = real_mount(path->mnt);
> +       DECLARE_BUFFER(b, buf, buflen);
> +
> +       rcu_read_lock();
> +       get_fs_root_rcu(current->fs, &root);
> +
> +       prepend(&b, "", 1);
> +       __prepend_path(path->dentry, mnt, &root, &b);
> +       rcu_read_unlock();
> +
> +       *prepend_len = b.len;
> +
> +       return b.buf;
> +}
> +
>  /*
>   * Helper function for dentry_operations.d_dname() members
>   */
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 9e23d33bb6f1..ec118b684055 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -301,6 +301,7 @@ char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
>  extern char *__d_path(const struct path *, const struct path *, char *, int);
>  extern char *d_absolute_path(const struct path *, char *, int);
>  extern char *d_path(const struct path *, char *, int);
> +extern char *d_path_unsafe(const struct path *, char *, int, int*);
>  extern char *dentry_path_raw(const struct dentry *, char *, int);
>  extern char *dentry_path(const struct dentry *, char *, int);
>
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFCv4 0/4] make '%pD' print full path for file

[Andy Shevchenko]

On Tue, Jun 15, 2021 at 6:54 PM Jia He <justin.he@arm.com> wrote:
>
> Background
> ==========
> Linus suggested printing full path for file instead of printing

the full
the file

> the components as '%pd'.
>
> Typically, there is no need for printk specifiers to take any real locks
> (ie mount_lock or rename_lock). So I introduce a new helper d_path_fast
> which is similar to d_path except it doesn't take any seqlock/spinlock.
>
> This series is based on Al Viro's d_path cleanup patches [1] which
> lifted the inner lockless loop into a new helper.
>
> [1] https://lkml.org/lkml/2021/5/18/1260
>
> Test
> ====
> The cases I tested:
> 1. print '%pD' with full path of ext4 file
> 2. mount a ext4 filesystem upon a ext4 filesystem, and print the file
>    with '%pD'
> 3. all test_print selftests, including the new '%14pD' '%-14pD'
> 4. kasnprintf

kasprintf()

After commenting on patch 1, I suggest you to always build with `make W=1 ...`

> Changelog
> =========
> v4:
> - don't support spec.precision anymore for '%pD'
> - add Rasmus's patch into this series
>
> v3:
> - implement new d_path_unsafe to use [buf, end] instead of stack space for
>   filling bytes (by Matthew)
> - add new test cases for '%pD'
> - drop patch "hmcdrv: remove the redundant directory path" before removing rfc.
>
> v2:
> - implement new d_path_fast based on Al Viro's patches
> - add check_pointer check (by Petr)
> - change the max full path size to 256 in stack space
> v1: https://lkml.org/lkml/2021/5/8/122
>
> Jia He (4):
>   fs: introduce helper d_path_unsafe()
>   lib/vsprintf.c: make '%pD' print full path for file
>   lib/test_printf.c: split write-beyond-buffer check in two
>   lib/test_printf.c: add test cases for '%pD'
>
>  Documentation/core-api/printk-formats.rst |  5 +-
>  fs/d_path.c                               | 83 ++++++++++++++++++++++-
>  include/linux/dcache.h                    |  1 +
>  lib/test_printf.c                         | 31 ++++++++-
>  lib/vsprintf.c                            | 37 ++++++++--
>  5 files changed, 148 insertions(+), 9 deletions(-)
>
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFCv4 2/4] lib/vsprintf.c: make '%pD' print full path for file

[Andy Shevchenko]

On Tue, Jun 15, 2021 at 6:54 PM Jia He <justin.he@arm.com> wrote:
>
> Previously, the specifier '%pD' is for printing dentry name of struct
> file. 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.

print the full

>
> Precision is never going to be used with %p (or any of its kernel
> extensions) if -Wformat is turned on.
> .
>
> [1] https://lore.kernel.org/lkml/CAHk-=wimsMqGdzik187YWLb-ru+iktb4MYbMQG1rnZ81dXYFVg@mail.gmail.com/

Put it as a Link: tag?

>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Jia He <justin.he@arm.com>
> ---
>  Documentation/core-api/printk-formats.rst |  5 +--
>  lib/vsprintf.c                            | 37 ++++++++++++++++++++---
>  2 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> 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
>  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..9d3166332726 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>

I know that this is an arbitrary order, but can you keep it after ctype.h?

>  #include <linux/kallsyms.h>
>  #include <linux/math64.h>
>  #include <linux/uaccess.h>
> @@ -920,13 +921,41 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
>  }
>
>  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);
> +
> +       /* Calculate the full d_path length, ignoring the tail '\0' */
> +       dpath_len = end - buf - prepend_len - 1;
> +
> +       reserved_size = max_t(int, dpath_len, spec.field_width);
> +
> +       /* case 1: no space at all, forward the buf with reserved size */

Case 1:

> +       if (buf >= end)
> +               return buf + reserved_size;
> +
> +       /*
> +        * case 2: small scratch space for long d_path name. The space

Case 2:

> +        * [buf,end] has been filled with truncated string. Hence use the
> +        * full dpath_len for further string widening.
> +        */
> +       if (prepend_len < 0)
> +               return widen_string(buf + dpath_len, dpath_len, end, spec);
> +
> +       /* case3: space is big enough */

Case 3:

> +       return string_nocheck(buf, end, p, spec);
>  }
>  #ifdef CONFIG_BLOCK
>  static noinline_for_stack
> @@ -2296,7 +2325,7 @@ early_param("no_hash_pointers", no_hash_pointers_enable);
>   * - 'a[pd]' For address types [p] phys_addr_t, [d] dma_addr_t and derivatives
>   *           (default assumed to be phys_addr_t, passed by reference)
>   * - 'd[234]' For a dentry name (optionally 2-4 last components)
> - * - 'D[234]' Same as 'd' but for a struct file
> + * - 'D' For full path name of a struct file
>   * - 'g' For block_device name (gendisk + partition number)
>   * - 't[RT][dt][r]' For time and date as represented by:
>   *      R    struct rtc_time
> @@ -2395,7 +2424,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>         case 'C':
>                 return clock(buf, end, ptr, spec, fmt);
>         case 'D':
> -               return file_dentry_name(buf, end, ptr, spec, fmt);
> +               return file_d_path_name(buf, end, ptr, spec, fmt);
>  #ifdef CONFIG_BLOCK
>         case 'g':
>                 return bdev_name(buf, end, ptr, spec, fmt);
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFCv4 4/4] lib/test_printf.c: add test cases for '%pD'

[Andy Shevchenko]

On Tue, Jun 15, 2021 at 6:55 PM Jia He <justin.he@arm.com> wrote:
>
> After the behaviour of specifier '%pD' is changed to print full path
> of struct file, the related test cases are also updated.
>
> Given the string of '%pD' is prepended from the end of the buffer, the
> check of "wrote beyond the nul-terminator" should be skipped.
>
> Signed-off-by: Jia He <justin.he@arm.com>
> ---
>  lib/test_printf.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index d1d2f898ebae..9f851a82b3af 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -16,6 +16,7 @@
>
>  #include <linux/bitmap.h>
>  #include <linux/dcache.h>
> +#include <linux/fs.h>
>  #include <linux/socket.h>
>  #include <linux/in.h>
>
> @@ -34,6 +35,7 @@ KSTM_MODULE_GLOBALS();
>
>  static char *test_buffer __initdata;
>  static char *alloced_buffer __initdata;
> +static bool is_prepended_buf __initdata;
>
>  extern bool no_hash_pointers;
>
> @@ -78,7 +80,7 @@ do_test(int bufsize, const char *expect, int elen,
>                 return 1;
>         }
>
> -       if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) {

> +       if (!is_prepended_buf && memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) {

Can it be parametrized? I don't like the custom test case being
involved here like this.

>                 pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n",
>                         bufsize, fmt);
>                 return 1;
> @@ -501,6 +503,27 @@ dentry(void)
>         test("  bravo/alfa|  bravo/alfa", "%12pd2|%*pd2", &test_dentry[2], 12, &test_dentry[2]);
>  }
>
> +static struct vfsmount test_vfsmnt __initdata = {};
> +
> +static struct file test_file __initdata = {
> +       .f_path = { .dentry = &test_dentry[2],
> +                   .mnt = &test_vfsmnt,
> +       },
> +};
> +
> +static void __init
> +f_d_path(void)
> +{
> +       test("(null)", "%pD", NULL);
> +       test("(efault)", "%pD", PTR_INVALID);
> +
> +       is_prepended_buf = true;
> +       test("/bravo/alfa   |/bravo/alfa   ", "%-14pD|%*pD", &test_file, -14, &test_file);
> +       test("   /bravo/alfa|   /bravo/alfa", "%14pD|%*pD", &test_file, 14, &test_file);
> +       test("   /bravo/alfa|/bravo/alfa   ", "%14pD|%-14pD", &test_file, &test_file);
> +       is_prepended_buf = false;
> +}
> +
>  static void __init
>  struct_va_format(void)
>  {
> @@ -784,6 +807,7 @@ test_pointer(void)
>         ip();
>         uuid();
>         dentry();
> +       f_d_path();
>         struct_va_format();
>         time_and_date();
>         struct_clk();
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH RFCv4 1/4] fs: introduce helper d_path_unsafe()

[Justin He]

Hi Andy
Thanks for the comments, I will resolve you mentioned typo/grammar issues

Some answer below

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Wednesday, June 16, 2021 4:41 AM
> To: Justin He <Justin.He@arm.com>
> Cc: Petr Mladek <pmladek@suse.com>; 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>; Peter Zijlstra (Intel) <peterz@infradead.org>; Eric
> Biggers <ebiggers@google.com>; Ahmed S. Darwish <a.darwish@linutronix.de>;
> Linux Documentation List <linux-doc@vger.kernel.org>; Linux Kernel Mailing
> List <linux-kernel@vger.kernel.org>; Linux FS Devel <linux-
> fsdevel@vger.kernel.org>; Matthew Wilcox <willy@infradead.org>
> Subject: Re: [PATCH RFCv4 1/4] fs: introduce helper d_path_unsafe()
> 
> On Tue, Jun 15, 2021 at 6:56 PM Jia He <justin.he@arm.com> wrote:
> >
> > This helper is similar to d_path except that it doesn't take any
> > seqlock/spinlock. It is typical for debugging purpose. Besides,
> 
> purposes
> 
> > an additional return value *prenpend_len* is used to get the full
> > path length of the dentry.
> >
> > prepend_name_with_len() enhances the behavior of prepend_name().
> > Previously it will skip the loop at once in __prepen_path() when the
> > space is not enough. __prepend_path() gets the full length of dentry
> > together with the parent recusively.
> 
> recursively
> 
> >
> > Besides, if someone invokes snprintf with small but positive space,
> > prepend_name_with() needs to move and copy the string partially.
> >
> > More than that, kasnprintf will pass NULL _buf_ and _end_, hence
> 
> kasprintf()
> 
> > it returns at the very beginning with false in this case;
> >
> > Suggested-by: Matthew Wilcox <willy@infradead.org>
> > Signed-off-by: Jia He <justin.he@arm.com>
> > ---
> >  fs/d_path.c            | 83 +++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/dcache.h |  1 +
> >  2 files changed, 82 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/d_path.c b/fs/d_path.c
> > index 23a53f7b5c71..4fc224eadf58 100644
> > --- a/fs/d_path.c
> > +++ b/fs/d_path.c
> > @@ -68,9 +68,66 @@ static bool prepend_name(struct prepend_buffer *p,
> const struct qstr *name)
> >         return true;
> >  }
> >
> > +static bool prepend_name_with_len(struct prepend_buffer *p, const struct
> qstr *name,
> > +                        int orig_buflen)
> > +{
> > +       const char *dname = smp_load_acquire(&name->name); /* ^^^ */
> 
> What does this funny comment mean?

It is inherited from the prepend_name()

> 
> > +       int dlen = READ_ONCE(name->len);
> > +       char *s;
> > +       int last_len = p->len;
> > +
> > +       p->len -= dlen + 1;
> > +
> > +       if (unlikely(!p->buf))
> > +               return false;
> > +
> > +       if (orig_buflen <= 0)
> > +               return false;
> > +       /*
> > +        * The first time we overflow the buffer. Then fill the string
> > +        * partially from the beginning
> > +        */
> > +       if (unlikely(p->len < 0)) {
> > +               int buflen = strlen(p->buf);
> > +
> > +               s = p->buf;
> > +
> > +               /* Still have small space to fill partially */
> > +               if (last_len > 0) {
> > +                       p->buf -= last_len;
> > +                       buflen += last_len;
> > +               }
> > +
> > +               if (buflen > dlen + 1) {
> > +                       /* This dentry name can be fully filled */
> > +                       memmove(p->buf + dlen + 1, s, buflen - dlen - 1);
> > +                       p->buf[0] = '/';
> > +                       memcpy(p->buf + 1, dname, dlen);
> > +               } else if (buflen > 0) {
> > +                       /* Partially filled, and drop last dentry name */
> > +                       p->buf[0] = '/';
> > +                       memcpy(p->buf + 1, dname, buflen - 1);
> > +               }
> > +
> > +               return false;
> > +       }
> > +
> > +       s = p->buf -= dlen + 1;
> > +       *s++ = '/';
> 
> > +       while (dlen--) {
> > +               char c = *dname++;
> > +
> > +               if (!c)
> > +                       break;
> > +               *s++ = c;
> 
> I'm wondering why can't memcpy() be used here as well.


From the comments of commit 7a5cf791:

>However, there may be mismatch between length and pointer.
> * The length cannot be trusted, we need to copy it byte-by-byte until
> * the length is reached or a null byte is found.

Seems we shouldn't use memcpy/strcpy here

> 
> > +       }
> > +       return true;
> > +}
> >  static int __prepend_path(const struct dentry *dentry, const struct
> mount *mnt,
> >                           const struct path *root, struct prepend_buffer
> *p)
> >  {
> > +       int orig_buflen = p->len;
> > +
> >         while (dentry != root->dentry || &mnt->mnt != root->mnt) {
> >                 const struct dentry *parent = READ_ONCE(dentry->d_parent);
> >
> > @@ -97,8 +154,7 @@ static int __prepend_path(const struct dentry *dentry,
> const struct mount *mnt,
> >                         return 3;
> >
> >                 prefetch(parent);
> > -               if (!prepend_name(p, &dentry->d_name))
> > -                       break;
> > +               prepend_name_with_len(p, &dentry->d_name, orig_buflen);
> >                 dentry = parent;
> >         }
> >         return 0;
> > @@ -263,6 +319,29 @@ char *d_path(const struct path *path, char *buf, int
> buflen)
> >  }
> >  EXPORT_SYMBOL(d_path);
> >
> > +/**
> > + * d_path_unsafe - fast return the full path of a dentry without taking
> > + * any seqlock/spinlock. This helper is typical for debugging purpose.
> 
> purposes
> 
> Haven't you got kernel doc validation warnings? Please, describe
> parameters as well.

I will check it and update if there is a warning, thanks for the reminder.


--
Cheers,
Justin (Jia He)

Re: [PATCH RFCv4 0/4] make '%pD' print full path for file

[Christoph Hellwig]

Btw, please throw in a patch to convert iomap_swapfile_fail over to
the new %pD as that started the whole flamewar^H^H^H^H^H^H^Hdiscussion.

RE: [PATCH RFCv4 0/4] make '%pD' print full path for file

[Justin He]

Hi Christoph

> -----Original Message-----
> From: Christoph Hellwig <hch@infradead.org>
> Sent: Wednesday, June 16, 2021 1:09 PM
> To: Justin He <Justin.He@arm.com>
> Cc: Petr Mladek <pmladek@suse.com>; 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>; Peter Zijlstra (Intel) <peterz@infradead.org>; Eric
> Biggers <ebiggers@google.com>; Ahmed S. Darwish <a.darwish@linutronix.de>;
> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> fsdevel@vger.kernel.org; Matthew Wilcox <willy@infradead.org>
> Subject: Re: [PATCH RFCv4 0/4] make '%pD' print full path for file
> 
> Btw, please throw in a patch to convert iomap_swapfile_fail over to
> the new %pD as that started the whole flamewar^H^H^H^H^H^H^Hdiscussion.

Ok, thanks for the reminder.

After all of the solution/helper/interface is stable (maybe after removing RFC),
I tend to add more patches:
1. s390/hmcdrv: remove the redundant directory after using '%pD'
2. remove all the '%pD[2,3,4]' usage in fs/
3. your mentioned iomap_swapfile_fail


--
Cheers,
Justin (Jia He)

Re: [PATCH RFCv4 2/4] lib/vsprintf.c: make '%pD' print full path for file

[Petr Mladek]

On Tue 2021-06-15 23:49:50, Jia He wrote:
> Previously, the specifier '%pD' is for printing dentry name of struct
> file. 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.
> 
> Precision is never going to be used with %p (or any of its kernel
> extensions) if -Wformat is turned on.
> .
> 
> [1] https://lore.kernel.org/lkml/CAHk-=wimsMqGdzik187YWLb-ru+iktb4MYbMQG1rnZ81dXYFVg@mail.gmail.com/
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Jia He <justin.he@arm.com>

> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -920,13 +921,41 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
>  }
>  
>  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);
> +
> +	/* Calculate the full d_path length, ignoring the tail '\0' */
> +	dpath_len = end - buf - prepend_len - 1;
> +
> +	reserved_size = max_t(int, dpath_len, spec.field_width);

"reserved_size" is kind of confusing. "dpath_widen_len" or just "widen_len"
look much more obvious.

The below comments are not bad. But they still made me thing about it
more than I wanted ;-) I wonder if it following is better:

> +	/* case 1: no space at all, forward the buf with reserved size */
> +	if (buf >= end)
> +		return buf + reserved_size;

	/* Case 1: Already started past the buffer. Just forward @buf. */
	if (buf >= end)
		return buf + widen_len;

> +
> +	/*
> +	 * case 2: small scratch space for long d_path name. The space
> +	 * [buf,end] has been filled with truncated string. Hence use the
> +	 * full dpath_len for further string widening.
> +	 */
> +	if (prepend_len < 0)
> +		return widen_string(buf + dpath_len, dpath_len, end, spec);

	/*
	 * Case 2: The entire remaining space of the buffer filled by
	 * the truncated path. Still need to get moved right when
	 * the filed width is greather than the full path length.
	 */
	if (prepend_len < 0)
		return widen_string(buf + dpath_len, dpath_len, end, spec);

> +	/* case3: space is big enough */
> +	return string_nocheck(buf, end, p, spec);

	/*
	 * Case 3: The full path is printed at the end of the buffer.
	 * Print it at the right location in the same buffer.
	 */
	return string_nocheck(buf, end, p, spec);
>  }
>  #ifdef CONFIG_BLOCK
>  static noinline_for_stack

In each case, I am happy that it was possible to simplify the logic.
I got lost several times in the previous version.

Best Regards,
Petr

Re: [PATCH RFCv4 3/4] lib/test_printf.c: split write-beyond-buffer check in two

[Petr Mladek]

On Tue 2021-06-15 23:49:51, Jia He wrote:
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> 
> Before each invocation of vsnprintf(), do_test() memsets the entire
> allocated buffer to a sentinel value. That buffer includes leading and
> trailing padding which is never included in the buffer area handed to
> vsnprintf (spaces merely for clarity):
> 
>   pad  test_buffer      pad
>   **** **************** ****
> 
> Then vsnprintf() is invoked with a bufsize argument <=
> BUF_SIZE. Suppose bufsize=10, then we'd have e.g.
> 
>  |pad |   test_buffer    |pad |
>   **** pizza0 **** ****** ****
>  A    B      C    D           E
> 
> where vsnprintf() was given the area from B to D.
> 
> It is obviously a bug for vsnprintf to touch anything between A and B
> or between D and E. The former is checked for as one would expect. But
> for the latter, we are actually a little stricter in that we check the
> area between C and E.
> 
> Split that check in two, providing a clearer error message in case it
> was a genuine buffer overrun and not merely a write within the
> provided buffer, but after the end of the generated string.
> 
> So far, no part of the vsnprintf() implementation has had any use for
> using the whole buffer as scratch space, but it's not unreasonable to
> allow that, as long as the result is properly nul-terminated and the
> return value is the right one. However, it is somewhat unusual, and
> most %<something> won't need this, so keep the [C,D] check, but make
> it easy for a later patch to make that part opt-out for certain tests.

Excellent commit message.

> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Tested-by: Jia He <justin.he@arm.com>
> Signed-off-by: Jia He <justin.he@arm.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH RFCv4 4/4] lib/test_printf.c: add test cases for '%pD'

[Petr Mladek]

On Tue 2021-06-15 23:47:29, Andy Shevchenko wrote:
> On Tue, Jun 15, 2021 at 6:55 PM Jia He <justin.he@arm.com> wrote:
> >
> > After the behaviour of specifier '%pD' is changed to print full path
> > of struct file, the related test cases are also updated.
> >
> > Given the string of '%pD' is prepended from the end of the buffer, the
> > check of "wrote beyond the nul-terminator" should be skipped.
> >
> > Signed-off-by: Jia He <justin.he@arm.com>
> > ---
> >  lib/test_printf.c | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/test_printf.c b/lib/test_printf.c
> > index d1d2f898ebae..9f851a82b3af 100644
> > --- a/lib/test_printf.c
> > +++ b/lib/test_printf.c
> > @@ -78,7 +80,7 @@ do_test(int bufsize, const char *expect, int elen,
> >                 return 1;
> >         }
> >
> > -       if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) {
> 
> > +       if (!is_prepended_buf && memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) {
> 
> Can it be parametrized? I don't like the custom test case being
> involved here like this.

Yup, it would be nice.

Also it is far from obvious what @is_prepended_buf means if you do not
have context of this patchset. I think about a more generic name
that comes from the wording used in 3rd patch, e.g.

    @need_scratch_space or @using_scratch_space or @dirty_buf

Anyway, the most easy way to pass this as a parameter would be to add it
to __test() and define a wrapper, .e.g:

static void __printf(3, 4) __init
__test(const char *expect, int elen, bool using_scratch_space,
	const char *fmt, ...)

/*
 * More relaxed test for non-standard formats that are using the provided buffer
 * as a scratch space and write beyond the trailing '\0'.
 */
#define test_using_scratch_space(expect, fmt, ...)			\
	__test(expect, strlen(expect), true, fmt, ##__VA_ARGS__)


Best Regards,
Petr

RE: [PATCH RFCv4 2/4] lib/vsprintf.c: make '%pD' print full path for file

[Justin He]

Hi Petr

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Thursday, June 17, 2021 10:10 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>; Peter Zijlstra (Intel) <peterz@infradead.org>; Eric
> Biggers <ebiggers@google.com>; Ahmed S. Darwish <a.darwish@linutronix.de>;
> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> fsdevel@vger.kernel.org; Matthew Wilcox <willy@infradead.org>
> Subject: Re: [PATCH RFCv4 2/4] lib/vsprintf.c: make '%pD' print full path
> for file
>
> On Tue 2021-06-15 23:49:50, Jia He wrote:
> > Previously, the specifier '%pD' is for printing dentry name of struct
> > file. 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.
> >
> > Precision is never going to be used with %p (or any of its kernel
> > extensions) if -Wformat is turned on.
> > .
> >
> > [1] https://lore.kernel.org/lkml/CAHk-=wimsMqGdzik187YWLb-
> ru+iktb4MYbMQG1rnZ81dXYFVg@mail.gmail.com/
> >
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Jia He <justin.he@arm.com>
>
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -920,13 +921,41 @@ char *dentry_name(char *buf, char *end, const
> struct dentry *d, struct printf_sp
> >  }
> >
> >  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);
> > +
> > +   /* Calculate the full d_path length, ignoring the tail '\0' */
> > +   dpath_len = end - buf - prepend_len - 1;
> > +
> > +   reserved_size = max_t(int, dpath_len, spec.field_width);
>
> "reserved_size" is kind of confusing. "dpath_widen_len" or just "widen_len"
> look much more obvious.

Okay

>
> The below comments are not bad. But they still made me thing about it
> more than I wanted ;-) I wonder if it following is better:
>
> > +   /* case 1: no space at all, forward the buf with reserved size */
> > +   if (buf >= end)
> > +           return buf + reserved_size;
>
>       /* Case 1: Already started past the buffer. Just forward @buf. */
>       if (buf >= end)
>               return buf + widen_len;
>
Okay
> > +
> > +   /*
> > +    * case 2: small scratch space for long d_path name. The space
> > +    * [buf,end] has been filled with truncated string. Hence use the
> > +    * full dpath_len for further string widening.
> > +    */
> > +   if (prepend_len < 0)
> > +           return widen_string(buf + dpath_len, dpath_len, end, spec);
>
>       /*
>        * Case 2: The entire remaining space of the buffer filled by
>        * the truncated path. Still need to get moved right when
>        * the filed width is greather than the full path length.
>        */
>       if (prepend_len < 0)
>               return widen_string(buf + dpath_len, dpath_len, end, spec);
>
Okay
> > +   /* case3: space is big enough */
> > +   return string_nocheck(buf, end, p, spec);
>
>       /*
>        * Case 3: The full path is printed at the end of the buffer.
>        * Print it at the right location in the same buffer.
>        */
>       return string_nocheck(buf, end, p, spec);
Okay
> >  }
> >  #ifdef CONFIG_BLOCK
> >  static noinline_for_stack
>
> In each case, I am happy that it was possible to simplify the logic.
> I got lost several times in the previous version.

Indeed, the cases can be much simpler if we don't consider spec.precision.
More than that, maybe we could remove the spec.precision consideration for
'%pd' or other pointer related specifiers also


--
Cheers,
Justin (Jia He)


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.

RE: [PATCH RFCv4 4/4] lib/test_printf.c: add test cases for '%pD'

[Justin He]

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Thursday, June 17, 2021 10:53 PM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Justin He <Justin.He@arm.com>; 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>; Peter Zijlstra (Intel) <peterz@infradead.org>; Eric
> Biggers <ebiggers@google.com>; Ahmed S. Darwish <a.darwish@linutronix.de>;
> Linux Documentation List <linux-doc@vger.kernel.org>; Linux Kernel Mailing
> List <linux-kernel@vger.kernel.org>; Linux FS Devel <linux-
> fsdevel@vger.kernel.org>; Matthew Wilcox <willy@infradead.org>
> Subject: Re: [PATCH RFCv4 4/4] lib/test_printf.c: add test cases for '%pD'
>
> On Tue 2021-06-15 23:47:29, Andy Shevchenko wrote:
> > On Tue, Jun 15, 2021 at 6:55 PM Jia He <justin.he@arm.com> wrote:
> > >
> > > After the behaviour of specifier '%pD' is changed to print full path
> > > of struct file, the related test cases are also updated.
> > >
> > > Given the string of '%pD' is prepended from the end of the buffer, the
> > > check of "wrote beyond the nul-terminator" should be skipped.
> > >
> > > Signed-off-by: Jia He <justin.he@arm.com>
> > > ---
> > >  lib/test_printf.c | 26 +++++++++++++++++++++++++-
> > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/test_printf.c b/lib/test_printf.c
> > > index d1d2f898ebae..9f851a82b3af 100644
> > > --- a/lib/test_printf.c
> > > +++ b/lib/test_printf.c
> > > @@ -78,7 +80,7 @@ do_test(int bufsize, const char *expect, int elen,
> > >                 return 1;
> > >         }
> > >
> > > -       if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize -
> (written + 1))) {
> >
> > > +       if (!is_prepended_buf && memchr_inv(test_buffer + written + 1,
> FILL_CHAR, bufsize - (written + 1))) {
> >
> > Can it be parametrized? I don't like the custom test case being
> > involved here like this.
>
> Yup, it would be nice.
>
> Also it is far from obvious what @is_prepended_buf means if you do not
> have context of this patchset. I think about a more generic name
> that comes from the wording used in 3rd patch, e.g.
>
>     @need_scratch_space or @using_scratch_space or @dirty_buf
>
> Anyway, the most easy way to pass this as a parameter would be to add it
> to __test() and define a wrapper, .e.g:
>
> static void __printf(3, 4) __init
> __test(const char *expect, int elen, bool using_scratch_space,
>       const char *fmt, ...)
>
> /*
>  * More relaxed test for non-standard formats that are using the provided
> buffer
>  * as a scratch space and write beyond the trailing '\0'.
>  */
> #define test_using_scratch_space(expect, fmt, ...)                    \
>       __test(expect, strlen(expect), true, fmt, ##__VA_ARGS__)
Okay, thanks for the suggestion.


--
Cheers,
Justin (Jia He)


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.

Re: [PATCH RFCv4 3/4] lib/test_printf.c: split write-beyond-buffer check in two

[Rasmus Villemoes]

On 17/06/2021 16.17, Petr Mladek wrote:
> On Tue 2021-06-15 23:49:51, Jia He wrote:
>> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>>
>> Before each invocation of vsnprintf(), do_test() memsets the entire
>> allocated buffer to a sentinel value. That buffer includes leading and
>> trailing padding which is never included in the buffer area handed to
>> vsnprintf (spaces merely for clarity):
>>
>>   pad  test_buffer      pad
>>   **** **************** ****
>>
>> Then vsnprintf() is invoked with a bufsize argument <=
>> BUF_SIZE. Suppose bufsize=10, then we'd have e.g.
>>
>>  |pad |   test_buffer    |pad |
>>   **** pizza0 **** ****** ****
>>  A    B      C    D           E
>>
>> where vsnprintf() was given the area from B to D.
>>
>> It is obviously a bug for vsnprintf to touch anything between A and B
>> or between D and E. The former is checked for as one would expect. But
>> for the latter, we are actually a little stricter in that we check the
>> area between C and E.
>>
>> Split that check in two, providing a clearer error message in case it
>> was a genuine buffer overrun and not merely a write within the
>> provided buffer, but after the end of the generated string.
>>
>> So far, no part of the vsnprintf() implementation has had any use for
>> using the whole buffer as scratch space, but it's not unreasonable to
>> allow that, as long as the result is properly nul-terminated and the
>> return value is the right one. However, it is somewhat unusual, and
>> most %<something> won't need this, so keep the [C,D] check, but make
>> it easy for a later patch to make that part opt-out for certain tests.
> 
> Excellent commit message.
> 
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> Tested-by: Jia He <justin.he@arm.com>
>> Signed-off-by: Jia He <justin.he@arm.com>
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Hi Petr

It seems Justin's series got stalled, but I still think this patch makes
sense on its own (especially since another series in flight mucks about
in this area), so can you please pick it up directly?

The lore link for the above is
https://lore.kernel.org/lkml/20210615154952.2744-4-justin.he@arm.com/ ,
while my original submission is at
https://lore.kernel.org/lkml/20210615085044.1923788-1-linux@rasmusvillemoes.dk/
.

Rasmus

Re: [PATCH RFCv4 3/4] lib/test_printf.c: split write-beyond-buffer check in two

[Kent Overstreet]

On 6/17/22 03:15, Rasmus Villemoes wrote:
> On 17/06/2021 16.17, Petr Mladek wrote:
>> On Tue 2021-06-15 23:49:51, Jia He wrote:
>>> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>>>
>>> Before each invocation of vsnprintf(), do_test() memsets the entire
>>> allocated buffer to a sentinel value. That buffer includes leading and
>>> trailing padding which is never included in the buffer area handed to
>>> vsnprintf (spaces merely for clarity):
>>>
>>>    pad  test_buffer      pad
>>>    **** **************** ****
>>>
>>> Then vsnprintf() is invoked with a bufsize argument <=
>>> BUF_SIZE. Suppose bufsize=10, then we'd have e.g.
>>>
>>>   |pad |   test_buffer    |pad |
>>>    **** pizza0 **** ****** ****
>>>   A    B      C    D           E
>>>
>>> where vsnprintf() was given the area from B to D.
>>>
>>> It is obviously a bug for vsnprintf to touch anything between A and B
>>> or between D and E. The former is checked for as one would expect. But
>>> for the latter, we are actually a little stricter in that we check the
>>> area between C and E.
>>>
>>> Split that check in two, providing a clearer error message in case it
>>> was a genuine buffer overrun and not merely a write within the
>>> provided buffer, but after the end of the generated string.
>>>
>>> So far, no part of the vsnprintf() implementation has had any use for
>>> using the whole buffer as scratch space, but it's not unreasonable to
>>> allow that, as long as the result is properly nul-terminated and the
>>> return value is the right one. However, it is somewhat unusual, and
>>> most %<something> won't need this, so keep the [C,D] check, but make
>>> it easy for a later patch to make that part opt-out for certain tests.
>>
>> Excellent commit message.
>>
>>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>>> Tested-by: Jia He <justin.he@arm.com>
>>> Signed-off-by: Jia He <justin.he@arm.com>
>>
>> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> Hi Petr
> 
> It seems Justin's series got stalled, but I still think this patch makes
> sense on its own (especially since another series in flight mucks about
> in this area), so can you please pick it up directly?
> 
> The lore link for the above is
> https://lore.kernel.org/lkml/20210615154952.2744-4-justin.he@arm.com/ ,
> while my original submission is at
> https://lore.kernel.org/lkml/20210615085044.1923788-1-linux@rasmusvillemoes.dk/

That patch definitely clarifies things, feel free to add my reviewed-by tag

Re: [PATCH RFCv4 3/4] lib/test_printf.c: split write-beyond-buffer check in two

[Petr Mladek]

On Fri 2022-06-17 09:15:53, Rasmus Villemoes wrote:
> On 17/06/2021 16.17, Petr Mladek wrote:
> > On Tue 2021-06-15 23:49:51, Jia He wrote:
> >> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >>
> >> Before each invocation of vsnprintf(), do_test() memsets the entire
> >> allocated buffer to a sentinel value. That buffer includes leading and
> >> trailing padding which is never included in the buffer area handed to
> >> vsnprintf (spaces merely for clarity):
> >>
> >>   pad  test_buffer      pad
> >>   **** **************** ****
> >>
> >> Then vsnprintf() is invoked with a bufsize argument <=
> >> BUF_SIZE. Suppose bufsize=10, then we'd have e.g.
> >>
> >>  |pad |   test_buffer    |pad |
> >>   **** pizza0 **** ****** ****
> >>  A    B      C    D           E
> >>
> >> where vsnprintf() was given the area from B to D.
> >>
> >> It is obviously a bug for vsnprintf to touch anything between A and B
> >> or between D and E. The former is checked for as one would expect. But
> >> for the latter, we are actually a little stricter in that we check the
> >> area between C and E.
> >>
> >> Split that check in two, providing a clearer error message in case it
> >> was a genuine buffer overrun and not merely a write within the
> >> provided buffer, but after the end of the generated string.
> >>
> >> So far, no part of the vsnprintf() implementation has had any use for
> >> using the whole buffer as scratch space, but it's not unreasonable to
> >> allow that, as long as the result is properly nul-terminated and the
> >> return value is the right one. However, it is somewhat unusual, and
> >> most %<something> won't need this, so keep the [C,D] check, but make
> >> it easy for a later patch to make that part opt-out for certain tests.
> > 
> > Excellent commit message.
> > 
> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >> Tested-by: Jia He <justin.he@arm.com>
> >> Signed-off-by: Jia He <justin.he@arm.com>
> > 
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> Hi Petr
> 
> It seems Justin's series got stalled, but I still think this patch makes
> sense on its own (especially since another series in flight mucks about
> in this area), so can you please pick it up directly?

I have just committed this patch into printk/linux.git, branch for-5.20.

I am sorry that it took so long. Too many things have happened during
last weeks.

Best Regards,
Petr