[PATCH v2 0/4] make '%pD' print the full path of file

[PATCH v2 0/4] make '%pD' print the full path of file

[Jia He]

Background
==========
Linus suggested printing the full path of 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_unsafe() 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. 

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

TODO
====
I plan to do the followup work after '%pD' behavior is changed.
- s390/hmcdrv: remove the redundant directory path in printing string.
- fs/iomap: simplify the iomap_swapfile_fail() with '%pD'.
- simplify the string printing when file_path() is invoked(in some
  cases, not all).
- change the previous '%pD[2,3,4]' to '%pD'
   
Changelog
=========
v2:
- refine the commit msg/comments (Andy)
- pass the validator check by "make C=1 W=1"
- add the R-b for patch 4/4 from Andy

v1: https://lkml.org/lkml/2021/6/22/680
- remove the RFC tag
- refine the commit msg/comments(by Petr, Andy)
- make using_scratch_space a new parameter of the test case 

RFCv4:
- don't support spec.precision anymore for '%pD'
- add Rasmus's patch into this series
 
RFCv3:
- 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.

RFCv2: 
- 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

RFCv1: https://lkml.org/lkml/2021/5/8/122

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

Jia He (3):
  fs: introduce helper d_path_unsafe()
  lib/vsprintf.c: make '%pD' print the full path of file
  lib/test_printf.c: add test cases for '%pD'

Rasmus Villemoes (1):
  lib/test_printf.c: split write-beyond-buffer check in two

 Documentation/core-api/printk-formats.rst |   5 +-
 fs/d_path.c                               | 122 ++++++++++++++++++++--
 include/linux/dcache.h                    |   1 +
 lib/test_printf.c                         |  54 ++++++++--
 lib/vsprintf.c                            |  40 ++++++-
 5 files changed, 199 insertions(+), 23 deletions(-)

-- 
2.17.1

[PATCH v2 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 purposes. Besides,
an additional return value *prenpend_len* is used to get the full
path length of the dentry, ingoring the tail '\0'.
the full path length = end - buf - prepend_length - 1.

Previously it will skip the prepend_name() loop at once in
__prepen_path() when the buffer length is not enough or even negative.
prepend_name_with_len() will get the full length of dentry name
together with the parent recursively regardless of the buffer length.

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

diff --git a/fs/d_path.c b/fs/d_path.c
index 23a53f7b5c71..7a3ea88f8c5c 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -33,9 +33,8 @@ static void prepend(struct prepend_buffer *p, const char *str, int namelen)
 
 /**
  * prepend_name - prepend a pathname in front of current buffer pointer
- * @buffer: buffer pointer
- * @buflen: allocated length of the buffer
- * @name:   name string and length qstr structure
+ * @p: prepend buffer which contains buffer pointer and allocated length
+ * @name: name string and length qstr structure
  *
  * With RCU path tracing, it may race with d_move(). Use READ_ONCE() to
  * make sure that either the old or the new name pointer and length are
@@ -68,9 +67,84 @@ static bool prepend_name(struct prepend_buffer *p, const struct qstr *name)
 	return true;
 }
 
+/**
+ * prepend_name_with_len - prepend a pathname in front of current buffer
+ * pointer with limited orig_buflen.
+ * @p: prepend buffer which contains buffer pointer and allocated length
+ * @name: name string and length qstr structure
+ * @orig_buflen: original length of the buffer
+ *
+ * p.ptr is updated each time when prepends dentry name and its parent.
+ * Given the orginal buffer length might be less than name string, the
+ * dentry name can be moved or truncated. Returns at once if !buf or
+ * original length is not positive to avoid memory copy.
+ *
+ * Load acquire is needed to make sure that we see that terminating NUL,
+ * which is similar to prepend_name().
+ */
+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);
+
+		/* memcpy src */
+		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) {
+			/* 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) {
+			/* Can be partially filled, and drop last dentry */
+			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 +171,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;
@@ -108,8 +181,7 @@ static int __prepend_path(const struct dentry *dentry, const struct mount *mnt,
  * prepend_path - Prepend path string to a buffer
  * @path: the dentry/vfsmount to report
  * @root: root vfsmnt/dentry
- * @buffer: pointer to the end of the buffer
- * @buflen: pointer to buffer length
+ * @p: prepend buffer which contains buffer pointer and allocated length
  *
  * The function will first try to write out the pathname without taking any
  * lock other than the RCU read lock to make sure that dentries won't go away.
@@ -263,6 +335,42 @@ char *d_path(const struct path *path, char *buf, int buflen)
 }
 EXPORT_SYMBOL(d_path);
 
+/**
+ * d_path_unsafe - return the full path of a dentry without taking
+ * any seqlock/spinlock. This helper is typical for debugging purposes.
+ * @path: path to report
+ * @buf: buffer to return value in
+ * @buflen: buffer length
+ * @prepend_len: prepended length when going through the full path
+ *
+ * Convert a dentry into an ASCII path name.
+ *
+ * Returns a pointer into the buffer or an error code if the path was
+ * errous.
+ *
+ * @buf can be NULL, and @buflen can be 0 or negative. But NULL @buf
+ * and buflen>0 is considered as an obvious caller bug.
+ *
+ */
+char *d_path_unsafe(const struct path *path, char *buf, int buflen,
+		    int *prepend_len)
+{
+	struct path root;
+	DECLARE_BUFFER(b, buf, buflen);
+	struct mount *mnt = real_mount(path->mnt);
+
+	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

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

[Andy Shevchenko]

On Wed, Jun 23, 2021 at 01:50:08PM +0800, Jia He wrote:
> This helper is similar to d_path() except that it doesn't take any
> seqlock/spinlock. It is typical for debugging purposes. Besides,
> an additional return value *prenpend_len* is used to get the full
> path length of the dentry, ingoring the tail '\0'.
> the full path length = end - buf - prepend_length - 1.
> 
> Previously it will skip the prepend_name() loop at once in
> __prepen_path() when the buffer length is not enough or even negative.
> prepend_name_with_len() will get the full length of dentry name
> together with the parent recursively regardless of the buffer length.

...

>  /**
>   * prepend_name - prepend a pathname in front of current buffer pointer
> - * @buffer: buffer pointer
> - * @buflen: allocated length of the buffer
> - * @name:   name string and length qstr structure
> + * @p: prepend buffer which contains buffer pointer and allocated length
> + * @name: name string and length qstr structure
>   *
>   * With RCU path tracing, it may race with d_move(). Use READ_ONCE() to
>   * make sure that either the old or the new name pointer and length are

This should be separate patch. You are sending new version too fast...
Instead of speeding up it will slow down the review process.

...

> +	const char *dname = smp_load_acquire(&name->name); /* ^^^ */

I have commented on the comment here. What does it mean for mere reader?

> +	int dlen = READ_ONCE(name->len);
> +	char *s;
> +	int last_len = p->len;

Reversed xmas tree order, please.

The rule of thumb is when you have gotten a comment against a piece of code,
try to fix all similar places at once.

...

> @@ -108,8 +181,7 @@ static int __prepend_path(const struct dentry *dentry, const struct mount *mnt,
>   * prepend_path - Prepend path string to a buffer
>   * @path: the dentry/vfsmount to report
>   * @root: root vfsmnt/dentry
> - * @buffer: pointer to the end of the buffer
> - * @buflen: pointer to buffer length
> + * @p: prepend buffer which contains buffer pointer and allocated length
>   *
>   * The function will first try to write out the pathname without taking any
>   * lock other than the RCU read lock to make sure that dentries won't go away.

Kernel doc fix should be in a separate patch.


-- 
With Best Regards,
Andy Shevchenko

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

[Justin He]

Hi Andy

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Wednesday, June 23, 2021 5:11 PM
> To: Justin He <Justin.He@arm.com>
> Cc: Petr Mladek <pmladek@suse.com>; Steven Rostedt <rostedt@goodmis.org>;
> Sergey Senozhatsky <senozhatsky@chromium.org>; 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>; Christoph
> Hellwig <hch@infradead.org>; nd <nd@arm.com>
> Subject: Re: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()
> 
> On Wed, Jun 23, 2021 at 01:50:08PM +0800, Jia He wrote:
> > This helper is similar to d_path() except that it doesn't take any
> > seqlock/spinlock. It is typical for debugging purposes. Besides,
> > an additional return value *prenpend_len* is used to get the full
> > path length of the dentry, ingoring the tail '\0'.
> > the full path length = end - buf - prepend_length - 1.
> >
> > Previously it will skip the prepend_name() loop at once in
> > __prepen_path() when the buffer length is not enough or even negative.
> > prepend_name_with_len() will get the full length of dentry name
> > together with the parent recursively regardless of the buffer length.
> 
> ...
> 
> >  /**
> >   * prepend_name - prepend a pathname in front of current buffer pointer
> > - * @buffer: buffer pointer
> > - * @buflen: allocated length of the buffer
> > - * @name:   name string and length qstr structure
> > + * @p: prepend buffer which contains buffer pointer and allocated length
> > + * @name: name string and length qstr structure
> >   *
> >   * With RCU path tracing, it may race with d_move(). Use READ_ONCE() to
> >   * make sure that either the old or the new name pointer and length are
> 
> This should be separate patch. You are sending new version too fast...
> Instead of speeding up it will slow down the review process.

Okay, sorry about sending the new version too fast.
I will slow it down and check carefully before sending out.
> 
> ...
> 
> > +	const char *dname = smp_load_acquire(&name->name); /* ^^^ */
> 
> I have commented on the comment here. What does it mean for mere reader?
> 

Do you suggest making the comment "/* ^^^ */" more clear?
It is detailed already in prepend_name_with_len()'s comments:
> * Load acquire is needed to make sure that we see that terminating NUL,
> * which is similar to prepend_name().

Or do you suggest removing the smp_load_acquire()?

> > +	int dlen = READ_ONCE(name->len);
> > +	char *s;
> > +	int last_len = p->len;
> 
> Reversed xmas tree order, please.
> 
> The rule of thumb is when you have gotten a comment against a piece of code,
> try to fix all similar places at once.

Sorry, I misunderstood it, will change it with reverse xmas tree order.

--
Cheers,
Justin (Jia He)

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

[Justin He]

Hi Andy

> -----Original Message-----
> From: Justin He
> Sent: Thursday, June 24, 2021 1:49 PM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Petr Mladek <pmladek@suse.com>; Steven Rostedt <rostedt@goodmis.org>;
> Sergey Senozhatsky <senozhatsky@chromium.org>; 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>; Christoph
> Hellwig <hch@infradead.org>; nd <nd@arm.com>
> Subject: RE: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()
> 
> Hi Andy
> 
> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Wednesday, June 23, 2021 5:11 PM
> > To: Justin He <Justin.He@arm.com>
> > Cc: Petr Mladek <pmladek@suse.com>; Steven Rostedt <rostedt@goodmis.org>;
> > Sergey Senozhatsky <senozhatsky@chromium.org>; 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>; Christoph
> > Hellwig <hch@infradead.org>; nd <nd@arm.com>
> > Subject: Re: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()
> >
> > On Wed, Jun 23, 2021 at 01:50:08PM +0800, Jia He wrote:
> > > This helper is similar to d_path() except that it doesn't take any
> > > seqlock/spinlock. It is typical for debugging purposes. Besides,
> > > an additional return value *prenpend_len* is used to get the full
> > > path length of the dentry, ingoring the tail '\0'.
> > > the full path length = end - buf - prepend_length - 1.
> > >
> > > Previously it will skip the prepend_name() loop at once in
> > > __prepen_path() when the buffer length is not enough or even negative.
> > > prepend_name_with_len() will get the full length of dentry name
> > > together with the parent recursively regardless of the buffer length.
> >
> > ...
> >
> > >  /**
> > >   * prepend_name - prepend a pathname in front of current buffer
> pointer
> > > - * @buffer: buffer pointer
> > > - * @buflen: allocated length of the buffer
> > > - * @name:   name string and length qstr structure
> > > + * @p: prepend buffer which contains buffer pointer and allocated
> length
> > > + * @name: name string and length qstr structure
> > >   *
> > >   * With RCU path tracing, it may race with d_move(). Use READ_ONCE()
> to
> > >   * make sure that either the old or the new name pointer and length
> are
> >
> > This should be separate patch. You are sending new version too fast...
> > Instead of speeding up it will slow down the review process.
> 
> Okay, sorry about sending the new version too fast.
> I will slow it down and check carefully before sending out.
> >
> > ...
> >
> > > +	const char *dname = smp_load_acquire(&name->name); /* ^^^ */
> >
> > I have commented on the comment here. What does it mean for mere reader?
> >
> 
> Do you suggest making the comment "/* ^^^ */" more clear?
> It is detailed already in prepend_name_with_len()'s comments:
> > * Load acquire is needed to make sure that we see that terminating NUL,
> > * which is similar to prepend_name().
> 
> Or do you suggest removing the smp_load_acquire()?

This smp_load_acquire() is to add a barrier btw ->name and ->len. This is the
pair of smp_store_release() in __d_alloc().
Please see the details in 
commit 7088efa9137a15d7d21e3abce73e40c9c8a18d68

    fs/dcache: Use release-acquire for name/length update
    
    The code in __d_alloc() carefully orders filling in the NUL character
    of the name (and the length, hash, and the name itself) with assigning
    of the name itself.  However, prepend_name() does not order the accesses
    to the ->name and ->len fields, other than on TSO systems.  This commit
    therefore replaces prepend_name()'s READ_ONCE() of ->name with an
    smp_load_acquire(), which orders against the subsequent READ_ONCE() of
    ->len.  Because READ_ONCE() now incorporates smp_read_barrier_depends(),
    prepend_name()'s smp_read_barrier_depends() is removed.  Finally,
    to save a line, the smp_wmb()/store pair in __d_alloc() is replaced
    by smp_store_release().

I prefer to keep it as previous, what do you think of it?


--
Cheers,
Justin (Jia He)

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

[Andy Shevchenko]

On Wed, Jul 14, 2021 at 08:33:10AM +0000, Justin He wrote:
> > From: Justin He
> > Sent: Thursday, June 24, 2021 1:49 PM
> > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Sent: Wednesday, June 23, 2021 5:11 PM
> > > On Wed, Jun 23, 2021 at 01:50:08PM +0800, Jia He wrote:

...

> > > > +	const char *dname = smp_load_acquire(&name->name); /* ^^^ */
> > >
> > > I have commented on the comment here. What does it mean for mere reader?
> > >
> > 
> > Do you suggest making the comment "/* ^^^ */" more clear?

Yes.

> > It is detailed already in prepend_name_with_len()'s comments:
> > > * Load acquire is needed to make sure that we see that terminating NUL,
> > > * which is similar to prepend_name().

-- 
With Best Regards,
Andy Shevchenko

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

[Justin He]

Hi Andy, Petr

> -----Original Message-----
> From: Jia He <justin.he@arm.com>
> Sent: Wednesday, June 23, 2021 1:50 PM
> To: 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>
> 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; Matthew Wilcox <willy@infradead.org>; Christoph
> Hellwig <hch@infradead.org>; nd <nd@arm.com>; Justin He <Justin.He@arm.com>
> Subject: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()
> 
> This helper is similar to d_path() except that it doesn't take any
> seqlock/spinlock. It is typical for debugging purposes. Besides,
> an additional return value *prenpend_len* is used to get the full
> path length of the dentry, ingoring the tail '\0'.
> the full path length = end - buf - prepend_length - 1.
> 
> Previously it will skip the prepend_name() loop at once in
> __prepen_path() when the buffer length is not enough or even negative.
> prepend_name_with_len() will get the full length of dentry name
> together with the parent recursively regardless of the buffer length.
> 
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Jia He <justin.he@arm.com>
> ---
>  fs/d_path.c            | 122 ++++++++++++++++++++++++++++++++++++++---
>  include/linux/dcache.h |   1 +
>  2 files changed, 116 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/d_path.c b/fs/d_path.c
> index 23a53f7b5c71..7a3ea88f8c5c 100644
> --- a/fs/d_path.c
> +++ b/fs/d_path.c
> @@ -33,9 +33,8 @@ static void prepend(struct prepend_buffer *p, const char
> *str, int namelen)
> 
>  /**
>   * prepend_name - prepend a pathname in front of current buffer pointer
> - * @buffer: buffer pointer
> - * @buflen: allocated length of the buffer
> - * @name:   name string and length qstr structure
> + * @p: prepend buffer which contains buffer pointer and allocated length
> + * @name: name string and length qstr structure
>   *
>   * With RCU path tracing, it may race with d_move(). Use READ_ONCE() to
>   * make sure that either the old or the new name pointer and length are
> @@ -68,9 +67,84 @@ static bool prepend_name(struct prepend_buffer *p,
> const struct qstr *name)
>  	return true;
>  }
> 
> +/**
> + * prepend_name_with_len - prepend a pathname in front of current buffer
> + * pointer with limited orig_buflen.
> + * @p: prepend buffer which contains buffer pointer and allocated length
> + * @name: name string and length qstr structure
> + * @orig_buflen: original length of the buffer
> + *
> + * p.ptr is updated each time when prepends dentry name and its parent.
> + * Given the orginal buffer length might be less than name string, the
> + * dentry name can be moved or truncated. Returns at once if !buf or
> + * original length is not positive to avoid memory copy.
> + *
> + * Load acquire is needed to make sure that we see that terminating NUL,
> + * which is similar to prepend_name().
> + */
> +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);
> +
> +		/* memcpy src */
> +		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) {
> +			/* 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) {
> +			/* Can be partially filled, and drop last dentry */
> +			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 +171,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);

I have new concern here.
Previously,  __prepend_path() would break the loop at once when p.len<0.
And the return value of __prepend_path() was 0.
The caller of prepend_path() would typically check as follows:
  if (prepend_path(...) > 0)
  	do_sth();

After I replaced prepend_name() with prepend_name_with_len(),
the return value of prepend_path() is possibly positive
together with p.len<0. The behavior is different from previous.

The possible ways I figured out to resolve this:
1. parameterize a new one *need_len* for prepend_path
2. change __prepend_path() to return 0 instead of 1,2,3
if p.len<0 at that point

the patch of solution 2 looks like(basically verified):
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -144,6 +144,7 @@ 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;
+       int ret = 0;
 
        while (dentry != root->dentry || &mnt->mnt != root->mnt) {
                const struct dentry *parent = READ_ONCE(dentry->d_parent);
@@ -161,19 +162,27 @@ static int __prepend_path(const struct dentry *dentry, const struct mount *mnt,
                        mnt_ns = READ_ONCE(mnt->mnt_ns);
                        /* open-coded is_mounted() to use local mnt_ns */
                        if (!IS_ERR_OR_NULL(mnt_ns) && !is_anon_ns(mnt_ns))
-                               return 1;       // absolute root
+                               ret = 1;        // absolute root
                        else
-                               return 2;       // detached or not attached yet
+                               ret = 2;        // detached or not attached yet
+
+                       break;
                }
 
-               if (unlikely(dentry == parent))
+               if (unlikely(dentry == parent)) {
                        /* Escaped? */
-                       return 3;
+                       ret = 3;
+                       break;
+               }
 
                prefetch(parent);
                prepend_name_with_len(p, &dentry->d_name, orig_buflen);
                dentry = parent;
        }
+
+       if (p->len >= 0)
+               return ret;
+
        return 0;
 }

What do you think of it?

--
Cheers,
Justin (Jia He)

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

[Petr Mladek]

On Mon 2021-06-28 05:13:51, Justin He wrote:
> Hi Andy, Petr
> 
> > -----Original Message-----
> > From: Jia He <justin.he@arm.com>
> > Sent: Wednesday, June 23, 2021 1:50 PM
> > To: 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>
> > 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; Matthew Wilcox <willy@infradead.org>; Christoph
> > Hellwig <hch@infradead.org>; nd <nd@arm.com>; Justin He <Justin.He@arm.com>
> > Subject: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()
> > 
> > This helper is similar to d_path() except that it doesn't take any
> > seqlock/spinlock. It is typical for debugging purposes. Besides,
> > an additional return value *prenpend_len* is used to get the full
> > path length of the dentry, ingoring the tail '\0'.
> > the full path length = end - buf - prepend_length - 1.
> > 
> > Previously it will skip the prepend_name() loop at once in
> > __prepen_path() when the buffer length is not enough or even negative.
> > prepend_name_with_len() will get the full length of dentry name
> > together with the parent recursively regardless of the buffer length.
> > 
> > Suggested-by: Matthew Wilcox <willy@infradead.org>
> > Signed-off-by: Jia He <justin.he@arm.com>
> > ---
> >  fs/d_path.c            | 122 ++++++++++++++++++++++++++++++++++++++---
> >  include/linux/dcache.h |   1 +
> >  2 files changed, 116 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/d_path.c b/fs/d_path.c
> > index 23a53f7b5c71..7a3ea88f8c5c 100644
> > --- a/fs/d_path.c
> > +++ b/fs/d_path.c
> > @@ -33,9 +33,8 @@ static void prepend(struct prepend_buffer *p, const char
> > *str, int namelen)
> > 
> >  /**
> >   * prepend_name - prepend a pathname in front of current buffer pointer
> > - * @buffer: buffer pointer
> > - * @buflen: allocated length of the buffer
> > - * @name:   name string and length qstr structure
> > + * @p: prepend buffer which contains buffer pointer and allocated length
> > + * @name: name string and length qstr structure
> >   *
> >   * With RCU path tracing, it may race with d_move(). Use READ_ONCE() to
> >   * make sure that either the old or the new name pointer and length are
> > @@ -68,9 +67,84 @@ static bool prepend_name(struct prepend_buffer *p,
> > const struct qstr *name)
> >  	return true;
> >  }
> > 
> > +/**
> > + * prepend_name_with_len - prepend a pathname in front of current buffer
> > + * pointer with limited orig_buflen.
> > + * @p: prepend buffer which contains buffer pointer and allocated length
> > + * @name: name string and length qstr structure
> > + * @orig_buflen: original length of the buffer
> > + *
> > + * p.ptr is updated each time when prepends dentry name and its parent.
> > + * Given the orginal buffer length might be less than name string, the
> > + * dentry name can be moved or truncated. Returns at once if !buf or
> > + * original length is not positive to avoid memory copy.
> > + *
> > + * Load acquire is needed to make sure that we see that terminating NUL,
> > + * which is similar to prepend_name().
> > + */
> > +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);
> > +
> > +		/* memcpy src */
> > +		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) {
> > +			/* 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) {
> > +			/* Can be partially filled, and drop last dentry */
> > +			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 +171,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);
> 
> I have new concern here.
> Previously,  __prepend_path() would break the loop at once when p.len<0.
> And the return value of __prepend_path() was 0.
> The caller of prepend_path() would typically check as follows:
>   if (prepend_path(...) > 0)
>   	do_sth();
> 
> After I replaced prepend_name() with prepend_name_with_len(),
> the return value of prepend_path() is possibly positive
> together with p.len<0. The behavior is different from previous.

I do not feel qualified to make decision here.I do not have enough
experience with this code.

Anyway, the new behavior looks correct to me. The return values
1, 2, 3 mean that there was something wrong with the path. The
new code checks the entire path which looks correct to me.

We only need to make sure that all callers handle this correctly.
Both __prepend_path() and prepend_path() are static so that
the scope is well defined.

If I did not miss something, all callers handle this correctly:

   + __d_patch() ignores buf when prepend_patch() > 0

   + d_absolute_path() and d_path() use extract_string(). It ignores
     the buffer when p->len < 0

   + SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned long, size)
     ignores the path as well. It is less obvious because it is done
     this way:

		len = PATH_MAX - b.len;
		if (unlikely(len > PATH_MAX))
			error = -ENAMETOOLONG;

     The condition (len > PATH_MAX) is true when b.len is negative.


Best Regards,
Petr

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

[Justin He]

Hi Petr

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Monday, June 28, 2021 5:07 PM
> To: Justin He <Justin.He@arm.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; 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>; Christoph Hellwig <hch@infradead.org>; nd
> <nd@arm.com>; Steven Rostedt <rostedt@goodmis.org>; Sergey Senozhatsky
> <senozhatsky@chromium.org>; Rasmus Villemoes <linux@rasmusvillemoes.dk>;
> Jonathan Corbet <corbet@lwn.net>; Alexander Viro <viro@zeniv.linux.org.uk>;
> Linus Torvalds <torvalds@linux-foundation.org>
> Subject: Re: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()
> 
> On Mon 2021-06-28 05:13:51, Justin He wrote:
> > Hi Andy, Petr
> >
> > > -----Original Message-----
> > > From: Jia He <justin.he@arm.com>
> > > Sent: Wednesday, June 23, 2021 1:50 PM
> > > To: 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>
> > > 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; Matthew Wilcox <willy@infradead.org>;
> Christoph
> > > Hellwig <hch@infradead.org>; nd <nd@arm.com>; Justin He
> <Justin.He@arm.com>
> > > Subject: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()
> > >
> > > This helper is similar to d_path() except that it doesn't take any
> > > seqlock/spinlock. It is typical for debugging purposes. Besides,
> > > an additional return value *prenpend_len* is used to get the full
> > > path length of the dentry, ingoring the tail '\0'.
> > > the full path length = end - buf - prepend_length - 1.
> > >
> > > Previously it will skip the prepend_name() loop at once in
> > > __prepen_path() when the buffer length is not enough or even negative.
> > > prepend_name_with_len() will get the full length of dentry name
> > > together with the parent recursively regardless of the buffer length.
> > >
> > > Suggested-by: Matthew Wilcox <willy@infradead.org>
> > > Signed-off-by: Jia He <justin.he@arm.com>
> > > ---
> > >  fs/d_path.c            | 122 ++++++++++++++++++++++++++++++++++++++---
> > >  include/linux/dcache.h |   1 +
> > >  2 files changed, 116 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/d_path.c b/fs/d_path.c
> > > index 23a53f7b5c71..7a3ea88f8c5c 100644
> > > --- a/fs/d_path.c
> > > +++ b/fs/d_path.c
> > > @@ -33,9 +33,8 @@ static void prepend(struct prepend_buffer *p, const
> char
> > > *str, int namelen)
> > >
> > >  /**
> > >   * prepend_name - prepend a pathname in front of current buffer
> pointer
> > > - * @buffer: buffer pointer
> > > - * @buflen: allocated length of the buffer
> > > - * @name:   name string and length qstr structure
> > > + * @p: prepend buffer which contains buffer pointer and allocated
> length
> > > + * @name: name string and length qstr structure
> > >   *
> > >   * With RCU path tracing, it may race with d_move(). Use READ_ONCE()
> to
> > >   * make sure that either the old or the new name pointer and length
> are
> > > @@ -68,9 +67,84 @@ static bool prepend_name(struct prepend_buffer *p,
> > > const struct qstr *name)
> > >  	return true;
> > >  }
> > >
> > > +/**
> > > + * prepend_name_with_len - prepend a pathname in front of current
> buffer
> > > + * pointer with limited orig_buflen.
> > > + * @p: prepend buffer which contains buffer pointer and allocated
> length
> > > + * @name: name string and length qstr structure
> > > + * @orig_buflen: original length of the buffer
> > > + *
> > > + * p.ptr is updated each time when prepends dentry name and its parent.
> > > + * Given the orginal buffer length might be less than name string, the
> > > + * dentry name can be moved or truncated. Returns at once if !buf or
> > > + * original length is not positive to avoid memory copy.
> > > + *
> > > + * Load acquire is needed to make sure that we see that terminating
> NUL,
> > > + * which is similar to prepend_name().
> > > + */
> > > +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);
> > > +
> > > +		/* memcpy src */
> > > +		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) {
> > > +			/* 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) {
> > > +			/* Can be partially filled, and drop last dentry */
> > > +			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 +171,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);
> >
> > I have new concern here.
> > Previously,  __prepend_path() would break the loop at once when p.len<0.
> > And the return value of __prepend_path() was 0.
> > The caller of prepend_path() would typically check as follows:
> >   if (prepend_path(...) > 0)
> >   	do_sth();
> >
> > After I replaced prepend_name() with prepend_name_with_len(),
> > the return value of prepend_path() is possibly positive
> > together with p.len<0. The behavior is different from previous.
> 
> I do not feel qualified to make decision here.I do not have enough
> experience with this code.
> 
> Anyway, the new behavior looks correct to me. The return values
> 1, 2, 3 mean that there was something wrong with the path. The
> new code checks the entire path which looks correct to me.

Okay, got it. Thanks for the explanation.
Seems my concern is not necessary. I once compared the old and new
prepend_path return value, they are always the same in my test scenarios.

--
Cheers,
Justin (Jia He)

[PATCH v2 2/4] lib/vsprintf.c: make '%pD' print the full path of 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 [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 the full path of that file.

If someone invokes snprintf() with small but positive space,
prepend_name_with_len() moves or truncates the string partially. More
than that, kasprintf() will pass NULL @buf and @end as the parameters,
and @end - @buf can be negative in some case. Hence make it return at
the very beginning with false in these cases.

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

Link: https://lore.kernel.org/lkml/CAHk-=wimsMqGdzik187YWLb-ru+iktb4MYbMQG1rnZ81dXYFVg@mail.gmail.com/ [1]
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                            | 40 ++++++++++++++++++++---
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index f063a384c7c8..3c0d0f90b171 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..f4494129081f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -26,6 +26,7 @@
 #include <linux/types.h>
 #include <linux/string.h>
 #include <linux/ctype.h>
+#include <linux/dcache.h>
 #include <linux/kernel.h>
 #include <linux/kallsyms.h>
 #include <linux/math64.h>
@@ -920,13 +921,44 @@ 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)
 {
+	char *p;
+	const struct path *path;
+	int prepend_len, widen_len, 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;
+
+	widen_len = max_t(int, dpath_len, spec.field_width);
+
+	/* Case 1: Already started past the buffer. Just forward @buf. */
+	if (buf >= end)
+		return buf + widen_len;
+
+	/*
+	 * Case 2: The entire remaining space of the buffer filled by
+	 * the truncated path. Still need to get moved right when
+	 * the filled width is greather than the full path length.
+	 */
+	if (prepend_len < 0)
+		return widen_string(buf + dpath_len, dpath_len, end, 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
@@ -2296,7 +2328,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 +2427,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

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

[Andy Shevchenko]

On Wed, Jun 23, 2021 at 01:50:09PM +0800, 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 [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 the full path of that file.
> 
> If someone invokes snprintf() with small but positive space,
> prepend_name_with_len() moves or truncates the string partially. More
> than that, kasprintf() will pass NULL @buf and @end as the parameters,
> and @end - @buf can be negative in some case. Hence make it return at
> the very beginning with false in these cases.
> 
> Precision is never going to be used with %p (or any of its kernel
> extensions) if -Wformat is turned on.

...

> +	char *p;
> +	const struct path *path;
> +	int prepend_len, widen_len, dpath_len;

Reversed xmas tree order?

-- 
With Best Regards,
Andy Shevchenko

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

[Petr Mladek]

On Wed 2021-06-23 13:50:09, 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 [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 the full path of that file.
> 
> If someone invokes snprintf() with small but positive space,
> prepend_name_with_len() moves or truncates the string partially.

Does this comment belong to the 1st patch?
prepend_name_with_len() is not called in this patch.

> More
> than that, kasprintf() will pass NULL @buf and @end as the parameters,
> and @end - @buf can be negative in some case. Hence make it return at
> the very beginning with false in these cases.

Same here. file_d_path_name() does not return bool.

Well, please mention in the commit message that %pD uses the entire
given buffer as a scratch space. It might write something behind
the trailing '\0'.

It would make sense to warn about this also in
Documentation/core-api/printk-formats.rst. It is a bit non-standard
behavior.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index f0c35d9b65bf..f4494129081f 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -920,13 +921,44 @@ 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)
>  {
> +	char *p;
> +	const struct path *path;
> +	int prepend_len, widen_len, 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;
> +
> +	widen_len = max_t(int, dpath_len, spec.field_width);
> +
> +	/* Case 1: Already started past the buffer. Just forward @buf. */
> +	if (buf >= end)
> +		return buf + widen_len;
> +
> +	/*
> +	 * Case 2: The entire remaining space of the buffer filled by
> +	 * the truncated path. Still need to get moved right when
> +	 * the filled width is greather than the full path length.

s/filled/field/ ?

> +	 */
> +	if (prepend_len < 0)
> +		return widen_string(buf + dpath_len, dpath_len, end, spec);

Otherwise, it looks good to me.

Best Regards,
Petr

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

[Andy Shevchenko]

On Thu, Jun 24, 2021 at 11:01:31AM +0200, Petr Mladek wrote:
> On Wed 2021-06-23 13:50:09, Jia He wrote:

...

> > If someone invokes snprintf() with small but positive space,
> > prepend_name_with_len() moves or truncates the string partially.
> 
> Does this comment belong to the 1st patch?
> prepend_name_with_len() is not called in this patch.
> 
> > More
> > than that, kasprintf() will pass NULL @buf and @end as the parameters,
> > and @end - @buf can be negative in some case. Hence make it return at
> > the very beginning with false in these cases.
> 
> Same here. file_d_path_name() does not return bool.

It was my (apparently unclear) suggestion either to move it here, or be
rewritten in generic way as you suggested in the other thread.

-- 
With Best Regards,
Andy Shevchenko

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

[Justin He]

Hi Petr

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Thursday, June 24, 2021 5:02 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>; Christoph
> Hellwig <hch@infradead.org>; nd <nd@arm.com>
> Subject: Re: [PATCH v2 2/4] lib/vsprintf.c: make '%pD' print the full path
> of file
> 
> On Wed 2021-06-23 13:50:09, 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 [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 the full path of that file.
> >
> > If someone invokes snprintf() with small but positive space,
> > prepend_name_with_len() moves or truncates the string partially.
> 
> Does this comment belong to the 1st patch?
> prepend_name_with_len() is not called in this patch.
> 
Tend to remove this paragraph since the comments in do_path_unsafe (patch1/4)
was clear enough.

> > More
> > than that, kasprintf() will pass NULL @buf and @end as the parameters,
> > and @end - @buf can be negative in some case. Hence make it return at
> > the very beginning with false in these cases.
> 
> Same here. file_d_path_name() does not return bool.
> 
> Well, please mention in the commit message that %pD uses the entire
> given buffer as a scratch space. It might write something behind
> the trailing '\0'.
> 
> It would make sense to warn about this also in
> Documentation/core-api/printk-formats.rst. It is a bit non-standard
> behavior.
> 
Okay

> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index f0c35d9b65bf..f4494129081f 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -920,13 +921,44 @@ 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)
> >  {
> > +	char *p;
> > +	const struct path *path;
> > +	int prepend_len, widen_len, 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;
> > +
> > +	widen_len = max_t(int, dpath_len, spec.field_width);
> > +
> > +	/* Case 1: Already started past the buffer. Just forward @buf. */
> > +	if (buf >= end)
> > +		return buf + widen_len;
> > +
> > +	/*
> > +	 * Case 2: The entire remaining space of the buffer filled by
> > +	 * the truncated path. Still need to get moved right when
> > +	 * the filled width is greather than the full path length.
> 
> s/filled/field/ ?
> 
Ah, sorry, I changed it. I would have thought it as a typo 😊


--
Cheers,
Justin (Jia He)

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

[Justin He]

> -----Original Message-----
> From: Justin He
> Sent: Friday, June 25, 2021 10:30 AM
> To: 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; Matthew Wilcox <willy@infradead.org>; Christoph
> Hellwig <hch@infradead.org>; nd <nd@arm.com>
> Subject: RE: [PATCH v2 2/4] lib/vsprintf.c: make '%pD' print the full path
> of file
> 
> Hi Petr
> 
> > -----Original Message-----
> > From: Petr Mladek <pmladek@suse.com>
> > Sent: Thursday, June 24, 2021 5:02 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>; Christoph
> > Hellwig <hch@infradead.org>; nd <nd@arm.com>
> > Subject: Re: [PATCH v2 2/4] lib/vsprintf.c: make '%pD' print the full
> path
> > of file
> >
> > On Wed 2021-06-23 13:50:09, 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 [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 the full path of that file.
> > >
> > > If someone invokes snprintf() with small but positive space,
> > > prepend_name_with_len() moves or truncates the string partially.
> >
> > Does this comment belong to the 1st patch?
> > prepend_name_with_len() is not called in this patch.
> >
> Tend to remove this paragraph since the comments in do_path_unsafe
> (patch1/4)
> was clear enough.
Hi,
I noticed your suggested commit msg in another thread. Please ignore above
my words.

--
Cheers,
Justin (Jia He)

[PATCH v2 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>
Reviewed-by: Petr Mladek <pmladek@suse.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

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

[Andy Shevchenko]

On Wed, Jun 23, 2021 at 01:50:10PM +0800, 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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

-- 
With Best Regards,
Andy Shevchenko

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

[Jia He]

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

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

Parameterize the new using_scratch_space in __test(), do_test() and wrapper
macros to skip the test case mentioned above,

Signed-off-by: Jia He <justin.he@arm.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_printf.c | 49 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index d1d2f898ebae..f48da88bc77b 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>
 
@@ -37,8 +38,8 @@ static char *alloced_buffer __initdata;
 
 extern bool no_hash_pointers;
 
-static int __printf(4, 0) __init
-do_test(int bufsize, const char *expect, int elen,
+static int __printf(5, 0) __init
+do_test(int bufsize, const char *expect, int elen, bool using_scratch_space,
 	const char *fmt, va_list ap)
 {
 	va_list aq;
@@ -78,7 +79,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 (!using_scratch_space && 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;
@@ -97,8 +98,9 @@ do_test(int bufsize, const char *expect, int elen,
 	return 0;
 }
 
-static void __printf(3, 4) __init
-__test(const char *expect, int elen, const char *fmt, ...)
+static void __printf(4, 5) __init
+__test(const char *expect, int elen, bool using_scratch_space,
+	const char *fmt, ...)
 {
 	va_list ap;
 	int rand;
@@ -119,11 +121,11 @@ __test(const char *expect, int elen, const char *fmt, ...)
 	 * enough and 0), and then we also test that kvasprintf would
 	 * be able to print it as expected.
 	 */
-	failed_tests += do_test(BUF_SIZE, expect, elen, fmt, ap);
+	failed_tests += do_test(BUF_SIZE, expect, elen, using_scratch_space, fmt, ap);
 	rand = 1 + prandom_u32_max(elen+1);
 	/* Since elen < BUF_SIZE, we have 1 <= rand <= BUF_SIZE. */
-	failed_tests += do_test(rand, expect, elen, fmt, ap);
-	failed_tests += do_test(0, expect, elen, fmt, ap);
+	failed_tests += do_test(rand, expect, elen, using_scratch_space, fmt, ap);
+	failed_tests += do_test(0, expect, elen, using_scratch_space, fmt, ap);
 
 	p = kvasprintf(GFP_KERNEL, fmt, ap);
 	if (p) {
@@ -138,8 +140,15 @@ __test(const char *expect, int elen, const char *fmt, ...)
 	va_end(ap);
 }
 
+/*
+ * 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__)
+
 #define test(expect, fmt, ...)					\
-	__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
+	__test(expect, strlen(expect), false, fmt, ##__VA_ARGS__)
 
 static void __init
 test_basic(void)
@@ -150,7 +159,7 @@ test_basic(void)
 	test("", &nul);
 	test("100%", "100%%");
 	test("xxx%yyy", "xxx%cyyy", '%');
-	__test("xxx\0yyy", 7, "xxx%cyyy", '\0');
+	__test("xxx\0yyy", 7, false, "xxx%cyyy", '\0');
 }
 
 static void __init
@@ -501,6 +510,25 @@ 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);
+
+	test_using_scratch_space("/bravo/alfa   |/bravo/alfa   ", "%-14pD|%*pD", &test_file, -14, &test_file);
+	test_using_scratch_space("   /bravo/alfa|   /bravo/alfa", "%14pD|%*pD", &test_file, 14, &test_file);
+	test_using_scratch_space("   /bravo/alfa|/bravo/alfa   ", "%14pD|%-14pD", &test_file, &test_file);
+}
+
 static void __init
 struct_va_format(void)
 {
@@ -784,6 +812,7 @@ test_pointer(void)
 	ip();
 	uuid();
 	dentry();
+	f_d_path();
 	struct_va_format();
 	time_and_date();
 	struct_clk();
-- 
2.17.1

Re: [PATCH v2 0/4] make '%pD' print the full path of file

[Andy Shevchenko]

On Wed, Jun 23, 2021 at 01:50:07PM +0800, Jia He wrote:
> Background
> ==========
> Linus suggested printing the full path of 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_unsafe() 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. 
> 
> 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. kasprintf
> 
> TODO
> ====
> I plan to do the followup work after '%pD' behavior is changed.
> - s390/hmcdrv: remove the redundant directory path in printing string.
> - fs/iomap: simplify the iomap_swapfile_fail() with '%pD'.
> - simplify the string printing when file_path() is invoked(in some
>   cases, not all).
> - change the previous '%pD[2,3,4]' to '%pD'
>    
> Changelog
> =========
> v2:

Should be v6 now. So, next v7, otherwise you confuse bots and people.

My remark was for you for the future submission, this one is already spoiled.

> - refine the commit msg/comments (Andy)
> - pass the validator check by "make C=1 W=1"
> - add the R-b for patch 4/4 from Andy

-- 
With Best Regards,
Andy Shevchenko