lkml.org 
[lkml]   [2021]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRE: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()
Date
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)


\
 
 \ /
  Last update: 2021-06-28 07:14    [W:0.158 / U:0.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site