| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <AM6PR08MB43762FF7E76E4C7A0CD36314F7039@AM6PR08MB4376.eurprd08.prod.outlook.com>
Date: Mon, 28 Jun 2021 05:13:51 +0000
From: Justin He <Justin.He@....com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Petr Mladek <pmladek@...e.com>
CC: "Peter Zijlstra (Intel)" <peterz@...radead.org>,
Eric Biggers <ebiggers@...gle.com>,
"Ahmed S. Darwish" <a.darwish@...utronix.de>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
Matthew Wilcox <willy@...radead.org>,
Christoph Hellwig <hch@...radead.org>, nd <nd@....com>,
Justin He <Justin.He@....com>, Petr Mladek <pmladek@...e.com>,
Steven Rostedt <rostedt@...dmis.org>,
Sergey Senozhatsky <senozhatsky@...omium.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Rasmus Villemoes <linux@...musvillemoes.dk>,
Jonathan Corbet <corbet@....net>,
Alexander Viro <viro@...iv.linux.org.uk>,
Linus Torvalds <torvalds@...ux-foundation.org>
Subject: RE: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()
Hi Andy, Petr
> -----Original Message-----
> From: Jia He <justin.he@....com>
> Sent: Wednesday, June 23, 2021 1:50 PM
> To: Petr Mladek <pmladek@...e.com>; Steven Rostedt <rostedt@...dmis.org>;
> Sergey Senozhatsky <senozhatsky@...omium.org>; Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com>; Rasmus Villemoes
> <linux@...musvillemoes.dk>; Jonathan Corbet <corbet@....net>; Alexander
> Viro <viro@...iv.linux.org.uk>; Linus Torvalds <torvalds@...ux-
> foundation.org>
> Cc: Peter Zijlstra (Intel) <peterz@...radead.org>; Eric Biggers
> <ebiggers@...gle.com>; Ahmed S. Darwish <a.darwish@...utronix.de>; linux-
> doc@...r.kernel.org; linux-kernel@...r.kernel.org; linux-
> fsdevel@...r.kernel.org; Matthew Wilcox <willy@...radead.org>; Christoph
> Hellwig <hch@...radead.org>; nd <nd@....com>; Justin He <Justin.He@....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@...radead.org>
> Signed-off-by: Jia He <justin.he@....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)
Powered by blists - more mailing lists