[PATCH RFCv3 0/3] make '%pD' print full path for file

[PATCH RFCv3 0/3] 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'
5. all test_print selftests, including the new '%14pD' '%-14pD'
4. kasnprintf
   
After this set, I found many lines containing '%pD[234]' should be changed
to '%pD'. I don't want to involve those subsystems in this patch series
before the helper is stable enough.
   
You can get the lines by:
$find fs/ -name \*.[ch] | xargs grep -rn "\%pD[234"

fs/overlayfs/file.c:65: pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
fs/nfs/direct.c:453:    dfprintk(FILE, "NFS: direct read(%pD2, %zd@%Ld)\n",
fs/nfs/direct.c:908:    dfprintk(FILE, "NFS: direct write(%pD2, %zd@%Ld)\n",
fs/nfs/write.c:1371:    dprintk("NFS:       nfs_updatepage(%pD2 %d@%lld)\n",
fs/nfs/nfs4file.c:116:  dprintk("NFS: flush(%pD2)\n", file);
fs/nfs/file.c:69:       dprintk("NFS: open file(%pD2)\n", filp);
fs/nfs/file.c:83:       dprintk("NFS: release(%pD2)\n", filp);
fs/nfs/file.c:117:      dprintk("NFS: llseek file(%pD2, %lld, %d)\n",
fs/nfs/file.c:145:      dprintk("NFS: flush(%pD2)\n", file);
fs/nfs/file.c:166:      dprintk("NFS: read(%pD2, %zu@%lu)\n",
fs/nfs/file.c:188:      dprintk("NFS: mmap(%pD2)\n", file);
fs/nfs/file.c:213:      dprintk("NFS: fsync file(%pD2) datasync %d\n", file, datasync);
fs/nfs/file.c:328:      dfprintk(PAGECACHE, "NFS: write_begin(%pD2(%lu), %u@%lld)\n",
fs/nfs/file.c:360:      dfprintk(PAGECACHE, "NFS: write_end(%pD2(%lu), %u@%lld)\n",
fs/nfs/file.c:551:      dfprintk(PAGECACHE, "NFS: vm_page_mkwrite(%pD2(%lu), offset %lld)\n",
fs/nfs/file.c:621:      dprintk("NFS: write(%pD2, %zu@%Ld)\n",
fs/nfs/file.c:803:      dprintk("NFS: lock(%pD2, t=%x, fl=%x, r=%lld:%lld)\n",
fs/nfs/file.c:841:      dprintk("NFS: flock(%pD2, t=%x, fl=%x)\n",
fs/nfs/dir.c:111:       dfprintk(FILE, "NFS: open dir(%pD2)\n", filp);
fs/nfs/dir.c:456:                                               pr_notice("NFS: directory %pD2 contains a readdir loop."
fs/nfs/dir.c:1084:      dfprintk(FILE, "NFS: readdir(%pD2) starting at cookie %llu\n",
fs/nfs/dir.c:1158:      dfprintk(FILE, "NFS: readdir(%pD2) returns %d\n", file, res);
fs/nfs/dir.c:1166:      dfprintk(FILE, "NFS: llseek dir(%pD2, %lld, %d)\n",
fs/nfs/dir.c:1208:      dfprintk(FILE, "NFS: fsync dir(%pD2) datasync %d\n", filp, datasync);
fs/nfsd/nfs4state.c:2439:         seq_printf(s, "filename: \"%pD2\"", f->nf_file);
fs/exec.c:817:          pr_warn_once("process '%pD4' started with executable stack\n",
fs/iomap/direct-io.c:429:               pr_warn_ratelimited("Direct I/O collision with buffered writes! File: %pD4 Comm: %.20s\n",
fs/ioctl.c:81:          pr_warn_ratelimited("[%s/%d] FS: %s File: %pD4 would truncate fibmap result\n",
fs/read_write.c:425:            "kernel %s not supported for file %pD4 (pid: %d comm: %.20s)\n",
fs/splice.c:754:                "splice %s not supported for file %pD4 (pid: %d comm: %.20s)\n",
fs/afs/mntpt.c:64:      _enter("%p,%p{%pD2}", inode, file, file);

Changelog:
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 (3):
  fs: introduce helper d_path_unsafe()
  lib/vsprintf.c: make %pD print full path for file
  lib/test_printf: add test cases for '%pD'

 Documentation/core-api/printk-formats.rst |  5 +-
 fs/d_path.c                               | 82 ++++++++++++++++++++++-
 include/linux/dcache.h                    |  1 +
 lib/test_printf.c                         | 26 ++++++-
 lib/vsprintf.c                            | 47 +++++++++++--
 5 files changed, 152 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH RFCv3 1/3] 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 RFCv3 2/3] lib/vsprintf.c: make %pD print full path for file

[Jia He]

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/

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                            | 47 +++++++++++++++++++++--
 2 files changed, 46 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..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)
 }
 
 /* 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) {
+		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;
+}
 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
 }
 
 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);
+
+	/* 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;
+
+	/* small space for long name */
+	if (buf < end && prepend_len < 0)
+		return string_truncate(buf, end, p, dpath_len, spec);
+
+	/* space is enough */
+	return string_nocheck(buf, end, p, spec);
 }
 #ifdef CONFIG_BLOCK
 static noinline_for_stack
@@ -2296,7 +2335,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 +2434,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 RFCv3 3/3] lib/test_printf: 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 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 ec0d5976bb69..3632bd6cf906 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_prepend_buf;
 
 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, BUF_SIZE + PAD_SIZE - (written + 1))) {
+	if (!is_prepend_buf && memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1))) {
 		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n",
 			bufsize, fmt);
 		return 1;
@@ -496,6 +498,27 @@ dentry(void)
 	test("  bravo/alfa|  bravo/alfa", "%12pd2|%*pd2", &test_dentry[2], 12, &test_dentry[2]);
 }
 
+static struct vfsmount test_vfsmnt = {};
+
+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_prepend_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_prepend_buf = false;
+}
+
 static void __init
 struct_va_format(void)
 {
@@ -779,6 +802,7 @@ test_pointer(void)
 	ip();
 	uuid();
 	dentry();
+	f_d_path();
 	struct_va_format();
 	time_and_date();
 	struct_clk();
-- 
2.17.1

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

[Rasmus Villemoes]

On 11/06/2021 17.59, 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 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/
> 
> 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                            | 47 +++++++++++++++++++++--
>  2 files changed, 46 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..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)
>  }
>  
>  /* 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,

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

> +}
>  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
>  }
>  
>  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 [*]

> +	/* 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.

> +
> +	/* 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 [*]:

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

Re: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'

[Rasmus Villemoes]

On 11/06/2021 17.59, Jia He 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 is prepended from the end of the buffer, the check
> of "wrote beyond the nul-terminator" should be skipped.

Sorry, that is far from enough justification.

I should probably have split the "wrote beyond nul-terminator" check in two:

One that checks whether any memory beyond the buffer given to
vsnprintf() was touched (including all the padding, but possibly more
for the cases where we pass a known-too-short buffer), symmetric to the
"wrote before buffer" check.

And then another that checks the area between the '\0' and the end of
the given buffer - I suppose that it's fair game for vsnprintf to use
all of that as scratch space, and for that it could be ok to add that
boolean knob.

Rasmus

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

[Petr Mladek]

On Fri 2021-06-11 23:59:52, 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 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/
> 
> 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                            | 47 +++++++++++++++++++++--
>  2 files changed, 46 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..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)
>  }
>  
>  /* Handle string from a well known address. */

This comment is for widen_string().

string_truncate() functionality is far from obvious. It would deserve
it's own description, including description of each parammeter.

Well, do we really need it? See below.

> +static char *string_truncate(char *buf, char *end, const char *s,
> +			     u32 full_len, struct printf_spec spec)
> +{
> +	int lim = 0;
> +
> +	if (buf < end) {
> +		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;
> +}
>  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
>  }
>  
>  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);
> +
> +	/* 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;
> +
> +	/* small space for long name */
> +	if (buf < end && prepend_len < 0)
> +		return string_truncate(buf, end, p, dpath_len, spec);

We need this only because we allowed to write the path behind
spec.field_width. Do I get it right?

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

It easy to get lost in all the computations, including the one
in string_truncate():

	dpath_len = end - buf - prepend_len - 1;
	reserved_size = max_t(int, dpath_len, spec.field_width);
and
	lim = strlen(s) - min_t(int, spec.precision, strlen(s));
	return widen_string(buf + full_len, full_len, end - lim, spec);

Please, add comments explaining the meaning of the variables a bit.
They should help to understand why it is done this way.


I tried another approach below. The main trick is that
max_len is limited by spec.field_width and spec.precision before calling
d_path_unsave():


	if (check_pointer(&buf, end, f, spec))
		return buf;

	path = &f->f_path;
	if (check_pointer(&buf, end, path, spec))
		return buf;

	max_len = end - buf;
	if (spec.field_width >= 0 && spec.field_width < max_len)
		max_len = spec.filed_width;
	if (spec.precision >= 0 && spec.precision < max_len)
		max_len = spec.precision;

	p = d_path_unsafe(path, buf, max_len, &prepend_len);

	/*
	 * The path has been printed from the end of the buffer.
	 * Process it like a normal string to handle "precission"
	 * and "width" effects. In the "worst" case, the string
	 * will stay as is.
	 */
	if (buf < end) {
		buf = string_nocheck(buf, end, p, spec);
		/* Return buf when output was limited or did fit in. */
		if (spec.field_width >= 0 || spec.precision >= 0 ||
		    prepend_len >= 0) {
			return buf;
		}
		/* Otherwise, add what was missing. Ignore tail '\0' */
		return buf - prepend_len - 1;
	}

	/*
	 * Nothing has been written to the buffer. Just count the length.
	 * I is fixed when field_with is defined. */
	if (spec.field_width >= 0)
		return buf + spec.field_width;

	/* Otherwise, use the length of the path. */
	dpath_len = max_len - prepend_len - 1;

	/* The path might still get limited by precision number. */
	if (spec.precision >= 0 && spec.precision < dpath_len)
		return buf + spec.precision;

	return buf + dpath_len;


Note that the above code is not even compile tested. There might be
off by one mistakes. Also, it is possible that I missed something.

Best Regards,
Petr

Re: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'

[Petr Mladek]

On Fri 2021-06-11 23:59:53, Jia He 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 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 ec0d5976bb69..3632bd6cf906 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, BUF_SIZE + PAD_SIZE - (written + 1))) {
> +	if (!is_prepend_buf && memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1))) {
>  		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n",
>  			bufsize, fmt);
>  		return 1;
> @@ -496,6 +498,27 @@ dentry(void)
>  	test("  bravo/alfa|  bravo/alfa", "%12pd2|%*pd2", &test_dentry[2], 12, &test_dentry[2]);
>  }
>  
> +static struct vfsmount test_vfsmnt = {};
> +
> +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_prepend_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);

Please, add more test for scenarios when the path does not fit into
the buffer or when there are no limitations, ...

I still have to think about is_prepend_buf hack.


> +	is_prepend_buf = false;
> +}
> +
>  static void __init
>  struct_va_format(void)
>  {

Best Regards,
PEtr

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

[Justin He]

Hi Rasmus

> -----Original Message-----
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Sent: Saturday, June 12, 2021 5:28 AM
> To: Justin He <Justin.He@arm.com>; Petr Mladek <pmladek@suse.com>; Steven
> Rostedt <rostedt@goodmis.org>; Sergey Senozhatsky
> <senozhatsky@chromium.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Jonathan Corbet <corbet@lwn.net>;
> Alexander Viro <viro@zeniv.linux.org.uk>; Linus Torvalds <torvalds@linux-
> foundation.org>
> Cc: 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
> 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:
> > 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/
> >
> > 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                            | 47 +++++++++++++++++++++--
> >  2 files changed, 46 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..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)
> >  }
> >
> >  /* 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,

Ok, will remove this check statement

>
> > +           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).
>
> > +}
> >  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
> >  }
> >
> >  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 [*]
>
> > +   /* 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.

My original purpose is to avoid any memory copy/move for kvasprintf->
vsnprintf(NULL, 0,...). But as you said, this can be folded into the case
buf >= end.
Do you think whether following case should be forbidden?:
vsnprintf(NULL, 8,...).
Sorry if it is too verbose. If above invoking is valid, !buf should
still be checked.

>
> > +
> > +   /* 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 [*]:
>
> > +           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.

When code goes here, the buffer space must be as follows:
|.........|.........|
buf       p         end

So string_nocheck is safe because essential it would byte-to-byte copy p to buf.

But I agree comments are needed here.


--
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 RFCv3 2/3] lib/vsprintf.c: make %pD print full path for file

[Justin He]

Hi Petr

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Monday, June 14, 2021 11:41 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
> Subject: Re: [PATCH RFCv3 2/3] lib/vsprintf.c: make %pD print full path for
> file
>
> On Fri 2021-06-11 23:59:52, 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 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/
> >
> > 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                            | 47 +++++++++++++++++++++--
> >  2 files changed, 46 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..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)
> >  }
> >
> >  /* Handle string from a well known address. */
>
> This comment is for widen_string().
>
> string_truncate() functionality is far from obvious. It would deserve
> it's own description, including description of each parammeter.
>
> Well, do we really need it? See below.
>
> > +static char *string_truncate(char *buf, char *end, const char *s,
> > +                        u32 full_len, struct printf_spec spec)
> > +{
> > +   int lim = 0;
> > +
> > +   if (buf < end) {
> > +           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;
> > +}
> >  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
> >  }
> >
> >  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);
> > +
> > +   /* 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;
> > +
> > +   /* small space for long name */
> > +   if (buf < end && prepend_len < 0)
> > +           return string_truncate(buf, end, p, dpath_len, spec);
>
> We need this only because we allowed to write the path behind
> spec.field_width. Do I get it right?

Both of field_width and precision:
"%.14pD" or "%8.14pD"

>
> > +
> > +   /* space is enough */
> > +   return string_nocheck(buf, end, p, spec);
> >  }
>
> It easy to get lost in all the computations, including the one
> in string_truncate():
>
>       dpath_len = end - buf - prepend_len - 1;
>       reserved_size = max_t(int, dpath_len, spec.field_width);
> and
>       lim = strlen(s) - min_t(int, spec.precision, strlen(s));
>       return widen_string(buf + full_len, full_len, end - lim, spec);
>
> Please, add comments explaining the meaning of the variables a bit.
> They should help to understand why it is done this way.
>
Sure, sorry about that
>
> I tried another approach below. The main trick is that
> max_len is limited by spec.field_width and spec.precision before calling
> d_path_unsave():
>
>
>       if (check_pointer(&buf, end, f, spec))
>               return buf;
>
>       path = &f->f_path;
>       if (check_pointer(&buf, end, path, spec))
>               return buf;
>
>       max_len = end - buf;
>       if (spec.field_width >= 0 && spec.field_width < max_len)
>               max_len = spec.filed_width;
>       if (spec.precision >= 0 && spec.precision < max_len)
>               max_len = spec.precision;
>
>       p = d_path_unsafe(path, buf, max_len, &prepend_len);
>
>       /*
>        * The path has been printed from the end of the buffer.
>        * Process it like a normal string to handle "precission"
>        * and "width" effects. In the "worst" case, the string
>        * will stay as is.
>        */
>       if (buf < end) {
>               buf = string_nocheck(buf, end, p, spec);
>               /* Return buf when output was limited or did fit in. */
>               if (spec.field_width >= 0 || spec.precision >= 0 ||
>                   prepend_len >= 0) {
>                       return buf;
>               }
>               /* Otherwise, add what was missing. Ignore tail '\0' */
>               return buf - prepend_len - 1;
>       }
>
>       /*
>        * Nothing has been written to the buffer. Just count the length.
>        * I is fixed when field_with is defined. */
>       if (spec.field_width >= 0)
>               return buf + spec.field_width;
>
>       /* Otherwise, use the length of the path. */
>       dpath_len = max_len - prepend_len - 1;
>
>       /* The path might still get limited by precision number. */
>       if (spec.precision >= 0 && spec.precision < dpath_len)
>               return buf + spec.precision;
>
>       return buf + dpath_len;
>

Let me check it carefully, thanks for your suggestion.


--
Cheers,
Justin (Jia He)


>
> Note that the above code is not even compile tested. There might be
> off by one mistakes. Also, it is possible that I missed something.
>
> Best Regards,
> Petr
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 RFCv3 2/3] lib/vsprintf.c: make %pD print full path for file

[Rasmus Villemoes]

On 15/06/2021 08.43, Justin He wrote:
> Hi Rasmus
> 
>> -----Original Message-----
>> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>

>> 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.
> 
> My original purpose is to avoid any memory copy/move for kvasprintf->
> vsnprintf(NULL, 0,...). But as you said, this can be folded into the case
> buf >= end.
> Do you think whether following case should be forbidden?:
> vsnprintf(NULL, 8,...).

That is an obvious caller bug. The caller tells vsnprintf "here's a
buffer of size 8 at address 0x0". And checking buf for NULL in the guts
of %pD would anyway be completely pointless as it would crash for a fmt of

"x%pD"

or basically anything at all before %pD because those specifiers (or
literal parts) would cause a write to buf - and if that somehow
survived, the buf %pD would be given would now be (void*)1L.

>> 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.
> 
> When code goes here, the buffer space must be as follows:
> |.........|.........|
> buf       p         end
> 
> So string_nocheck is safe because essential it would byte-to-byte copy p to buf.
> 
> But I agree comments are needed here.

Yes, because no matter how string_nocheck happens to be implemented
today, some day somebody might throw in a memcpy() or do something else
that means overlapping "buf" and "s" arguments are suddenly broken in
some configurations or arches.

Rasmus

RE: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'

[Justin He]

Hi Rasmus

> -----Original Message-----
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Sent: Saturday, June 12, 2021 5:40 AM
> To: Justin He <Justin.He@arm.com>; Petr Mladek <pmladek@suse.com>; Steven
> Rostedt <rostedt@goodmis.org>; Sergey Senozhatsky
> <senozhatsky@chromium.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Jonathan Corbet <corbet@lwn.net>;
> Alexander Viro <viro@zeniv.linux.org.uk>; Linus Torvalds <torvalds@linux-
> foundation.org>
> Cc: 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
> Subject: Re: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'
>
> On 11/06/2021 17.59, Jia He 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 is prepended from the end of the buffer, the check
> > of "wrote beyond the nul-terminator" should be skipped.
>
> Sorry, that is far from enough justification.
>
> I should probably have split the "wrote beyond nul-terminator" check in two:
>
> One that checks whether any memory beyond the buffer given to
> vsnprintf() was touched (including all the padding, but possibly more
> for the cases where we pass a known-too-short buffer), symmetric to the
> "wrote before buffer" check.
>
> And then another that checks the area between the '\0' and the end of
> the given buffer - I suppose that it's fair game for vsnprintf to use
> all of that as scratch space, and for that it could be ok to add that
> boolean knob.
>
Sorry, I could have thought sth like "write beyond the buffer" had been checked by
old test cases, but seems not.
I will split the "wrote beyond nul-terminator" check into 2 parts. One for
Non-%pD case, the other for %pD.

For %pD, it needs to check whether the space beyond test_buffer[] is written


--
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 RFCv3 3/3] lib/test_printf: add test cases for '%pD'

[Justin He]

Hi Petr

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Monday, June 14, 2021 11:44 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
> Subject: Re: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'
>
> On Fri 2021-06-11 23:59:53, Jia He 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 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 ec0d5976bb69..3632bd6cf906 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, BUF_SIZE +
> PAD_SIZE - (written + 1))) {
> > +   if (!is_prepend_buf && memchr_inv(test_buffer + written + 1,
> FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1))) {
> >             pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-
> terminator\n",
> >                     bufsize, fmt);
> >             return 1;
> > @@ -496,6 +498,27 @@ dentry(void)
> >     test("  bravo/alfa|  bravo/alfa", "%12pd2|%*pd2", &test_dentry[2],
> 12, &test_dentry[2]);
> >  }
> >
> > +static struct vfsmount test_vfsmnt = {};
> > +
> > +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_prepend_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);
>
> Please, add more test for scenarios when the path does not fit into
> the buffer or when there are no limitations, ...

Indeed, thanks


--
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 RFCv3 2/3] lib/vsprintf.c: make %pD print full path for file

[Rasmus Villemoes]

On 15/06/2021 08.48, Justin He wrote:
> Hi Petr
> 

>>> +   /* no filling space at all */
>>> +   if (buf >= end || !buf)
>>> +           return buf + reserved_size;
>>> +
>>> +   /* small space for long name */
>>> +   if (buf < end && prepend_len < 0)
>>> +           return string_truncate(buf, end, p, dpath_len, spec);
>>
>> We need this only because we allowed to write the path behind
>> spec.field_width. Do I get it right?
> 
> Both of field_width and precision:
> "%.14pD" or "%8.14pD"

Precision is never gonna be used with %p (or any of its kernel
extensions) because gcc would tell you

foo.c:5:13: warning: precision used with ‘%p’ gnu_printf format [-Wformat=]
    5 |  printf("%.5p\n", foo);

and there's no way -Wformat is going to be turned off to allow that usage.

IOW, there's no need to add complexity to make "%.3pD" of something that
would normally print "/foo/bar" merely print "/fo", similar to what a
precision with %s would mean.

As for field width, I don't know if it's worth honouring, but IIRC the
original %pd and %pD did that (that's where we have widen_string etc. from).

Other %p extensions put the field with to some other use (e.g. the
bitmap and hex string printing), so they obviously cannot simultaneously
use it in the traditional sense.

Rasmus

RE: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'

[Justin He]

Hi Rasmus

> -----Original Message-----
> From: Justin He
> Sent: Tuesday, June 15, 2021 3:06 PM
> To: Rasmus Villemoes <linux@rasmusvillemoes.dk>; Petr Mladek
> <pmladek@suse.com>; Steven Rostedt <rostedt@goodmis.org>; Sergey
> Senozhatsky <senozhatsky@chromium.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Jonathan Corbet <corbet@lwn.net>;
> Alexander Viro <viro@zeniv.linux.org.uk>; Linus Torvalds <torvalds@linux-
> foundation.org>
> Cc: 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
> Subject: RE: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'
>
> Hi Rasmus
>
> > -----Original Message-----
> > From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Sent: Saturday, June 12, 2021 5:40 AM
> > To: Justin He <Justin.He@arm.com>; Petr Mladek <pmladek@suse.com>; Steven
> > Rostedt <rostedt@goodmis.org>; Sergey Senozhatsky
> > <senozhatsky@chromium.org>; Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com>; Jonathan Corbet <corbet@lwn.net>;
> > Alexander Viro <viro@zeniv.linux.org.uk>; Linus Torvalds <torvalds@linux-
> > foundation.org>
> > Cc: 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
> > Subject: Re: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'
> >
> > On 11/06/2021 17.59, Jia He 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 is prepended from the end of the buffer, the check
> > > of "wrote beyond the nul-terminator" should be skipped.
> >
> > Sorry, that is far from enough justification.
> >
> > I should probably have split the "wrote beyond nul-terminator" check in
> two:
> >
> > One that checks whether any memory beyond the buffer given to
> > vsnprintf() was touched (including all the padding, but possibly more
> > for the cases where we pass a known-too-short buffer), symmetric to the
> > "wrote before buffer" check.
> >
> > And then another that checks the area between the '\0' and the end of
> > the given buffer - I suppose that it's fair game for vsnprintf to use
> > all of that as scratch space, and for that it could be ok to add that
> > boolean knob.
> >
> Sorry, I could have thought sth like "write beyond the buffer" had been
> checked by
> old test cases, but seems not.
> I will split the "wrote beyond nul-terminator" check into 2 parts. One for
> Non-%pD case, the other for %pD.
>
> For %pD, it needs to check whether the space beyond test_buffer[] is
> written
>
>

Another question is about precision,
Do you think I should add some test cases e.g. "%.10pD" here?
I once added some, but the gcc report warning:
warning: precision used with '%p' gnu_printf

What do you think of that?


--
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 RFCv3 2/3] lib/vsprintf.c: make %pD print full path for file

[Justin He]

> -----Original Message-----
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Sent: Tuesday, June 15, 2021 3:15 PM
> To: Justin He <Justin.He@arm.com>; Petr Mladek <pmladek@suse.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
> Subject: Re: [PATCH RFCv3 2/3] lib/vsprintf.c: make %pD print full path for
> file
>
> On 15/06/2021 08.48, Justin He wrote:
> > Hi Petr
> >
>
> >>> +   /* no filling space at all */
> >>> +   if (buf >= end || !buf)
> >>> +           return buf + reserved_size;
> >>> +
> >>> +   /* small space for long name */
> >>> +   if (buf < end && prepend_len < 0)
> >>> +           return string_truncate(buf, end, p, dpath_len, spec);
> >>
> >> We need this only because we allowed to write the path behind
> >> spec.field_width. Do I get it right?
> >
> > Both of field_width and precision:
> > "%.14pD" or "%8.14pD"
>
> Precision is never gonna be used with %p (or any of its kernel
> extensions) because gcc would tell you
>
> foo.c:5:13: warning: precision used with �%p� gnu_printf format [-
> Wformat=]
>     5 |  printf("%.5p\n", foo);
>
> and there's no way -Wformat is going to be turned off to allow that usage.
>
> IOW, there's no need to add complexity to make "%.3pD" of something that
> would normally print "/foo/bar" merely print "/fo", similar to what a
> precision with %s would mean.
>
Aha, this answer my question in last email.
Thank you


--
Cheers,
Justin (Jia He)


> As for field width, I don't know if it's worth honouring, but IIRC the
> original %pd and %pD did that (that's where we have widen_string etc. from).
>
> Other %p extensions put the field with to some other use (e.g. the
> bitmap and hex string printing), so they obviously cannot simultaneously
> use it in the traditional sense.
>
> 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.

Re: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'

[Rasmus Villemoes]

On 15/06/2021 09.06, Justin He wrote:
> Hi Rasmus
> 
>> -----Original Message-----
>> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> Sent: Saturday, June 12, 2021 5:40 AM
>> To: Justin He <Justin.He@arm.com>; Petr Mladek <pmladek@suse.com>; Steven
>> Rostedt <rostedt@goodmis.org>; Sergey Senozhatsky
>> <senozhatsky@chromium.org>; Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com>; Jonathan Corbet <corbet@lwn.net>;
>> Alexander Viro <viro@zeniv.linux.org.uk>; Linus Torvalds <torvalds@linux-
>> foundation.org>
>> Cc: 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
>> Subject: Re: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'
>>
>> On 11/06/2021 17.59, Jia He 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 is prepended from the end of the buffer, the check
>>> of "wrote beyond the nul-terminator" should be skipped.
>>
>> Sorry, that is far from enough justification.
>>
>> I should probably have split the "wrote beyond nul-terminator" check in two:
>>
>> One that checks whether any memory beyond the buffer given to
>> vsnprintf() was touched (including all the padding, but possibly more
>> for the cases where we pass a known-too-short buffer), symmetric to the
>> "wrote before buffer" check.
>>
>> And then another that checks the area between the '\0' and the end of
>> the given buffer - I suppose that it's fair game for vsnprintf to use
>> all of that as scratch space, and for that it could be ok to add that
>> boolean knob.
>>
> Sorry, I could have thought sth like "write beyond the buffer" had been checked by
> old test cases, but seems not.

It does. Before each (sub)test, we have (assume PAD_SIZE=4, BUF_SIZE=12)


|    <- alloced_buffer ->    |
|  PAD |  test_buffer | PAD  |
| **** | ************ | **** |

Then after snprintf(buf, 10, "pizza") we have

|    <- alloced_buffer ->    |
|  PAD |  test_buffer | PAD  |
| **** | pizza0****** | **** |
A      B       C   D         E

(with 0 being the nul character). Then

        if (memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE)) {

checks whether snprint wrote anything between A and B, while

        if (memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE +
PAD_SIZE - (written + 1))) {

checks whether there was a write between C and E.

What I'm saying is that I can see it being reasonable for (some helper
inside) snprintf to actually write something beyond C, but certainly
never beyond D. So the "wrote beyond" test could be split up, with the
first half possibly being allowed to be opt-out for certain test cases.

> I will split the "wrote beyond nul-terminator" check into 2 parts. One for
> Non-%pD case, the other for %pD.
> 
> For %pD, it needs to check whether the space beyond test_buffer[] is written

No, that's not the right way to do this. Let me cook up a patch you can
include in your series.

Rasmus

Re: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'

[Rasmus Villemoes]

On 15/06/2021 09.07, Justin He wrote:
> Hi Petr
> 

>>> +static void __init
>>> +f_d_path(void)
>>> +{
>>> +   test("(null)", "%pD", NULL);
>>> +   test("(efault)", "%pD", PTR_INVALID);
>>> +
>>> +   is_prepend_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);
>>
>> Please, add more test for scenarios when the path does not fit into
>> the buffer or when there are no limitations, ...
> 
> Indeed, thanks

Doesn't the existing test() helper do this for you automatically?

        /*
         * Every fmt+args is subjected to four tests: Three where we
         * tell vsnprintf varying buffer sizes (plenty, not quite
         * enough and 0), and then we also test that kvasprintf would
         * be able to print it as expected.
         */

I don't see why one would need to do anything special for %pD.

Rasmus

RE: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'

[Justin He]

> -----Original Message-----
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Sent: Tuesday, June 15, 2021 3:48 PM
> To: Justin He <Justin.He@arm.com>; Petr Mladek <pmladek@suse.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>; Sergey Senozhatsky
> <senozhatsky@chromium.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; 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
> Subject: Re: [PATCH RFCv3 3/3] lib/test_printf: add test cases for '%pD'
>
> On 15/06/2021 09.07, Justin He wrote:
> > Hi Petr
> >
>
> >>> +static void __init
> >>> +f_d_path(void)
> >>> +{
> >>> +   test("(null)", "%pD", NULL);
> >>> +   test("(efault)", "%pD", PTR_INVALID);
> >>> +
> >>> +   is_prepend_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);
> >>
> >> Please, add more test for scenarios when the path does not fit into
> >> the buffer or when there are no limitations, ...
> >
> > Indeed, thanks
>
> Doesn't the existing test() helper do this for you automatically?
>
>         /*
>          * Every fmt+args is subjected to four tests: Three where we
>          * tell vsnprintf varying buffer sizes (plenty, not quite
>          * enough and 0), and then we also test that kvasprintf would
>          * be able to print it as expected.
>          */
>

Yes, it had invoked vsnprintf for 3 times in __test()
vsnprintf(buf,256)
vsnprintf(buf,random_bytes,...)
vsnprintf(buf, 0,...);
seems no need to add more test cases

> I don't see why one would need to do anything special for %pD.

Okay, got it, agree


--
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 RFCv3 3/3] lib/test_printf: add test cases for '%pD'

[Andy Shevchenko]

On Tue, Jun 15, 2021 at 07:56:01AM +0000, Justin He wrote:
> > From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Sent: Tuesday, June 15, 2021 3:48 PM
> > On 15/06/2021 09.07, Justin He wrote:

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

Just a reminder that you still have this footer that prevents (some) people to
respond to your messages.

-- 
With Best Regards,
Andy Shevchenko

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

[Justin He]

> -----Original Message-----
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Sent: Saturday, June 12, 2021 5:28 AM
> To: Justin He <Justin.He@arm.com>; Petr Mladek <pmladek@suse.com>; Steven
> Rostedt <rostedt@goodmis.org>; Sergey Senozhatsky
> <senozhatsky@chromium.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Jonathan Corbet <corbet@lwn.net>;
> Alexander Viro <viro@zeniv.linux.org.uk>; Linus Torvalds <torvalds@linux-
> foundation.org>
> Cc: 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
> 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:
> > 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/
> >
> > 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                            | 47 +++++++++++++++++++++--
> >  2 files changed, 46 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..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)
> >  }
> >
> >  /* 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,
>
> > +           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)



>
> > +}
> >  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
> >  }
> >
> >  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 [*]
>
> > +   /* 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.
>
> > +
> > +   /* 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 [*]:
>
> > +           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.

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

[Justin He]

Hi Petr

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Monday, June 14, 2021 11:41 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
> Subject: Re: [PATCH RFCv3 2/3] lib/vsprintf.c: make %pD print full path
> for file
> 
> On Fri 2021-06-11 23:59:52, 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 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/
> >
> > 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                            | 47 +++++++++++++++++++++--
> >  2 files changed, 46 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..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)
> >  }
> >
> >  /* Handle string from a well known address. */
> 
> This comment is for widen_string().
> 
> string_truncate() functionality is far from obvious. It would deserve
> it's own description, including description of each parammeter.
> 
> Well, do we really need it? See below.
> 
> > +static char *string_truncate(char *buf, char *end, const char *s,
> > +			     u32 full_len, struct printf_spec spec)
> > +{
> > +	int lim = 0;
> > +
> > +	if (buf < end) {
> > +		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;
> > +}
> >  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
> >  }
> >
> >  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);
> > +
> > +	/* 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;
> > +
> > +	/* small space for long name */
> > +	if (buf < end && prepend_len < 0)
> > +		return string_truncate(buf, end, p, dpath_len, spec);
> 
> We need this only because we allowed to write the path behind
> spec.field_width. Do I get it right?
> 
> > +
> > +	/* space is enough */
> > +	return string_nocheck(buf, end, p, spec);
> >  }
> 
> It easy to get lost in all the computations, including the one
> in string_truncate():
> 
> 	dpath_len = end - buf - prepend_len - 1;
> 	reserved_size = max_t(int, dpath_len, spec.field_width);
> and
> 	lim = strlen(s) - min_t(int, spec.precision, strlen(s));
> 	return widen_string(buf + full_len, full_len, end - lim, spec);
> 
> Please, add comments explaining the meaning of the variables a bit.
> They should help to understand why it is done this way.
> 
> 
> I tried another approach below. The main trick is that
> max_len is limited by spec.field_width and spec.precision before calling
> d_path_unsave():
> 
> 
> 	if (check_pointer(&buf, end, f, spec))
> 		return buf;
> 
> 	path = &f->f_path;
> 	if (check_pointer(&buf, end, path, spec))
> 		return buf;
> 
> 	max_len = end - buf;
> 	if (spec.field_width >= 0 && spec.field_width < max_len)
> 		max_len = spec.filed_width;
> 	if (spec.precision >= 0 && spec.precision < max_len)
> 		max_len = spec.precision;
> 
> 	p = d_path_unsafe(path, buf, max_len, &prepend_len);
> 
> 	/*
> 	 * The path has been printed from the end of the buffer.
> 	 * Process it like a normal string to handle "precission"
> 	 * and "width" effects. In the "worst" case, the string
> 	 * will stay as is.
> 	 */
> 	if (buf < end) {
> 		buf = string_nocheck(buf, end, p, spec);
> 		/* Return buf when output was limited or did fit in. */
> 		if (spec.field_width >= 0 || spec.precision >= 0 ||
> 		    prepend_len >= 0) {
> 			return buf;
> 		}
> 		/* Otherwise, add what was missing. Ignore tail '\0' */
> 		return buf - prepend_len - 1;
> 	}
> 
> 	/*
> 	 * Nothing has been written to the buffer. Just count the length.
> 	 * I is fixed when field_with is defined. */
> 	if (spec.field_width >= 0)
> 		return buf + spec.field_width;
> 
> 	/* Otherwise, use the length of the path. */
> 	dpath_len = max_len - prepend_len - 1;
> 
> 	/* The path might still get limited by precision number. */
> 	if (spec.precision >= 0 && spec.precision < dpath_len)
> 		return buf + spec.precision;
> 
> 	return buf + dpath_len;

As Rasmus confirmed that we needn't consider the spec.precision,
the code can be more concise.
I will send out v4 after testing together with one test_printf patch
from Rasmus.


--
Cheers,
Justin (Jia He)


> 
> 
> Note that the above code is not even compile tested. There might be
> off by one mistakes. Also, it is possible that I missed something.
> 
> Best Regards,
> Petr