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

[Justin He <Justin.He@arm.com>]

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)