| Message ID | 20210622140634.2436-2-justin.he@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | make '%pD' print the full path of file | expand |
On Tue, Jun 22, 2021 at 10:06:31PM +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 Missed period at the end of sentence. > 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. > If someone invokes snprintf() with small but positive space, > prepend_name_with_len() moves and copies the string partially. > > More than that, kasprintf() will pass NULL _buf_ and _end_ as the > parameters. Hence return at the very beginning with false in this case. These two paragraphs are talking about printf() interface, while patch has nothing to do with it. Please, rephrase in a way that it doesn't refer to the particular callers. Better to mention them in the corresponding printf() patch(es). ... > * prepend_name - prepend a pathname in front of current buffer pointer > - * @buffer: buffer pointer > - * @buflen: allocated length of the buffer > + * @p: prepend buffer which contains buffer pointer and allocated length > * @name: name string and length qstr structure Indentation issue btw, can be fixed in the same patch. > * > * With RCU path tracing, it may race with d_move(). Use READ_ONCE() to Shouldn't this be a separate change with corresponding Fixes tag? ... > +/** > + * d_path_unsafe - return the full path of a dentry without taking > + * any seqlock/spinlock. This helper is typical for debugging purposes. Seems you ignored my comment, or forget to test, or compile test with kernel doc validator enabled doesn't show any issues. If it's the latter, we have to fix kernel doc validator. TL;DR: describe parameters as well. > + */ ... > + struct path root; > + struct mount *mnt = real_mount(path->mnt); > + DECLARE_BUFFER(b, buf, buflen); Can wee keep this in reversed xmas tree order?
Hi Andy > -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Tuesday, June 22, 2021 10:37 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 v5 1/4] fs: introduce helper d_path_unsafe() > > On Tue, Jun 22, 2021 at 10:06:31PM +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 > > Missed period at the end of sentence. > Okay > > 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. > > > If someone invokes snprintf() with small but positive space, > > prepend_name_with_len() moves and copies the string partially. > > > > More than that, kasprintf() will pass NULL _buf_ and _end_ as the > > parameters. Hence return at the very beginning with false in this case. > > These two paragraphs are talking about printf() interface, while patch has > nothing to do with it. Please, rephrase in a way that it doesn't refer to > the > particular callers. Better to mention them in the corresponding printf() > patch(es). > Okay > ... > > > * prepend_name - prepend a pathname in front of current buffer pointer > > - * @buffer: buffer pointer > > - * @buflen: allocated length of the buffer > > + * @p: prepend buffer which contains buffer pointer and allocated length > > > * @name: name string and length qstr structure > > Indentation issue btw, can be fixed in the same patch. Okay > > > * > > * With RCU path tracing, it may race with d_move(). Use READ_ONCE() to > > Shouldn't this be a separate change with corresponding Fixes tag? Sorry, I don't quite understand here. What do you want to fix? > > ... > > > +/** > > + * d_path_unsafe - return the full path of a dentry without taking > > + * any seqlock/spinlock. This helper is typical for debugging purposes. > > Seems you ignored my comment, or forget to test, or compile test with > kernel > doc validator enabled doesn't show any issues. If it's the latter, we have > to > fix kernel doc validator. > > TL;DR: describe parameters as well. > My bad. Apologize for that. > > + */ > > ... > > > + struct path root; > > + struct mount *mnt = real_mount(path->mnt); > > + DECLARE_BUFFER(b, buf, buflen); > > Can wee keep this in reversed xmas tree order? Sure
On Wed, Jun 23, 2021 at 02:02:45AM +0000, Justin He wrote: > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Sent: Tuesday, June 22, 2021 10:37 PM > > On Tue, Jun 22, 2021 at 10:06:31PM +0800, Jia He wrote: ... > > > * prepend_name - prepend a pathname in front of current buffer pointer > > > - * @buffer: buffer pointer > > > - * @buflen: allocated length of the buffer > > > + * @p: prepend buffer which contains buffer pointer and allocated length > > > > > * @name: name string and length qstr structure > > > > Indentation issue btw, can be fixed in the same patch. > > Okay > > > > > * > > > * With RCU path tracing, it may race with d_move(). Use READ_ONCE() to > > > > Shouldn't this be a separate change with corresponding Fixes tag? > > Sorry, I don't quite understand here. > What do you want to fix? Kernel doc. The Fixes tag should correspond to the changes that missed the update of kernel doc.
Hi Andy > -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Wednesday, June 23, 2021 5:07 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 v5 1/4] fs: introduce helper d_path_unsafe() > > On Wed, Jun 23, 2021 at 02:02:45AM +0000, Justin He wrote: > > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Sent: Tuesday, June 22, 2021 10:37 PM > > > On Tue, Jun 22, 2021 at 10:06:31PM +0800, Jia He wrote: > > ... > > > > > * prepend_name - prepend a pathname in front of current buffer > pointer > > > > - * @buffer: buffer pointer > > > > - * @buflen: allocated length of the buffer > > > > + * @p: prepend buffer which contains buffer pointer and allocated > length > > > > > > > * @name: name string and length qstr structure > > > > > > Indentation issue btw, can be fixed in the same patch. > > > > Okay > > > > > > > * > > > > * With RCU path tracing, it may race with d_move(). Use READ_ONCE() > to > > > > > > Shouldn't this be a separate change with corresponding Fixes tag? > > > > Sorry, I don't quite understand here. > > What do you want to fix? > > Kernel doc. The Fixes tag should correspond to the changes that missed the > update of kernel doc. Ah, I got your point. Actually, this is originated from an unmerged patch [1] [1] https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/commit/?h=work.d_path&id=ad08ae586586ea9e2c0228a3d5a083500ea54202 I will ping Al Viro to fix this -- Cheers, Justin (Jia He)
On Tue 2021-06-22 17:36:39, Andy Shevchenko wrote: > On Tue, Jun 22, 2021 at 10:06:31PM +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 > > Missed period at the end of sentence. > > > 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. > > > If someone invokes snprintf() with small but positive space, > > prepend_name_with_len() moves and copies the string partially. > > > > More than that, kasprintf() will pass NULL _buf_ and _end_ as the > > parameters. Hence return at the very beginning with false in this case. > > These two paragraphs are talking about printf() interface, while patch has > nothing to do with it. Please, rephrase in a way that it doesn't refer to the > particular callers. Better to mention them in the corresponding printf() > patch(es). The two paragraphs are actually repeated in the 2nd patch. Unfortunately, they do not make sense there either because they comment code that is modified in this patch. We could describe it here a generic way. For example: prepend_name_with_len() moves and copies the path when the given buffer is not big enough. It cuts off the end of the path. It returns immediately when there is no buffer at all. Best Regards, Petr
On Thu, Jun 24, 2021 at 11:26:53AM +0200, Petr Mladek wrote: > On Tue 2021-06-22 17:36:39, Andy Shevchenko wrote: > > On Tue, Jun 22, 2021 at 10:06:31PM +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 > > > > Missed period at the end of sentence. > > > > > 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. > > > > > If someone invokes snprintf() with small but positive space, > > > prepend_name_with_len() moves and copies the string partially. > > > > > > More than that, kasprintf() will pass NULL _buf_ and _end_ as the > > > parameters. Hence return at the very beginning with false in this case. > > > > These two paragraphs are talking about printf() interface, while patch has > > nothing to do with it. Please, rephrase in a way that it doesn't refer to the > > particular callers. Better to mention them in the corresponding printf() > > patch(es). > > The two paragraphs are actually repeated in the 2nd > patch. Unfortunately, they do not make sense there either because they > comment code that is modified in this patch. > > We could describe it here a generic way. For example: > > prepend_name_with_len() moves and copies the path when the given > buffer is not big enough. It cuts off the end of the path. > It returns immediately when there is no buffer at all. Yes, that's my point, but sorry if I made it unclear.
diff --git a/fs/d_path.c b/fs/d_path.c index 23a53f7b5c71..84a738375698 100644 --- a/fs/d_path.c +++ b/fs/d_path.c @@ -33,8 +33,7 @@ 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 + * @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 @@ -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. + * + * With the original length of the buffer (p.ptr is changable), the dentry + * name string will be filled into the prepending buffer. Given the orginal + * length might be less than name string, the dentry name can be moved or + * truncated. + * + * 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; @@ -263,6 +336,29 @@ 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. + */ +char *d_path_unsafe(const struct path *path, char *buf, int buflen, + int *prepend_len) +{ + struct path root; + struct mount *mnt = real_mount(path->mnt); + DECLARE_BUFFER(b, buf, buflen); + + rcu_read_lock(); + get_fs_root_rcu(current->fs, &root); + + prepend(&b, "", 1); + __prepend_path(path->dentry, mnt, &root, &b); + rcu_read_unlock(); + + *prepend_len = b.len; + + return b.buf; +} + /* * Helper function for dentry_operations.d_dname() members */ diff --git a/include/linux/dcache.h b/include/linux/dcache.h index 9e23d33bb6f1..ec118b684055 100644 --- a/include/linux/dcache.h +++ b/include/linux/dcache.h @@ -301,6 +301,7 @@ char *dynamic_dname(struct dentry *, char *, int, const char *, ...); extern char *__d_path(const struct path *, const struct path *, char *, int); extern char *d_absolute_path(const struct path *, char *, int); extern char *d_path(const struct path *, char *, int); +extern char *d_path_unsafe(const struct path *, char *, int, int*); extern char *dentry_path_raw(const struct dentry *, char *, int); extern char *dentry_path(const struct dentry *, char *, int);
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. If someone invokes snprintf() with small but positive space, prepend_name_with_len() moves and copies the string partially. More than that, kasprintf() will pass NULL _buf_ and _end_ as the parameters. Hence return at the very beginning with false in this case. Suggested-by: Matthew Wilcox <willy@infradead.org> Signed-off-by: Jia He <justin.he@arm.com> --- fs/d_path.c | 104 +++++++++++++++++++++++++++++++++++++++-- include/linux/dcache.h | 1 + 2 files changed, 101 insertions(+), 4 deletions(-)