| Message ID | 20210623055011.22916-2-justin.he@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | make '%pD' print the full path of file | expand |
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.
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)
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)
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
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)
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)
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().
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);
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(-)