* [PATCH 02/14] d_path: saner calling conventions for __dentry_path()
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
@ 2021-05-19 0:48 ` Al Viro
2021-06-25 9:32 ` Justin He
2021-05-19 0:48 ` [PATCH 03/14] d_path: regularize handling of root dentry in __dentry_path() Al Viro
` (12 subsequent siblings)
13 siblings, 1 reply; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
1) lift NUL-termination into the callers
2) pass pointer to the end of buffer instead of that to beginning.
(1) allows to simplify dentry_path() - we don't need to play silly
games with restoring the leading / of "//deleted" after __dentry_path()
would've overwritten it with NUL.
We also do not need to check if (either) prepend() in there fails -
if the buffer is not large enough, we'll end with negative buflen
after prepend() and __dentry_path() will return the right value
(ERR_PTR(-ENAMETOOLONG)) just fine.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index 01df5dfa1f88..1a1cf05e7780 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -326,22 +326,21 @@ char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
/*
* Write full pathname from the root of the filesystem into the buffer.
*/
-static char *__dentry_path(const struct dentry *d, char *buf, int buflen)
+static char *__dentry_path(const struct dentry *d, char *p, int buflen)
{
const struct dentry *dentry;
char *end, *retval;
int len, seq = 0;
int error = 0;
- if (buflen < 2)
+ if (buflen < 1)
goto Elong;
rcu_read_lock();
restart:
dentry = d;
- end = buf + buflen;
+ end = p;
len = buflen;
- prepend(&end, &len, "", 1);
/* Get '/' right */
retval = end-1;
*retval = '/';
@@ -373,27 +372,21 @@ static char *__dentry_path(const struct dentry *d, char *buf, int buflen)
char *dentry_path_raw(const struct dentry *dentry, char *buf, int buflen)
{
- return __dentry_path(dentry, buf, buflen);
+ char *p = buf + buflen;
+ prepend(&p, &buflen, "", 1);
+ return __dentry_path(dentry, p, buflen);
}
EXPORT_SYMBOL(dentry_path_raw);
char *dentry_path(const struct dentry *dentry, char *buf, int buflen)
{
- char *p = NULL;
- char *retval;
-
- if (d_unlinked(dentry)) {
- p = buf + buflen;
- if (prepend(&p, &buflen, "//deleted", 10) != 0)
- goto Elong;
- buflen++;
- }
- retval = __dentry_path(dentry, buf, buflen);
- if (!IS_ERR(retval) && p)
- *p = '/'; /* restore '/' overriden with '\0' */
- return retval;
-Elong:
- return ERR_PTR(-ENAMETOOLONG);
+ char *p = buf + buflen;
+
+ if (unlikely(d_unlinked(dentry)))
+ prepend(&p, &buflen, "//deleted", 10);
+ else
+ prepend(&p, &buflen, "", 1);
+ return __dentry_path(dentry, p, buflen);
}
static void get_fs_root_and_pwd_rcu(struct fs_struct *fs, struct path *root,
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* RE: [PATCH 02/14] d_path: saner calling conventions for __dentry_path()
2021-05-19 0:48 ` [PATCH 02/14] d_path: saner calling conventions for __dentry_path() Al Viro
@ 2021-06-25 9:32 ` Justin He
2021-07-07 4:52 ` Justin He
0 siblings, 1 reply; 75+ messages in thread
From: Justin He @ 2021-06-25 9:32 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
Hi Al
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 02/14] d_path: saner calling conventions for __dentry_path()
>
> 1) lift NUL-termination into the callers
> 2) pass pointer to the end of buffer instead of that to beginning.
>
> (1) allows to simplify dentry_path() - we don't need to play silly
> games with restoring the leading / of "//deleted" after __dentry_path()
> would've overwritten it with NUL.
>
> We also do not need to check if (either) prepend() in there fails -
> if the buffer is not large enough, we'll end with negative buflen
> after prepend() and __dentry_path() will return the right value
> (ERR_PTR(-ENAMETOOLONG)) just fine.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> fs/d_path.c | 33 +++++++++++++--------------------
> 1 file changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/fs/d_path.c b/fs/d_path.c
> index 01df5dfa1f88..1a1cf05e7780 100644
> --- a/fs/d_path.c
> +++ b/fs/d_path.c
> @@ -326,22 +326,21 @@ char *simple_dname(struct dentry *dentry, char
> *buffer, int buflen)
> /*
> * Write full pathname from the root of the filesystem into the buffer.
> */
I suggest adding the comments to remind NUL terminator should be prepended
before invoking __dentry_path()
--
Cheers,
Justin (Jia He)
> -static char *__dentry_path(const struct dentry *d, char *buf, int buflen)
> +static char *__dentry_path(const struct dentry *d, char *p, int buflen)
> {
> const struct dentry *dentry;
> char *end, *retval;
> int len, seq = 0;
> int error = 0;
>
> - if (buflen < 2)
> + if (buflen < 1)
> goto Elong;
>
> rcu_read_lock();
> restart:
> dentry = d;
> - end = buf + buflen;
> + end = p;
> len = buflen;
> - prepend(&end, &len, "", 1);
> /* Get '/' right */
> retval = end-1;
> *retval = '/';
> @@ -373,27 +372,21 @@ static char *__dentry_path(const struct dentry *d,
> char *buf, int buflen)
>
> char *dentry_path_raw(const struct dentry *dentry, char *buf, int buflen)
> {
> - return __dentry_path(dentry, buf, buflen);
> + char *p = buf + buflen;
> + prepend(&p, &buflen, "", 1);
> + return __dentry_path(dentry, p, buflen);
> }
> EXPORT_SYMBOL(dentry_path_raw);
>
> char *dentry_path(const struct dentry *dentry, char *buf, int buflen)
> {
> - char *p = NULL;
> - char *retval;
> -
> - if (d_unlinked(dentry)) {
> - p = buf + buflen;
> - if (prepend(&p, &buflen, "//deleted", 10) != 0)
> - goto Elong;
> - buflen++;
> - }
> - retval = __dentry_path(dentry, buf, buflen);
> - if (!IS_ERR(retval) && p)
> - *p = '/'; /* restore '/' overriden with '\0' */
> - return retval;
> -Elong:
> - return ERR_PTR(-ENAMETOOLONG);
> + char *p = buf + buflen;
> +
> + if (unlikely(d_unlinked(dentry)))
> + prepend(&p, &buflen, "//deleted", 10);
> + else
> + prepend(&p, &buflen, "", 1);
> + return __dentry_path(dentry, p, buflen);
> }
>
> static void get_fs_root_and_pwd_rcu(struct fs_struct *fs, struct path
> *root,
> --
> 2.11.0
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 75+ messages in thread
* RE: [PATCH 02/14] d_path: saner calling conventions for __dentry_path()
2021-06-25 9:32 ` Justin He
@ 2021-07-07 4:52 ` Justin He
0 siblings, 0 replies; 75+ messages in thread
From: Justin He @ 2021-07-07 4:52 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel, nd
> -----Original Message-----
> From: Justin He
> Sent: Friday, June 25, 2021 5:33 PM
> To: Al Viro <viro@zeniv.linux.org.uk>; Linus Torvalds <torvalds@linux-
> foundation.org>
> Cc: 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: RE: [PATCH 02/14] d_path: saner calling conventions for
> __dentry_path()
>
> Hi Al
>
> > -----Original Message-----
> > From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> > Sent: Wednesday, May 19, 2021 8:49 AM
> > To: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> > Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>;
> Christian
> > Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> > <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> > Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> > Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> > <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> > doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> > fsdevel <linux-fsdevel@vger.kernel.org>
> > Subject: [PATCH 02/14] d_path: saner calling conventions for
> __dentry_path()
> >
> > 1) lift NUL-termination into the callers
> > 2) pass pointer to the end of buffer instead of that to beginning.
> >
> > (1) allows to simplify dentry_path() - we don't need to play silly
> > games with restoring the leading / of "//deleted" after __dentry_path()
> > would've overwritten it with NUL.
> >
> > We also do not need to check if (either) prepend() in there fails -
> > if the buffer is not large enough, we'll end with negative buflen
> > after prepend() and __dentry_path() will return the right value
> > (ERR_PTR(-ENAMETOOLONG)) just fine.
> >
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> > fs/d_path.c | 33 +++++++++++++--------------------
> > 1 file changed, 13 insertions(+), 20 deletions(-)
> >
> > diff --git a/fs/d_path.c b/fs/d_path.c
> > index 01df5dfa1f88..1a1cf05e7780 100644
> > --- a/fs/d_path.c
> > +++ b/fs/d_path.c
> > @@ -326,22 +326,21 @@ char *simple_dname(struct dentry *dentry, char
> > *buffer, int buflen)
> > /*
> > * Write full pathname from the root of the filesystem into the buffer.
> > */
> I suggest adding the comments to remind NUL terminator should be prepended
> before invoking __dentry_path()
>
Except for my suggestion about adding comments
Reviewed-by: Jia He <justin.he@arm.com>
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 03/14] d_path: regularize handling of root dentry in __dentry_path()
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
2021-05-19 0:48 ` [PATCH 02/14] d_path: saner calling conventions for __dentry_path() Al Viro
@ 2021-05-19 0:48 ` Al Viro
2021-07-07 4:50 ` Justin He
2021-05-19 0:48 ` [PATCH 04/14] d_path: get rid of path_with_deleted() Al Viro
` (11 subsequent siblings)
13 siblings, 1 reply; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
All path-forming primitives boil down to sequence of prepend_name()
on dentries encountered along the way toward root. Each time we prepend
/ + dentry name to the buffer. Normally that does exactly what we want,
but there's a corner case when we don't call prepend_name() at all (in case
of __dentry_path() that happens if we are given root dentry). We obviously
want to end up with "/", rather than "", so this corner case needs to be
handled.
__dentry_path() used to manually put '/' in the end of buffer before
doing anything else, to be overwritten by the first call of prepend_name()
if one happens and to be left in place if we don't call prepend_name() at
all. That required manually checking that we had space in the buffer
(prepend_name() and prepend() take care of such checks themselves) and lead
to clumsy keeping track of return value.
A better approach is to check if the main loop has added anything
into the buffer and prepend "/" if it hasn't. A side benefit of using prepend()
is that it does the right thing if we'd already run out of buffer, making
the overflow-handling logics simpler.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 21 +++++----------------
1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index 1a1cf05e7780..b3324ae7cfe2 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -329,31 +329,22 @@ char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
static char *__dentry_path(const struct dentry *d, char *p, int buflen)
{
const struct dentry *dentry;
- char *end, *retval;
+ char *end;
int len, seq = 0;
- int error = 0;
-
- if (buflen < 1)
- goto Elong;
rcu_read_lock();
restart:
dentry = d;
end = p;
len = buflen;
- /* Get '/' right */
- retval = end-1;
- *retval = '/';
read_seqbegin_or_lock(&rename_lock, &seq);
while (!IS_ROOT(dentry)) {
const struct dentry *parent = dentry->d_parent;
prefetch(parent);
- error = prepend_name(&end, &len, &dentry->d_name);
- if (error)
+ if (unlikely(prepend_name(&end, &len, &dentry->d_name) < 0))
break;
- retval = end;
dentry = parent;
}
if (!(seq & 1))
@@ -363,11 +354,9 @@ static char *__dentry_path(const struct dentry *d, char *p, int buflen)
goto restart;
}
done_seqretry(&rename_lock, seq);
- if (error)
- goto Elong;
- return retval;
-Elong:
- return ERR_PTR(-ENAMETOOLONG);
+ if (len == buflen)
+ prepend(&end, &len, "/", 1);
+ return len >= 0 ? end : ERR_PTR(-ENAMETOOLONG);
}
char *dentry_path_raw(const struct dentry *dentry, char *buf, int buflen)
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* RE: [PATCH 03/14] d_path: regularize handling of root dentry in __dentry_path()
2021-05-19 0:48 ` [PATCH 03/14] d_path: regularize handling of root dentry in __dentry_path() Al Viro
@ 2021-07-07 4:50 ` Justin He
0 siblings, 0 replies; 75+ messages in thread
From: Justin He @ 2021-07-07 4:50 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 03/14] d_path: regularize handling of root dentry in
> __dentry_path()
>
> All path-forming primitives boil down to sequence of prepend_name()
> on dentries encountered along the way toward root. Each time we prepend
> / + dentry name to the buffer. Normally that does exactly what we want,
> but there's a corner case when we don't call prepend_name() at all (in case
> of __dentry_path() that happens if we are given root dentry). We obviously
> want to end up with "/", rather than "", so this corner case needs to be
> handled.
>
> __dentry_path() used to manually put '/' in the end of buffer before
> doing anything else, to be overwritten by the first call of prepend_name()
> if one happens and to be left in place if we don't call prepend_name() at
> all. That required manually checking that we had space in the buffer
> (prepend_name() and prepend() take care of such checks themselves) and lead
> to clumsy keeping track of return value.
>
> A better approach is to check if the main loop has added anything
> into the buffer and prepend "/" if it hasn't. A side benefit of using
> prepend()
> is that it does the right thing if we'd already run out of buffer, making
> the overflow-handling logics simpler.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Reviewed-by: Jia He <justin.he@arm.com>
--
Cheers,
Justin (Jia He)
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 04/14] d_path: get rid of path_with_deleted()
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
2021-05-19 0:48 ` [PATCH 02/14] d_path: saner calling conventions for __dentry_path() Al Viro
2021-05-19 0:48 ` [PATCH 03/14] d_path: regularize handling of root dentry in __dentry_path() Al Viro
@ 2021-05-19 0:48 ` Al Viro
2021-05-19 0:48 ` [PATCH 05/14] getcwd(2): saner logics around prepend_path() call Al Viro
` (10 subsequent siblings)
13 siblings, 0 replies; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
expand in the sole caller; transform the initial prepends similar to
what we'd done in dentry_path() (prepend_path() will fail the right
way if we call it with negative buflen, same as __dentry_path() does).
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 23 +++++------------------
1 file changed, 5 insertions(+), 18 deletions(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index b3324ae7cfe2..7f3fac544bbb 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -211,23 +211,6 @@ char *d_absolute_path(const struct path *path,
return res;
}
-/*
- * same as __d_path but appends "(deleted)" for unlinked files.
- */
-static int path_with_deleted(const struct path *path,
- const struct path *root,
- char **buf, int *buflen)
-{
- prepend(buf, buflen, "", 1);
- if (d_unlinked(path->dentry)) {
- int error = prepend(buf, buflen, " (deleted)", 10);
- if (error)
- return error;
- }
-
- return prepend_path(path, root, buf, buflen);
-}
-
static int prepend_unreachable(char **buffer, int *buflen)
{
return prepend(buffer, buflen, "(unreachable)", 13);
@@ -282,7 +265,11 @@ char *d_path(const struct path *path, char *buf, int buflen)
rcu_read_lock();
get_fs_root_rcu(current->fs, &root);
- error = path_with_deleted(path, &root, &res, &buflen);
+ if (unlikely(d_unlinked(path->dentry)))
+ prepend(&res, &buflen, " (deleted)", 11);
+ else
+ prepend(&res, &buflen, "", 1);
+ error = prepend_path(path, &root, &res, &buflen);
rcu_read_unlock();
if (error < 0)
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCH 05/14] getcwd(2): saner logics around prepend_path() call
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
` (2 preceding siblings ...)
2021-05-19 0:48 ` [PATCH 04/14] d_path: get rid of path_with_deleted() Al Viro
@ 2021-05-19 0:48 ` Al Viro
2021-07-07 7:41 ` Justin He
2021-05-19 0:48 ` [PATCH 06/14] d_path: don't bother with return value of prepend() Al Viro
` (9 subsequent siblings)
13 siblings, 1 reply; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
The only negative value that might get returned by prepend_path() is
-ENAMETOOLONG, and that happens only on overflow. The same goes for
prepend_unreachable(). Overflow is detectable by observing negative
buflen, so we can simplify the control flow around the prepend_path()
call. Expand prepend_unreachable(), while we are at it - that's the
only caller.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index 7f3fac544bbb..311d43287572 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -211,11 +211,6 @@ char *d_absolute_path(const struct path *path,
return res;
}
-static int prepend_unreachable(char **buffer, int *buflen)
-{
- return prepend(buffer, buflen, "(unreachable)", 13);
-}
-
static void get_fs_root_rcu(struct fs_struct *fs, struct path *root)
{
unsigned seq;
@@ -414,17 +409,13 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned long, size)
int buflen = PATH_MAX;
prepend(&cwd, &buflen, "", 1);
- error = prepend_path(&pwd, &root, &cwd, &buflen);
+ if (prepend_path(&pwd, &root, &cwd, &buflen) > 0)
+ prepend(&cwd, &buflen, "(unreachable)", 13);
rcu_read_unlock();
- if (error < 0)
+ if (buflen < 0) {
+ error = -ENAMETOOLONG;
goto out;
-
- /* Unreachable from current root */
- if (error > 0) {
- error = prepend_unreachable(&cwd, &buflen);
- if (error)
- goto out;
}
error = -ERANGE;
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* RE: [PATCH 05/14] getcwd(2): saner logics around prepend_path() call
2021-05-19 0:48 ` [PATCH 05/14] getcwd(2): saner logics around prepend_path() call Al Viro
@ 2021-07-07 7:41 ` Justin He
0 siblings, 0 replies; 75+ messages in thread
From: Justin He @ 2021-07-07 7:41 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel, nd
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 05/14] getcwd(2): saner logics around prepend_path() call
>
> The only negative value that might get returned by prepend_path() is
> -ENAMETOOLONG, and that happens only on overflow. The same goes for
> prepend_unreachable(). Overflow is detectable by observing negative
> buflen, so we can simplify the control flow around the prepend_path()
> call. Expand prepend_unreachable(), while we are at it - that's the
> only caller.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Reviewed-by: Jia He <justin.he@arm.com>
--
Cheers,
Justin (Jia He)
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 06/14] d_path: don't bother with return value of prepend()
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
` (3 preceding siblings ...)
2021-05-19 0:48 ` [PATCH 05/14] getcwd(2): saner logics around prepend_path() call Al Viro
@ 2021-05-19 0:48 ` Al Viro
2021-06-24 6:13 ` Justin He
2021-05-19 0:48 ` [PATCH 07/14] d_path: lift -ENAMETOOLONG handling into callers of prepend_path() Al Viro
` (8 subsequent siblings)
13 siblings, 1 reply; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
Only simple_dname() checks it, and there we can simply do those
calls and check for overflow (by looking of negative buflen)
in the end.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index 311d43287572..72b8087aaf9c 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -8,14 +8,13 @@
#include <linux/prefetch.h>
#include "mount.h"
-static int prepend(char **buffer, int *buflen, const char *str, int namelen)
+static void prepend(char **buffer, int *buflen, const char *str, int namelen)
{
*buflen -= namelen;
- if (*buflen < 0)
- return -ENAMETOOLONG;
- *buffer -= namelen;
- memcpy(*buffer, str, namelen);
- return 0;
+ if (likely(*buflen >= 0)) {
+ *buffer -= namelen;
+ memcpy(*buffer, str, namelen);
+ }
}
/**
@@ -298,11 +297,10 @@ char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
{
char *end = buffer + buflen;
/* these dentries are never renamed, so d_lock is not needed */
- if (prepend(&end, &buflen, " (deleted)", 11) ||
- prepend(&end, &buflen, dentry->d_name.name, dentry->d_name.len) ||
- prepend(&end, &buflen, "/", 1))
- end = ERR_PTR(-ENAMETOOLONG);
- return end;
+ prepend(&end, &buflen, " (deleted)", 11);
+ prepend(&end, &buflen, dentry->d_name.name, dentry->d_name.len);
+ prepend(&end, &buflen, "/", 1);
+ return buflen >= 0 ? end : ERR_PTR(-ENAMETOOLONG);
}
/*
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* RE: [PATCH 06/14] d_path: don't bother with return value of prepend()
2021-05-19 0:48 ` [PATCH 06/14] d_path: don't bother with return value of prepend() Al Viro
@ 2021-06-24 6:13 ` Justin He
0 siblings, 0 replies; 75+ messages in thread
From: Justin He @ 2021-06-24 6:13 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 06/14] d_path: don't bother with return value of prepend()
>
> Only simple_dname() checks it, and there we can simply do those
> calls and check for overflow (by looking of negative buflen)
> in the end.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Reviewed-by: Jia He <justin.he@arm.com>
--
Cheers,
Justin (Jia He)
> ---
> fs/d_path.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/fs/d_path.c b/fs/d_path.c
> index 311d43287572..72b8087aaf9c 100644
> --- a/fs/d_path.c
> +++ b/fs/d_path.c
> @@ -8,14 +8,13 @@
> #include <linux/prefetch.h>
> #include "mount.h"
>
> -static int prepend(char **buffer, int *buflen, const char *str, int
> namelen)
> +static void prepend(char **buffer, int *buflen, const char *str, int
> namelen)
> {
> *buflen -= namelen;
> - if (*buflen < 0)
> - return -ENAMETOOLONG;
> - *buffer -= namelen;
> - memcpy(*buffer, str, namelen);
> - return 0;
> + if (likely(*buflen >= 0)) {
> + *buffer -= namelen;
> + memcpy(*buffer, str, namelen);
> + }
> }
>
> /**
> @@ -298,11 +297,10 @@ char *simple_dname(struct dentry *dentry, char
> *buffer, int buflen)
> {
> char *end = buffer + buflen;
> /* these dentries are never renamed, so d_lock is not needed */
> - if (prepend(&end, &buflen, " (deleted)", 11) ||
> - prepend(&end, &buflen, dentry->d_name.name, dentry->d_name.len)
> ||
> - prepend(&end, &buflen, "/", 1))
> - end = ERR_PTR(-ENAMETOOLONG);
> - return end;
> + prepend(&end, &buflen, " (deleted)", 11);
> + prepend(&end, &buflen, dentry->d_name.name, dentry->d_name.len);
> + prepend(&end, &buflen, "/", 1);
> + return buflen >= 0 ? end : ERR_PTR(-ENAMETOOLONG);
> }
>
> /*
> --
> 2.11.0
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 07/14] d_path: lift -ENAMETOOLONG handling into callers of prepend_path()
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
` (4 preceding siblings ...)
2021-05-19 0:48 ` [PATCH 06/14] d_path: don't bother with return value of prepend() Al Viro
@ 2021-05-19 0:48 ` Al Viro
2021-06-25 9:18 ` Justin He
2021-05-19 0:48 ` [PATCH 08/14] d_path: make prepend_name() boolean Al Viro
` (7 subsequent siblings)
13 siblings, 1 reply; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
The only negative value ever returned by prepend_path() is -ENAMETOOLONG
and callers can recognize that situation (overflow) by looking at the
sign of buflen. Lift that into the callers; we already have the
same logics (buf if buflen is non-negative, ERR_PTR(-ENAMETOOLONG) otherwise)
in several places and that'll become a new primitive several commits down
the road.
Make prepend_path() return 0 instead of -ENAMETOOLONG. That makes for
saner calling conventions (0/1/2/3/-ENAMETOOLONG is obnoxious) and
callers actually get simpler, especially once the aforementioned
primitive gets added.
In prepend_path() itself we switch prepending the / (in case of
empty path) to use of prepend() - no need to open-code that, compiler
will do the right thing. It's exactly the same logics as in
__dentry_path().
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 39 +++++++++++----------------------------
1 file changed, 11 insertions(+), 28 deletions(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index 72b8087aaf9c..327cc3744554 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -127,8 +127,7 @@ static int prepend_path(const struct path *path,
}
parent = dentry->d_parent;
prefetch(parent);
- error = prepend_name(&bptr, &blen, &dentry->d_name);
- if (error)
+ if (unlikely(prepend_name(&bptr, &blen, &dentry->d_name) < 0))
break;
dentry = parent;
@@ -149,12 +148,9 @@ static int prepend_path(const struct path *path,
}
done_seqretry(&mount_lock, m_seq);
- if (error >= 0 && bptr == *buffer) {
- if (--blen < 0)
- error = -ENAMETOOLONG;
- else
- *--bptr = '/';
- }
+ if (blen == *buflen)
+ prepend(&bptr, &blen, "/", 1);
+
*buffer = bptr;
*buflen = blen;
return error;
@@ -181,16 +177,11 @@ char *__d_path(const struct path *path,
char *buf, int buflen)
{
char *res = buf + buflen;
- int error;
prepend(&res, &buflen, "", 1);
- error = prepend_path(path, root, &res, &buflen);
-
- if (error < 0)
- return ERR_PTR(error);
- if (error > 0)
+ if (prepend_path(path, root, &res, &buflen) > 0)
return NULL;
- return res;
+ return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
}
char *d_absolute_path(const struct path *path,
@@ -198,16 +189,11 @@ char *d_absolute_path(const struct path *path,
{
struct path root = {};
char *res = buf + buflen;
- int error;
prepend(&res, &buflen, "", 1);
- error = prepend_path(path, &root, &res, &buflen);
-
- if (error > 1)
- error = -EINVAL;
- if (error < 0)
- return ERR_PTR(error);
- return res;
+ if (prepend_path(path, &root, &res, &buflen) > 1)
+ return ERR_PTR(-EINVAL);
+ return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
}
static void get_fs_root_rcu(struct fs_struct *fs, struct path *root)
@@ -240,7 +226,6 @@ char *d_path(const struct path *path, char *buf, int buflen)
{
char *res = buf + buflen;
struct path root;
- int error;
/*
* We have various synthetic filesystems that never get mounted. On
@@ -263,12 +248,10 @@ char *d_path(const struct path *path, char *buf, int buflen)
prepend(&res, &buflen, " (deleted)", 11);
else
prepend(&res, &buflen, "", 1);
- error = prepend_path(path, &root, &res, &buflen);
+ prepend_path(path, &root, &res, &buflen);
rcu_read_unlock();
- if (error < 0)
- res = ERR_PTR(error);
- return res;
+ return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
}
EXPORT_SYMBOL(d_path);
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* RE: [PATCH 07/14] d_path: lift -ENAMETOOLONG handling into callers of prepend_path()
2021-05-19 0:48 ` [PATCH 07/14] d_path: lift -ENAMETOOLONG handling into callers of prepend_path() Al Viro
@ 2021-06-25 9:18 ` Justin He
2021-06-28 5:20 ` Justin He
0 siblings, 1 reply; 75+ messages in thread
From: Justin He @ 2021-06-25 9:18 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
Hi Al
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 07/14] d_path: lift -ENAMETOOLONG handling into callers of
> prepend_path()
>
> The only negative value ever returned by prepend_path() is -ENAMETOOLONG
> and callers can recognize that situation (overflow) by looking at the
> sign of buflen. Lift that into the callers; we already have the
> same logics (buf if buflen is non-negative, ERR_PTR(-ENAMETOOLONG)
> otherwise)
> in several places and that'll become a new primitive several commits down
> the road.
>
> Make prepend_path() return 0 instead of -ENAMETOOLONG. That makes for
> saner calling conventions (0/1/2/3/-ENAMETOOLONG is obnoxious) and
> callers actually get simpler, especially once the aforementioned
> primitive gets added.
>
> In prepend_path() itself we switch prepending the / (in case of
> empty path) to use of prepend() - no need to open-code that, compiler
> will do the right thing. It's exactly the same logics as in
> __dentry_path().
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> fs/d_path.c | 39 +++++++++++----------------------------
> 1 file changed, 11 insertions(+), 28 deletions(-)
>
> diff --git a/fs/d_path.c b/fs/d_path.c
> index 72b8087aaf9c..327cc3744554 100644
> --- a/fs/d_path.c
> +++ b/fs/d_path.c
> @@ -127,8 +127,7 @@ static int prepend_path(const struct path *path,
> }
> parent = dentry->d_parent;
> prefetch(parent);
> - error = prepend_name(&bptr, &blen, &dentry->d_name);
> - if (error)
> + if (unlikely(prepend_name(&bptr, &blen, &dentry->d_name) < 0))
> break;
>
> dentry = parent;
> @@ -149,12 +148,9 @@ static int prepend_path(const struct path *path,
> }
> done_seqretry(&mount_lock, m_seq);
>
> - if (error >= 0 && bptr == *buffer) {
> - if (--blen < 0)
> - error = -ENAMETOOLONG;
> - else
> - *--bptr = '/';
> - }
> + if (blen == *buflen)
> + prepend(&bptr, &blen, "/", 1);
> +
> *buffer = bptr;
> *buflen = blen;
> return error;
> @@ -181,16 +177,11 @@ char *__d_path(const struct path *path,
> char *buf, int buflen)
> {
> char *res = buf + buflen;
> - int error;
>
> prepend(&res, &buflen, "", 1);
> - error = prepend_path(path, root, &res, &buflen);
> -
> - if (error < 0)
> - return ERR_PTR(error);
> - if (error > 0)
> + if (prepend_path(path, root, &res, &buflen) > 0)
> return NULL;
> - return res;
> + return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
> }
>
> char *d_absolute_path(const struct path *path,
> @@ -198,16 +189,11 @@ char *d_absolute_path(const struct path *path,
> {
> struct path root = {};
> char *res = buf + buflen;
> - int error;
>
> prepend(&res, &buflen, "", 1);
> - error = prepend_path(path, &root, &res, &buflen);
> -
> - if (error > 1)
> - error = -EINVAL;
> - if (error < 0)
> - return ERR_PTR(error);
> - return res;
> + if (prepend_path(path, &root, &res, &buflen) > 1)
> + return ERR_PTR(-EINVAL);
> + return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
This patch is *correct*.
But do you mind changing like:
if (buflen >= 0 || error == 1)
return res;
else
return ERR_PTR(-ENAMETOOLONG);
The reason why I comment here is that I will change the
prepend_name in __prepend_path to prepend_name_with_len.
The latter will go through all the dentries recursively instead
of returning false if p.len<0.
So (error == 1 && buflen < 0) is possible.
If you disagree, I will change it later in another single patch.
--
Cheers,
Justin (Jia He)
> }
>
> static void get_fs_root_rcu(struct fs_struct *fs, struct path *root)
> @@ -240,7 +226,6 @@ char *d_path(const struct path *path, char *buf, int
> buflen)
> {
> char *res = buf + buflen;
> struct path root;
> - int error;
>
> /*
> * We have various synthetic filesystems that never get mounted. On
> @@ -263,12 +248,10 @@ char *d_path(const struct path *path, char *buf, int
> buflen)
> prepend(&res, &buflen, " (deleted)", 11);
> else
> prepend(&res, &buflen, "", 1);
> - error = prepend_path(path, &root, &res, &buflen);
> + prepend_path(path, &root, &res, &buflen);
> rcu_read_unlock();
>
> - if (error < 0)
> - res = ERR_PTR(error);
> - return res;
> + return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
> }
> EXPORT_SYMBOL(d_path);
>
> --
> 2.11.0
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 75+ messages in thread
* RE: [PATCH 07/14] d_path: lift -ENAMETOOLONG handling into callers of prepend_path()
2021-06-25 9:18 ` Justin He
@ 2021-06-28 5:20 ` Justin He
0 siblings, 0 replies; 75+ messages in thread
From: Justin He @ 2021-06-28 5:20 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel, nd
> -----Original Message-----
> From: Justin He
> Sent: Friday, June 25, 2021 5:18 PM
> To: Al Viro <viro@zeniv.linux.org.uk>; Linus Torvalds <torvalds@linux-
> foundation.org>
> Cc: 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: RE: [PATCH 07/14] d_path: lift -ENAMETOOLONG handling into callers
> of prepend_path()
>
> Hi Al
>
> > -----Original Message-----
> > From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> > Sent: Wednesday, May 19, 2021 8:49 AM
> > To: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> > Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>;
> Christian
> > Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> > <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> > Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> > Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> > <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> > doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> > fsdevel <linux-fsdevel@vger.kernel.org>
> > Subject: [PATCH 07/14] d_path: lift -ENAMETOOLONG handling into callers
> of
> > prepend_path()
> >
> > The only negative value ever returned by prepend_path() is -ENAMETOOLONG
> > and callers can recognize that situation (overflow) by looking at the
> > sign of buflen. Lift that into the callers; we already have the
> > same logics (buf if buflen is non-negative, ERR_PTR(-ENAMETOOLONG)
> > otherwise)
> > in several places and that'll become a new primitive several commits down
> > the road.
> >
> > Make prepend_path() return 0 instead of -ENAMETOOLONG. That makes for
> > saner calling conventions (0/1/2/3/-ENAMETOOLONG is obnoxious) and
> > callers actually get simpler, especially once the aforementioned
> > primitive gets added.
> >
> > In prepend_path() itself we switch prepending the / (in case of
> > empty path) to use of prepend() - no need to open-code that, compiler
> > will do the right thing. It's exactly the same logics as in
> > __dentry_path().
> >
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> > fs/d_path.c | 39 +++++++++++----------------------------
> > 1 file changed, 11 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/d_path.c b/fs/d_path.c
> > index 72b8087aaf9c..327cc3744554 100644
> > --- a/fs/d_path.c
> > +++ b/fs/d_path.c
> > @@ -127,8 +127,7 @@ static int prepend_path(const struct path *path,
> > }
> > parent = dentry->d_parent;
> > prefetch(parent);
> > - error = prepend_name(&bptr, &blen, &dentry->d_name);
> > - if (error)
> > + if (unlikely(prepend_name(&bptr, &blen, &dentry->d_name) < 0))
> > break;
> >
> > dentry = parent;
> > @@ -149,12 +148,9 @@ static int prepend_path(const struct path *path,
> > }
> > done_seqretry(&mount_lock, m_seq);
> >
> > - if (error >= 0 && bptr == *buffer) {
> > - if (--blen < 0)
> > - error = -ENAMETOOLONG;
> > - else
> > - *--bptr = '/';
> > - }
> > + if (blen == *buflen)
> > + prepend(&bptr, &blen, "/", 1);
> > +
> > *buffer = bptr;
> > *buflen = blen;
> > return error;
> > @@ -181,16 +177,11 @@ char *__d_path(const struct path *path,
> > char *buf, int buflen)
> > {
> > char *res = buf + buflen;
> > - int error;
> >
> > prepend(&res, &buflen, "", 1);
> > - error = prepend_path(path, root, &res, &buflen);
> > -
> > - if (error < 0)
> > - return ERR_PTR(error);
> > - if (error > 0)
> > + if (prepend_path(path, root, &res, &buflen) > 0)
> > return NULL;
> > - return res;
> > + return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
> > }
> >
> > char *d_absolute_path(const struct path *path,
> > @@ -198,16 +189,11 @@ char *d_absolute_path(const struct path *path,
> > {
> > struct path root = {};
> > char *res = buf + buflen;
> > - int error;
> >
> > prepend(&res, &buflen, "", 1);
> > - error = prepend_path(path, &root, &res, &buflen);
> > -
> > - if (error > 1)
> > - error = -EINVAL;
> > - if (error < 0)
> > - return ERR_PTR(error);
> > - return res;
> > + if (prepend_path(path, &root, &res, &buflen) > 1)
> > + return ERR_PTR(-EINVAL);
> > + return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
>
> This patch is *correct*.
> But do you mind changing like:
> if (buflen >= 0 || error == 1)
> return res;
> else
> return ERR_PTR(-ENAMETOOLONG);
>
> The reason why I comment here is that I will change the
> prepend_name in __prepend_path to prepend_name_with_len.
> The latter will go through all the dentries recursively instead
> of returning false if p.len<0.
> So (error == 1 && buflen < 0) is possible.
Please ignore it, this is not relevant to this patch itself, I can
draft a new patch if it is needed.
Hence:
Reviewed-by: Jia He <justin.he@arm.com>
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 08/14] d_path: make prepend_name() boolean
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
` (5 preceding siblings ...)
2021-05-19 0:48 ` [PATCH 07/14] d_path: lift -ENAMETOOLONG handling into callers of prepend_path() Al Viro
@ 2021-05-19 0:48 ` Al Viro
2021-05-20 9:12 ` Justin He
2021-07-07 7:43 ` Justin He
2021-05-19 0:48 ` [PATCH 09/14] d_path: introduce struct prepend_buffer Al Viro
` (6 subsequent siblings)
13 siblings, 2 replies; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
It returns only 0 or -ENAMETOOLONG and both callers only check if
the result is negative. Might as well return true on success and
false on failure...
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index 327cc3744554..83db83446afd 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -34,15 +34,15 @@ static void prepend(char **buffer, int *buflen, const char *str, int namelen)
*
* Load acquire is needed to make sure that we see that terminating NUL.
*/
-static int prepend_name(char **buffer, int *buflen, const struct qstr *name)
+static bool prepend_name(char **buffer, int *buflen, const struct qstr *name)
{
const char *dname = smp_load_acquire(&name->name); /* ^^^ */
u32 dlen = READ_ONCE(name->len);
char *p;
*buflen -= dlen + 1;
- if (*buflen < 0)
- return -ENAMETOOLONG;
+ if (unlikely(*buflen < 0))
+ return false;
p = *buffer -= dlen + 1;
*p++ = '/';
while (dlen--) {
@@ -51,7 +51,7 @@ static int prepend_name(char **buffer, int *buflen, const struct qstr *name)
break;
*p++ = c;
}
- return 0;
+ return true;
}
/**
@@ -127,7 +127,7 @@ static int prepend_path(const struct path *path,
}
parent = dentry->d_parent;
prefetch(parent);
- if (unlikely(prepend_name(&bptr, &blen, &dentry->d_name) < 0))
+ if (!prepend_name(&bptr, &blen, &dentry->d_name))
break;
dentry = parent;
@@ -305,7 +305,7 @@ static char *__dentry_path(const struct dentry *d, char *p, int buflen)
const struct dentry *parent = dentry->d_parent;
prefetch(parent);
- if (unlikely(prepend_name(&end, &len, &dentry->d_name) < 0))
+ if (!prepend_name(&end, &len, &dentry->d_name))
break;
dentry = parent;
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* RE: [PATCH 08/14] d_path: make prepend_name() boolean
2021-05-19 0:48 ` [PATCH 08/14] d_path: make prepend_name() boolean Al Viro
@ 2021-05-20 9:12 ` Justin He
2021-05-20 9:19 ` Andy Shevchenko
2021-05-20 14:53 ` Petr Mladek
2021-07-07 7:43 ` Justin He
1 sibling, 2 replies; 75+ messages in thread
From: Justin He @ 2021-05-20 9:12 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
Hi Al
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 08/14] d_path: make prepend_name() boolean
>
> It returns only 0 or -ENAMETOOLONG and both callers only check if
> the result is negative. Might as well return true on success and
> false on failure...
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> fs/d_path.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/d_path.c b/fs/d_path.c
> index 327cc3744554..83db83446afd 100644
> --- a/fs/d_path.c
> +++ b/fs/d_path.c
> @@ -34,15 +34,15 @@ static void prepend(char **buffer, int *buflen, const
> char *str, int namelen)
> *
> * Load acquire is needed to make sure that we see that terminating NUL.
> */
> -static int prepend_name(char **buffer, int *buflen, const struct qstr
> *name)
> +static bool prepend_name(char **buffer, int *buflen, const struct qstr
> *name)
> {
> const char *dname = smp_load_acquire(&name->name); /* ^^^ */
> u32 dlen = READ_ONCE(name->len);
> char *p;
>
> *buflen -= dlen + 1;
> - if (*buflen < 0)
> - return -ENAMETOOLONG;
> + if (unlikely(*buflen < 0))
> + return false;
I don't object to this patch itself.
Just wonder whether we need to relax the check condition of "*buflen < 0" ?
Given that in vsnprintf code path, sometimes the *buflen is < 0.
Please see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/vsprintf.c#n2698
--
Cheers,
Justin (Jia He)
> p = *buffer -= dlen + 1;
> *p++ = '/';
> while (dlen--) {
> @@ -51,7 +51,7 @@ static int prepend_name(char **buffer, int *buflen, const
> struct qstr *name)
> break;
> *p++ = c;
> }
> - return 0;
> + return true;
> }
>
> /**
> @@ -127,7 +127,7 @@ static int prepend_path(const struct path *path,
> }
> parent = dentry->d_parent;
> prefetch(parent);
> - if (unlikely(prepend_name(&bptr, &blen, &dentry->d_name) < 0))
> + if (!prepend_name(&bptr, &blen, &dentry->d_name))
> break;
>
> dentry = parent;
> @@ -305,7 +305,7 @@ static char *__dentry_path(const struct dentry *d, char
> *p, int buflen)
> const struct dentry *parent = dentry->d_parent;
>
> prefetch(parent);
> - if (unlikely(prepend_name(&end, &len, &dentry->d_name) < 0))
> + if (!prepend_name(&end, &len, &dentry->d_name))
> break;
>
> dentry = parent;
> --
> 2.11.0
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 08/14] d_path: make prepend_name() boolean
2021-05-20 9:12 ` Justin He
@ 2021-05-20 9:19 ` Andy Shevchenko
2021-05-20 14:53 ` Petr Mladek
1 sibling, 0 replies; 75+ messages in thread
From: Andy Shevchenko @ 2021-05-20 9:19 UTC (permalink / raw)
To: Justin He
Cc: Al Viro, Linus Torvalds, Petr Mladek, Steven Rostedt,
Sergey Senozhatsky, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
On Thu, May 20, 2021 at 09:12:34AM +0000, Justin He wrote:
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Fix it.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 08/14] d_path: make prepend_name() boolean
2021-05-20 9:12 ` Justin He
2021-05-20 9:19 ` Andy Shevchenko
@ 2021-05-20 14:53 ` Petr Mladek
2021-05-20 19:35 ` Al Viro
1 sibling, 1 reply; 75+ messages in thread
From: Petr Mladek @ 2021-05-20 14:53 UTC (permalink / raw)
To: Justin He
Cc: Al Viro, Linus Torvalds, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
On Thu 2021-05-20 09:12:34, Justin He wrote:
> Hi Al
>
> > -----Original Message-----
> > From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> > Sent: Wednesday, May 19, 2021 8:49 AM
> > To: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> > Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> > Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> > <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> > Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> > Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> > <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> > doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> > fsdevel <linux-fsdevel@vger.kernel.org>
> > Subject: [PATCH 08/14] d_path: make prepend_name() boolean
> >
> > It returns only 0 or -ENAMETOOLONG and both callers only check if
> > the result is negative. Might as well return true on success and
> > false on failure...
> >
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> > fs/d_path.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/d_path.c b/fs/d_path.c
> > index 327cc3744554..83db83446afd 100644
> > --- a/fs/d_path.c
> > +++ b/fs/d_path.c
> > @@ -34,15 +34,15 @@ static void prepend(char **buffer, int *buflen, const
> > char *str, int namelen)
> > *
> > * Load acquire is needed to make sure that we see that terminating NUL.
> > */
> > -static int prepend_name(char **buffer, int *buflen, const struct qstr
> > *name)
> > +static bool prepend_name(char **buffer, int *buflen, const struct qstr
> > *name)
> > {
> > const char *dname = smp_load_acquire(&name->name); /* ^^^ */
> > u32 dlen = READ_ONCE(name->len);
> > char *p;
> >
> > *buflen -= dlen + 1;
> > - if (*buflen < 0)
> > - return -ENAMETOOLONG;
> > + if (unlikely(*buflen < 0))
> > + return false;
>
> I don't object to this patch itself.
> Just wonder whether we need to relax the check condition of "*buflen < 0" ?
>
> Given that in vsnprintf code path, sometimes the *buflen is < 0.
>
> Please see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/vsprintf.c#n2698
IMHO, the patch is fine. It is likely some misunderstanding.
The above link points to:
2693 str = buf;
2694 end = buf + size;
2695
2696 /* Make sure end is always >= buf */
2697 if (end < buf) {
2698 end = ((void *)-1);
2699 size = end - buf;
2700 }
"end" points right behind the end of the buffer. It is later
used instead of the buffer size. The above code handles a potential
overflow of "buf + size". I causes that "end" will be 0xffffffff
in case of the overflow.
That said. vsnprintf() returns the number of characters which would
be generated for the given input. But only the "size" is written.
This require copying the characters one by one.
It is useful to see how many characters were lost. But I am not sure
if this ever worked for the dentry functions.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 08/14] d_path: make prepend_name() boolean
2021-05-20 14:53 ` Petr Mladek
@ 2021-05-20 19:35 ` Al Viro
0 siblings, 0 replies; 75+ messages in thread
From: Al Viro @ 2021-05-20 19:35 UTC (permalink / raw)
To: Petr Mladek
Cc: Justin He, Linus Torvalds, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
On Thu, May 20, 2021 at 04:53:38PM +0200, Petr Mladek wrote:
> That said. vsnprintf() returns the number of characters which would
> be generated for the given input. But only the "size" is written.
> This require copying the characters one by one.
Not really - that's more of avoiding trouble if the sucker gets renamed
right under prepend_name(). Note that we have no way to guarantee that
length and string pointer come from the same moment. This looking for
NUL is about protection againts stepping off the end of allocated object.
^ permalink raw reply [flat|nested] 75+ messages in thread
* RE: [PATCH 08/14] d_path: make prepend_name() boolean
2021-05-19 0:48 ` [PATCH 08/14] d_path: make prepend_name() boolean Al Viro
2021-05-20 9:12 ` Justin He
@ 2021-07-07 7:43 ` Justin He
1 sibling, 0 replies; 75+ messages in thread
From: Justin He @ 2021-07-07 7:43 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel, nd
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 08/14] d_path: make prepend_name() boolean
>
> It returns only 0 or -ENAMETOOLONG and both callers only check if
> the result is negative. Might as well return true on success and
> false on failure...
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
Reviewed-by: Jia He <justin.he@arm.com>
--
Cheers,
Justin (Jia He)
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 09/14] d_path: introduce struct prepend_buffer
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
` (6 preceding siblings ...)
2021-05-19 0:48 ` [PATCH 08/14] d_path: make prepend_name() boolean Al Viro
@ 2021-05-19 0:48 ` Al Viro
2021-06-23 13:28 ` Justin He
2021-05-19 0:48 ` [PATCH 10/14] d_path: prepend_path(): get rid of vfsmnt Al Viro
` (5 subsequent siblings)
13 siblings, 1 reply; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
We've a lot of places where we have pairs of form (pointer to end
of buffer, amount of space left in front of that). These sit in pairs of
variables located next to each other and usually passed by reference.
Turn those into instances of new type (struct prepend_buffer) and pass
reference to the pair instead of pairs of references to its fields.
Declared and initialized by DECLARE_BUFFER(name, buf, buflen).
extract_string(prepend_buffer) returns the buffer contents if
no overflow has happened, ERR_PTR(ENAMETOOLONG) otherwise.
All places where we used to have that boilerplate converted to use
of that helper.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 142 ++++++++++++++++++++++++++++++++----------------------------
1 file changed, 75 insertions(+), 67 deletions(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index 83db83446afd..06e93dd031bf 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -8,12 +8,26 @@
#include <linux/prefetch.h>
#include "mount.h"
-static void prepend(char **buffer, int *buflen, const char *str, int namelen)
+struct prepend_buffer {
+ char *buf;
+ int len;
+};
+#define DECLARE_BUFFER(__name, __buf, __len) \
+ struct prepend_buffer __name = {.buf = __buf + __len, .len = __len}
+
+static char *extract_string(struct prepend_buffer *p)
{
- *buflen -= namelen;
- if (likely(*buflen >= 0)) {
- *buffer -= namelen;
- memcpy(*buffer, str, namelen);
+ if (likely(p->len >= 0))
+ return p->buf;
+ return ERR_PTR(-ENAMETOOLONG);
+}
+
+static void prepend(struct prepend_buffer *p, const char *str, int namelen)
+{
+ p->len -= namelen;
+ if (likely(p->len >= 0)) {
+ p->buf -= namelen;
+ memcpy(p->buf, str, namelen);
}
}
@@ -34,22 +48,22 @@ static void prepend(char **buffer, int *buflen, const char *str, int namelen)
*
* Load acquire is needed to make sure that we see that terminating NUL.
*/
-static bool prepend_name(char **buffer, int *buflen, const struct qstr *name)
+static bool prepend_name(struct prepend_buffer *p, const struct qstr *name)
{
const char *dname = smp_load_acquire(&name->name); /* ^^^ */
u32 dlen = READ_ONCE(name->len);
- char *p;
+ char *s;
- *buflen -= dlen + 1;
- if (unlikely(*buflen < 0))
+ p->len -= dlen + 1;
+ if (unlikely(p->len < 0))
return false;
- p = *buffer -= dlen + 1;
- *p++ = '/';
+ s = p->buf -= dlen + 1;
+ *s++ = '/';
while (dlen--) {
char c = *dname++;
if (!c)
break;
- *p++ = c;
+ *s++ = c;
}
return true;
}
@@ -73,15 +87,14 @@ static bool prepend_name(char **buffer, int *buflen, const struct qstr *name)
*/
static int prepend_path(const struct path *path,
const struct path *root,
- char **buffer, int *buflen)
+ struct prepend_buffer *p)
{
struct dentry *dentry;
struct vfsmount *vfsmnt;
struct mount *mnt;
int error = 0;
unsigned seq, m_seq = 0;
- char *bptr;
- int blen;
+ struct prepend_buffer b;
rcu_read_lock();
restart_mnt:
@@ -89,8 +102,7 @@ static int prepend_path(const struct path *path,
seq = 0;
rcu_read_lock();
restart:
- bptr = *buffer;
- blen = *buflen;
+ b = *p;
error = 0;
dentry = path->dentry;
vfsmnt = path->mnt;
@@ -105,8 +117,7 @@ static int prepend_path(const struct path *path,
/* Escaped? */
if (dentry != vfsmnt->mnt_root) {
- bptr = *buffer;
- blen = *buflen;
+ b = *p;
error = 3;
break;
}
@@ -127,7 +138,7 @@ static int prepend_path(const struct path *path,
}
parent = dentry->d_parent;
prefetch(parent);
- if (!prepend_name(&bptr, &blen, &dentry->d_name))
+ if (!prepend_name(&b, &dentry->d_name))
break;
dentry = parent;
@@ -148,11 +159,10 @@ static int prepend_path(const struct path *path,
}
done_seqretry(&mount_lock, m_seq);
- if (blen == *buflen)
- prepend(&bptr, &blen, "/", 1);
+ if (b.len == p->len)
+ prepend(&b, "/", 1);
- *buffer = bptr;
- *buflen = blen;
+ *p = b;
return error;
}
@@ -176,24 +186,24 @@ char *__d_path(const struct path *path,
const struct path *root,
char *buf, int buflen)
{
- char *res = buf + buflen;
+ DECLARE_BUFFER(b, buf, buflen);
- prepend(&res, &buflen, "", 1);
- if (prepend_path(path, root, &res, &buflen) > 0)
+ prepend(&b, "", 1);
+ if (prepend_path(path, root, &b) > 0)
return NULL;
- return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
+ return extract_string(&b);
}
char *d_absolute_path(const struct path *path,
char *buf, int buflen)
{
struct path root = {};
- char *res = buf + buflen;
+ DECLARE_BUFFER(b, buf, buflen);
- prepend(&res, &buflen, "", 1);
- if (prepend_path(path, &root, &res, &buflen) > 1)
+ prepend(&b, "", 1);
+ if (prepend_path(path, &root, &b) > 1)
return ERR_PTR(-EINVAL);
- return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
+ return extract_string(&b);
}
static void get_fs_root_rcu(struct fs_struct *fs, struct path *root)
@@ -224,7 +234,7 @@ static void get_fs_root_rcu(struct fs_struct *fs, struct path *root)
*/
char *d_path(const struct path *path, char *buf, int buflen)
{
- char *res = buf + buflen;
+ DECLARE_BUFFER(b, buf, buflen);
struct path root;
/*
@@ -245,13 +255,13 @@ char *d_path(const struct path *path, char *buf, int buflen)
rcu_read_lock();
get_fs_root_rcu(current->fs, &root);
if (unlikely(d_unlinked(path->dentry)))
- prepend(&res, &buflen, " (deleted)", 11);
+ prepend(&b, " (deleted)", 11);
else
- prepend(&res, &buflen, "", 1);
- prepend_path(path, &root, &res, &buflen);
+ prepend(&b, "", 1);
+ prepend_path(path, &root, &b);
rcu_read_unlock();
- return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
+ return extract_string(&b);
}
EXPORT_SYMBOL(d_path);
@@ -278,36 +288,34 @@ char *dynamic_dname(struct dentry *dentry, char *buffer, int buflen,
char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
{
- char *end = buffer + buflen;
+ DECLARE_BUFFER(b, buffer, buflen);
/* these dentries are never renamed, so d_lock is not needed */
- prepend(&end, &buflen, " (deleted)", 11);
- prepend(&end, &buflen, dentry->d_name.name, dentry->d_name.len);
- prepend(&end, &buflen, "/", 1);
- return buflen >= 0 ? end : ERR_PTR(-ENAMETOOLONG);
+ prepend(&b, " (deleted)", 11);
+ prepend(&b, dentry->d_name.name, dentry->d_name.len);
+ prepend(&b, "/", 1);
+ return extract_string(&b);
}
/*
* Write full pathname from the root of the filesystem into the buffer.
*/
-static char *__dentry_path(const struct dentry *d, char *p, int buflen)
+static char *__dentry_path(const struct dentry *d, struct prepend_buffer *p)
{
const struct dentry *dentry;
- char *end;
- int len, seq = 0;
+ struct prepend_buffer b;
+ int seq = 0;
rcu_read_lock();
restart:
dentry = d;
- end = p;
- len = buflen;
+ b = *p;
read_seqbegin_or_lock(&rename_lock, &seq);
while (!IS_ROOT(dentry)) {
const struct dentry *parent = dentry->d_parent;
prefetch(parent);
- if (!prepend_name(&end, &len, &dentry->d_name))
+ if (!prepend_name(&b, &dentry->d_name))
break;
-
dentry = parent;
}
if (!(seq & 1))
@@ -317,28 +325,29 @@ static char *__dentry_path(const struct dentry *d, char *p, int buflen)
goto restart;
}
done_seqretry(&rename_lock, seq);
- if (len == buflen)
- prepend(&end, &len, "/", 1);
- return len >= 0 ? end : ERR_PTR(-ENAMETOOLONG);
+ if (b.len == p->len)
+ prepend(&b, "/", 1);
+ return extract_string(&b);
}
char *dentry_path_raw(const struct dentry *dentry, char *buf, int buflen)
{
- char *p = buf + buflen;
- prepend(&p, &buflen, "", 1);
- return __dentry_path(dentry, p, buflen);
+ DECLARE_BUFFER(b, buf, buflen);
+
+ prepend(&b, "", 1);
+ return __dentry_path(dentry, &b);
}
EXPORT_SYMBOL(dentry_path_raw);
char *dentry_path(const struct dentry *dentry, char *buf, int buflen)
{
- char *p = buf + buflen;
+ DECLARE_BUFFER(b, buf, buflen);
if (unlikely(d_unlinked(dentry)))
- prepend(&p, &buflen, "//deleted", 10);
+ prepend(&b, "//deleted", 10);
else
- prepend(&p, &buflen, "", 1);
- return __dentry_path(dentry, p, buflen);
+ prepend(&b, "", 1);
+ return __dentry_path(dentry, &b);
}
static void get_fs_root_and_pwd_rcu(struct fs_struct *fs, struct path *root,
@@ -386,24 +395,23 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned long, size)
error = -ENOENT;
if (!d_unlinked(pwd.dentry)) {
unsigned long len;
- char *cwd = page + PATH_MAX;
- int buflen = PATH_MAX;
+ DECLARE_BUFFER(b, page, PATH_MAX);
- prepend(&cwd, &buflen, "", 1);
- if (prepend_path(&pwd, &root, &cwd, &buflen) > 0)
- prepend(&cwd, &buflen, "(unreachable)", 13);
+ prepend(&b, "", 1);
+ if (prepend_path(&pwd, &root, &b) > 0)
+ prepend(&b, "(unreachable)", 13);
rcu_read_unlock();
- if (buflen < 0) {
+ if (b.len < 0) {
error = -ENAMETOOLONG;
goto out;
}
error = -ERANGE;
- len = PATH_MAX + page - cwd;
+ len = PATH_MAX - b.len;
if (len <= size) {
error = len;
- if (copy_to_user(buf, cwd, len))
+ if (copy_to_user(buf, b.buf, len))
error = -EFAULT;
}
} else {
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* RE: [PATCH 09/14] d_path: introduce struct prepend_buffer
2021-05-19 0:48 ` [PATCH 09/14] d_path: introduce struct prepend_buffer Al Viro
@ 2021-06-23 13:28 ` Justin He
2021-06-24 9:29 ` Enrico Weigelt, metux IT consult
0 siblings, 1 reply; 75+ messages in thread
From: Justin He @ 2021-06-23 13:28 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
Hi Al
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 09/14] d_path: introduce struct prepend_buffer
>
> We've a lot of places where we have pairs of form (pointer to end
> of buffer, amount of space left in front of that). These sit in pairs of
> variables located next to each other and usually passed by reference.
> Turn those into instances of new type (struct prepend_buffer) and pass
> reference to the pair instead of pairs of references to its fields.
>
> Declared and initialized by DECLARE_BUFFER(name, buf, buflen).
>
> extract_string(prepend_buffer) returns the buffer contents if
> no overflow has happened, ERR_PTR(ENAMETOOLONG) otherwise.
> All places where we used to have that boilerplate converted to use
> of that helper.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> fs/d_path.c | 142 ++++++++++++++++++++++++++++++++-----------------------
> -----
> 1 file changed, 75 insertions(+), 67 deletions(-)
>
> diff --git a/fs/d_path.c b/fs/d_path.c
> index 83db83446afd..06e93dd031bf 100644
> --- a/fs/d_path.c
> +++ b/fs/d_path.c
> @@ -8,12 +8,26 @@
> #include <linux/prefetch.h>
> #include "mount.h"
>
> -static void prepend(char **buffer, int *buflen, const char *str, int
> namelen)
> +struct prepend_buffer {
> + char *buf;
> + int len;
> +};
> +#define DECLARE_BUFFER(__name, __buf, __len) \
> + struct prepend_buffer __name = {.buf = __buf + __len, .len = __len}
> +
> +static char *extract_string(struct prepend_buffer *p)
> {
> - *buflen -= namelen;
> - if (likely(*buflen >= 0)) {
> - *buffer -= namelen;
> - memcpy(*buffer, str, namelen);
> + if (likely(p->len >= 0))
> + return p->buf;
> + return ERR_PTR(-ENAMETOOLONG);
> +}
> +
> +static void prepend(struct prepend_buffer *p, const char *str, int
> namelen)
> +{
> + p->len -= namelen;
> + if (likely(p->len >= 0)) {
> + p->buf -= namelen;
> + memcpy(p->buf, str, namelen);
> }
> }
>
> @@ -34,22 +48,22 @@ static void prepend(char **buffer, int *buflen, const
> char *str, int namelen)
> *
> * Load acquire is needed to make sure that we see that terminating NUL.
> */
> -static bool prepend_name(char **buffer, int *buflen, const struct qstr
> *name)
> +static bool prepend_name(struct prepend_buffer *p, const struct qstr
> *name)
Please also change the parameter description in the comments of
prepend_name(), otherwise "make C=1 W=1" will report warnings.
--
Cheers,
Justin (Jia He)
> {
> const char *dname = smp_load_acquire(&name->name); /* ^^^ */
> u32 dlen = READ_ONCE(name->len);
> - char *p;
> + char *s;
>
> - *buflen -= dlen + 1;
> - if (unlikely(*buflen < 0))
> + p->len -= dlen + 1;
> + if (unlikely(p->len < 0))
> return false;
> - p = *buffer -= dlen + 1;
> - *p++ = '/';
> + s = p->buf -= dlen + 1;
> + *s++ = '/';
> while (dlen--) {
> char c = *dname++;
> if (!c)
> break;
> - *p++ = c;
> + *s++ = c;
> }
> return true;
> }
> @@ -73,15 +87,14 @@ static bool prepend_name(char **buffer, int *buflen,
> const struct qstr *name)
> */
> static int prepend_path(const struct path *path,
> const struct path *root,
> - char **buffer, int *buflen)
> + struct prepend_buffer *p)
> {
> struct dentry *dentry;
> struct vfsmount *vfsmnt;
> struct mount *mnt;
> int error = 0;
> unsigned seq, m_seq = 0;
> - char *bptr;
> - int blen;
> + struct prepend_buffer b;
>
> rcu_read_lock();
> restart_mnt:
> @@ -89,8 +102,7 @@ static int prepend_path(const struct path *path,
> seq = 0;
> rcu_read_lock();
> restart:
> - bptr = *buffer;
> - blen = *buflen;
> + b = *p;
> error = 0;
> dentry = path->dentry;
> vfsmnt = path->mnt;
> @@ -105,8 +117,7 @@ static int prepend_path(const struct path *path,
>
> /* Escaped? */
> if (dentry != vfsmnt->mnt_root) {
> - bptr = *buffer;
> - blen = *buflen;
> + b = *p;
> error = 3;
> break;
> }
> @@ -127,7 +138,7 @@ static int prepend_path(const struct path *path,
> }
> parent = dentry->d_parent;
> prefetch(parent);
> - if (!prepend_name(&bptr, &blen, &dentry->d_name))
> + if (!prepend_name(&b, &dentry->d_name))
> break;
>
> dentry = parent;
> @@ -148,11 +159,10 @@ static int prepend_path(const struct path *path,
> }
> done_seqretry(&mount_lock, m_seq);
>
> - if (blen == *buflen)
> - prepend(&bptr, &blen, "/", 1);
> + if (b.len == p->len)
> + prepend(&b, "/", 1);
>
> - *buffer = bptr;
> - *buflen = blen;
> + *p = b;
> return error;
> }
>
> @@ -176,24 +186,24 @@ char *__d_path(const struct path *path,
> const struct path *root,
> char *buf, int buflen)
> {
> - char *res = buf + buflen;
> + DECLARE_BUFFER(b, buf, buflen);
>
> - prepend(&res, &buflen, "", 1);
> - if (prepend_path(path, root, &res, &buflen) > 0)
> + prepend(&b, "", 1);
> + if (prepend_path(path, root, &b) > 0)
> return NULL;
> - return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
> + return extract_string(&b);
> }
>
> char *d_absolute_path(const struct path *path,
> char *buf, int buflen)
> {
> struct path root = {};
> - char *res = buf + buflen;
> + DECLARE_BUFFER(b, buf, buflen);
>
> - prepend(&res, &buflen, "", 1);
> - if (prepend_path(path, &root, &res, &buflen) > 1)
> + prepend(&b, "", 1);
> + if (prepend_path(path, &root, &b) > 1)
> return ERR_PTR(-EINVAL);
> - return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
> + return extract_string(&b);
> }
>
> static void get_fs_root_rcu(struct fs_struct *fs, struct path *root)
> @@ -224,7 +234,7 @@ static void get_fs_root_rcu(struct fs_struct *fs,
> struct path *root)
> */
> char *d_path(const struct path *path, char *buf, int buflen)
> {
> - char *res = buf + buflen;
> + DECLARE_BUFFER(b, buf, buflen);
> struct path root;
>
> /*
> @@ -245,13 +255,13 @@ char *d_path(const struct path *path, char *buf, int
> buflen)
> rcu_read_lock();
> get_fs_root_rcu(current->fs, &root);
> if (unlikely(d_unlinked(path->dentry)))
> - prepend(&res, &buflen, " (deleted)", 11);
> + prepend(&b, " (deleted)", 11);
> else
> - prepend(&res, &buflen, "", 1);
> - prepend_path(path, &root, &res, &buflen);
> + prepend(&b, "", 1);
> + prepend_path(path, &root, &b);
> rcu_read_unlock();
>
> - return buflen >= 0 ? res : ERR_PTR(-ENAMETOOLONG);
> + return extract_string(&b);
> }
> EXPORT_SYMBOL(d_path);
>
> @@ -278,36 +288,34 @@ char *dynamic_dname(struct dentry *dentry, char
> *buffer, int buflen,
>
> char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
> {
> - char *end = buffer + buflen;
> + DECLARE_BUFFER(b, buffer, buflen);
> /* these dentries are never renamed, so d_lock is not needed */
> - prepend(&end, &buflen, " (deleted)", 11);
> - prepend(&end, &buflen, dentry->d_name.name, dentry->d_name.len);
> - prepend(&end, &buflen, "/", 1);
> - return buflen >= 0 ? end : ERR_PTR(-ENAMETOOLONG);
> + prepend(&b, " (deleted)", 11);
> + prepend(&b, dentry->d_name.name, dentry->d_name.len);
> + prepend(&b, "/", 1);
> + return extract_string(&b);
> }
>
> /*
> * Write full pathname from the root of the filesystem into the buffer.
> */
> -static char *__dentry_path(const struct dentry *d, char *p, int buflen)
> +static char *__dentry_path(const struct dentry *d, struct prepend_buffer
> *p)
> {
> const struct dentry *dentry;
> - char *end;
> - int len, seq = 0;
> + struct prepend_buffer b;
> + int seq = 0;
>
> rcu_read_lock();
> restart:
> dentry = d;
> - end = p;
> - len = buflen;
> + b = *p;
> read_seqbegin_or_lock(&rename_lock, &seq);
> while (!IS_ROOT(dentry)) {
> const struct dentry *parent = dentry->d_parent;
>
> prefetch(parent);
> - if (!prepend_name(&end, &len, &dentry->d_name))
> + if (!prepend_name(&b, &dentry->d_name))
> break;
> -
> dentry = parent;
> }
> if (!(seq & 1))
> @@ -317,28 +325,29 @@ static char *__dentry_path(const struct dentry *d,
> char *p, int buflen)
> goto restart;
> }
> done_seqretry(&rename_lock, seq);
> - if (len == buflen)
> - prepend(&end, &len, "/", 1);
> - return len >= 0 ? end : ERR_PTR(-ENAMETOOLONG);
> + if (b.len == p->len)
> + prepend(&b, "/", 1);
> + return extract_string(&b);
> }
>
> char *dentry_path_raw(const struct dentry *dentry, char *buf, int buflen)
> {
> - char *p = buf + buflen;
> - prepend(&p, &buflen, "", 1);
> - return __dentry_path(dentry, p, buflen);
> + DECLARE_BUFFER(b, buf, buflen);
> +
> + prepend(&b, "", 1);
> + return __dentry_path(dentry, &b);
> }
> EXPORT_SYMBOL(dentry_path_raw);
>
> char *dentry_path(const struct dentry *dentry, char *buf, int buflen)
> {
> - char *p = buf + buflen;
> + DECLARE_BUFFER(b, buf, buflen);
>
> if (unlikely(d_unlinked(dentry)))
> - prepend(&p, &buflen, "//deleted", 10);
> + prepend(&b, "//deleted", 10);
> else
> - prepend(&p, &buflen, "", 1);
> - return __dentry_path(dentry, p, buflen);
> + prepend(&b, "", 1);
> + return __dentry_path(dentry, &b);
> }
>
> static void get_fs_root_and_pwd_rcu(struct fs_struct *fs, struct path
> *root,
> @@ -386,24 +395,23 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned
> long, size)
> error = -ENOENT;
> if (!d_unlinked(pwd.dentry)) {
> unsigned long len;
> - char *cwd = page + PATH_MAX;
> - int buflen = PATH_MAX;
> + DECLARE_BUFFER(b, page, PATH_MAX);
>
> - prepend(&cwd, &buflen, "", 1);
> - if (prepend_path(&pwd, &root, &cwd, &buflen) > 0)
> - prepend(&cwd, &buflen, "(unreachable)", 13);
> + prepend(&b, "", 1);
> + if (prepend_path(&pwd, &root, &b) > 0)
> + prepend(&b, "(unreachable)", 13);
> rcu_read_unlock();
>
> - if (buflen < 0) {
> + if (b.len < 0) {
> error = -ENAMETOOLONG;
> goto out;
> }
>
> error = -ERANGE;
> - len = PATH_MAX + page - cwd;
> + len = PATH_MAX - b.len;
> if (len <= size) {
> error = len;
> - if (copy_to_user(buf, cwd, len))
> + if (copy_to_user(buf, b.buf, len))
> error = -EFAULT;
> }
> } else {
> --
> 2.11.0
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 09/14] d_path: introduce struct prepend_buffer
2021-06-23 13:28 ` Justin He
@ 2021-06-24 9:29 ` Enrico Weigelt, metux IT consult
2021-06-25 0:43 ` Justin He
0 siblings, 1 reply; 75+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-06-24 9:29 UTC (permalink / raw)
To: Justin He, Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
Hi folks,
<snip>
>> We've a lot of places where we have pairs of form (pointer to end
>> of buffer, amount of space left in front of that). These sit in pairs of
>> variables located next to each other and usually passed by reference.
>> Turn those into instances of new type (struct prepend_buffer) and pass
>> reference to the pair instead of pairs of references to its fields.
>>
>> Declared and initialized by DECLARE_BUFFER(name, buf, buflen).
>>
>> extract_string(prepend_buffer) returns the buffer contents if
>> no overflow has happened, ERR_PTR(ENAMETOOLONG) otherwise.
>> All places where we used to have that boilerplate converted to use
>> of that helper.
this smells like a generic enough thing to go into lib, doesn't it ?
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply [flat|nested] 75+ messages in thread
* RE: [PATCH 09/14] d_path: introduce struct prepend_buffer
2021-06-24 9:29 ` Enrico Weigelt, metux IT consult
@ 2021-06-25 0:43 ` Justin He
2021-06-28 16:42 ` Enrico Weigelt, metux IT consult
0 siblings, 1 reply; 75+ messages in thread
From: Justin He @ 2021-06-25 0:43 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult, Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel, nd
Hi Enrico
> -----Original Message-----
> From: Enrico Weigelt, metux IT consult <lkml@metux.net>
> Sent: Thursday, June 24, 2021 5:30 PM
> To: Justin He <Justin.He@arm.com>; Al Viro <viro@zeniv.linux.org.uk>; Linus
> Torvalds <torvalds@linux-foundation.org>
> Cc: 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: Re: [PATCH 09/14] d_path: introduce struct prepend_buffer
>
> Hi folks,
>
> <snip>
>
> >> We've a lot of places where we have pairs of form (pointer to
> end
> >> of buffer, amount of space left in front of that). These sit in pairs
> of
> >> variables located next to each other and usually passed by reference.
> >> Turn those into instances of new type (struct prepend_buffer) and pass
> >> reference to the pair instead of pairs of references to its fields.
> >>
> >> Declared and initialized by DECLARE_BUFFER(name, buf, buflen).
> >>
> >> extract_string(prepend_buffer) returns the buffer contents if
> >> no overflow has happened, ERR_PTR(ENAMETOOLONG) otherwise.
> >> All places where we used to have that boilerplate converted to use
> >> of that helper.
>
> this smells like a generic enough thing to go into lib, doesn't it ?
>
Maybe, but the struct prepend_buffer also needs to be moved into lib.
Is it necessary? Is there any other user of struct prepend_buffer?
--
Cheers,
Justin (Jia He)
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 09/14] d_path: introduce struct prepend_buffer
2021-06-25 0:43 ` Justin He
@ 2021-06-28 16:42 ` Enrico Weigelt, metux IT consult
2021-06-28 17:10 ` Andy Shevchenko
0 siblings, 1 reply; 75+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-06-28 16:42 UTC (permalink / raw)
To: Justin He, Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel, nd
>> this smells like a generic enough thing to go into lib, doesn't it ?
>>
> Maybe, but the struct prepend_buffer also needs to be moved into lib.
> Is it necessary? Is there any other user of struct prepend_buffer?
Don't have a specific use case right now, but it smells like a pretty
generic string function. Is already having more than one user a hard
requirement for putting something into lib ?
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 09/14] d_path: introduce struct prepend_buffer
2021-06-28 16:42 ` Enrico Weigelt, metux IT consult
@ 2021-06-28 17:10 ` Andy Shevchenko
0 siblings, 0 replies; 75+ messages in thread
From: Andy Shevchenko @ 2021-06-28 17:10 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: Justin He, Al Viro, Linus Torvalds, Petr Mladek, Steven Rostedt,
Sergey Senozhatsky, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel, nd
On Mon, Jun 28, 2021 at 06:42:48PM +0200, Enrico Weigelt, metux IT consult wrote:
> > > this smells like a generic enough thing to go into lib, doesn't it ?
> > >
> > Maybe, but the struct prepend_buffer also needs to be moved into lib.
> > Is it necessary? Is there any other user of struct prepend_buffer?
>
> Don't have a specific use case right now, but it smells like a pretty
> generic string function. Is already having more than one user a hard
> requirement for putting something into lib ?
Why it should be in the lib/? Do we have users of the same functionality
already? The rule of thumb is to avoid generalization without need.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 10/14] d_path: prepend_path(): get rid of vfsmnt
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
` (7 preceding siblings ...)
2021-05-19 0:48 ` [PATCH 09/14] d_path: introduce struct prepend_buffer Al Viro
@ 2021-05-19 0:48 ` Al Viro
2021-05-19 0:48 ` [PATCH 11/14] d_path: prepend_path(): lift resetting b in case when we'd return 3 out of loop Al Viro
` (4 subsequent siblings)
13 siblings, 0 replies; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
it's kept equal to &mnt->mnt all along.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index 06e93dd031bf..3836f5d0b023 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -90,7 +90,6 @@ static int prepend_path(const struct path *path,
struct prepend_buffer *p)
{
struct dentry *dentry;
- struct vfsmount *vfsmnt;
struct mount *mnt;
int error = 0;
unsigned seq, m_seq = 0;
@@ -105,18 +104,17 @@ static int prepend_path(const struct path *path,
b = *p;
error = 0;
dentry = path->dentry;
- vfsmnt = path->mnt;
- mnt = real_mount(vfsmnt);
+ mnt = real_mount(path->mnt);
read_seqbegin_or_lock(&rename_lock, &seq);
- while (dentry != root->dentry || vfsmnt != root->mnt) {
+ while (dentry != root->dentry || &mnt->mnt != root->mnt) {
struct dentry * parent;
- if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
+ if (dentry == mnt->mnt.mnt_root || IS_ROOT(dentry)) {
struct mount *parent = READ_ONCE(mnt->mnt_parent);
struct mnt_namespace *mnt_ns;
/* Escaped? */
- if (dentry != vfsmnt->mnt_root) {
+ if (dentry != mnt->mnt.mnt_root) {
b = *p;
error = 3;
break;
@@ -125,7 +123,6 @@ static int prepend_path(const struct path *path,
if (mnt != parent) {
dentry = READ_ONCE(mnt->mnt_mountpoint);
mnt = parent;
- vfsmnt = &mnt->mnt;
continue;
}
mnt_ns = READ_ONCE(mnt->mnt_ns);
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCH 11/14] d_path: prepend_path(): lift resetting b in case when we'd return 3 out of loop
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
` (8 preceding siblings ...)
2021-05-19 0:48 ` [PATCH 10/14] d_path: prepend_path(): get rid of vfsmnt Al Viro
@ 2021-05-19 0:48 ` Al Viro
2021-05-19 0:48 ` [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a new helper Al Viro
` (3 subsequent siblings)
13 siblings, 0 replies; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
preparation to extracting the loop into helper (next commit)
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index 3836f5d0b023..9a0356cc98d3 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -115,7 +115,6 @@ static int prepend_path(const struct path *path,
/* Escaped? */
if (dentry != mnt->mnt.mnt_root) {
- b = *p;
error = 3;
break;
}
@@ -156,6 +155,9 @@ static int prepend_path(const struct path *path,
}
done_seqretry(&mount_lock, m_seq);
+ if (unlikely(error == 3))
+ b = *p;
+
if (b.len == p->len)
prepend(&b, "/", 1);
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a new helper
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
` (9 preceding siblings ...)
2021-05-19 0:48 ` [PATCH 11/14] d_path: prepend_path(): lift resetting b in case when we'd return 3 out of loop Al Viro
@ 2021-05-19 0:48 ` Al Viro
2021-05-19 8:07 ` Andy Shevchenko
2021-07-07 7:52 ` Justin He
2021-05-19 0:49 ` [PATCH 13/14] d_path: prepend_path() is unlikely to return non-zero Al Viro
` (2 subsequent siblings)
13 siblings, 2 replies; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
... and leave the rename_lock/mount_lock handling in prepend_path()
itself
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 77 ++++++++++++++++++++++++++++++-------------------------------
1 file changed, 38 insertions(+), 39 deletions(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index 9a0356cc98d3..ba629879a4bf 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -68,6 +68,42 @@ static bool prepend_name(struct prepend_buffer *p, const struct qstr *name)
return true;
}
+static int __prepend_path(const struct dentry *dentry, const struct mount *mnt,
+ const struct path *root, struct prepend_buffer *p)
+{
+ while (dentry != root->dentry || &mnt->mnt != root->mnt) {
+ const struct dentry *parent = READ_ONCE(dentry->d_parent);
+
+ if (dentry == mnt->mnt.mnt_root) {
+ struct mount *m = READ_ONCE(mnt->mnt_parent);
+ struct mnt_namespace *mnt_ns;
+
+ if (likely(mnt != m)) {
+ dentry = READ_ONCE(mnt->mnt_mountpoint);
+ mnt = m;
+ continue;
+ }
+ /* Global root */
+ 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
+ else
+ return 2; // detached or not attached yet
+ }
+
+ if (unlikely(dentry == parent))
+ /* Escaped? */
+ return 3;
+
+ prefetch(parent);
+ if (!prepend_name(p, &dentry->d_name))
+ break;
+ dentry = parent;
+ }
+ return 0;
+}
+
/**
* prepend_path - Prepend path string to a buffer
* @path: the dentry/vfsmount to report
@@ -89,11 +125,9 @@ static int prepend_path(const struct path *path,
const struct path *root,
struct prepend_buffer *p)
{
- struct dentry *dentry;
- struct mount *mnt;
- int error = 0;
unsigned seq, m_seq = 0;
struct prepend_buffer b;
+ int error;
rcu_read_lock();
restart_mnt:
@@ -102,43 +136,8 @@ static int prepend_path(const struct path *path,
rcu_read_lock();
restart:
b = *p;
- error = 0;
- dentry = path->dentry;
- mnt = real_mount(path->mnt);
read_seqbegin_or_lock(&rename_lock, &seq);
- while (dentry != root->dentry || &mnt->mnt != root->mnt) {
- struct dentry * parent;
-
- if (dentry == mnt->mnt.mnt_root || IS_ROOT(dentry)) {
- struct mount *parent = READ_ONCE(mnt->mnt_parent);
- struct mnt_namespace *mnt_ns;
-
- /* Escaped? */
- if (dentry != mnt->mnt.mnt_root) {
- error = 3;
- break;
- }
- /* Global root? */
- if (mnt != parent) {
- dentry = READ_ONCE(mnt->mnt_mountpoint);
- mnt = parent;
- continue;
- }
- 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))
- error = 1; // absolute root
- else
- error = 2; // detached or not attached yet
- break;
- }
- parent = dentry->d_parent;
- prefetch(parent);
- if (!prepend_name(&b, &dentry->d_name))
- break;
-
- dentry = parent;
- }
+ error = __prepend_path(path->dentry, real_mount(path->mnt), root, &b);
if (!(seq & 1))
rcu_read_unlock();
if (need_seqretry(&rename_lock, seq)) {
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* Re: [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a new helper
2021-05-19 0:48 ` [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a new helper Al Viro
@ 2021-05-19 8:07 ` Andy Shevchenko
2021-05-19 15:55 ` Al Viro
2021-07-07 7:52 ` Justin He
1 sibling, 1 reply; 75+ messages in thread
From: Andy Shevchenko @ 2021-05-19 8:07 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, Jia He, Petr Mladek, Steven Rostedt,
Sergey Senozhatsky, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
On Wed, May 19, 2021 at 12:48:59AM +0000, Al Viro wrote:
> ... and leave the rename_lock/mount_lock handling in prepend_path()
> itself
...
> + if (!IS_ERR_OR_NULL(mnt_ns) && !is_anon_ns(mnt_ns))
> + return 1; // absolute root
> + else
> + return 2; // detached or not attached yet
Would it be slightly better to read
if (IS_ERR_OR_NULL(mnt_ns) || is_anon_ns(mnt_ns))
return 2; // detached or not attached yet
else
return 1; // absolute root
?
Oh, I have noticed that it's in the original piece of code (perhaps separate
change if we ever need it?).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a new helper
2021-05-19 8:07 ` Andy Shevchenko
@ 2021-05-19 15:55 ` Al Viro
0 siblings, 0 replies; 75+ messages in thread
From: Al Viro @ 2021-05-19 15:55 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Torvalds, Jia He, Petr Mladek, Steven Rostedt,
Sergey Senozhatsky, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
On Wed, May 19, 2021 at 11:07:04AM +0300, Andy Shevchenko wrote:
> On Wed, May 19, 2021 at 12:48:59AM +0000, Al Viro wrote:
> > ... and leave the rename_lock/mount_lock handling in prepend_path()
> > itself
>
> ...
>
> > + if (!IS_ERR_OR_NULL(mnt_ns) && !is_anon_ns(mnt_ns))
> > + return 1; // absolute root
> > + else
> > + return 2; // detached or not attached yet
>
> Would it be slightly better to read
>
> if (IS_ERR_OR_NULL(mnt_ns) || is_anon_ns(mnt_ns))
> return 2; // detached or not attached yet
> else
> return 1; // absolute root
>
> ?
>
> Oh, I have noticed that it's in the original piece of code (perhaps separate
> change if we ever need it?).
The real readability problem here is not the negations. There are 4 possible
states for vfsmount encoded via ->mnt_ns:
1) not attached to any tree, kept alive by refcount alone.
->mnt_ns == NULL.
2) long-term unattached. Not a part of any mount tree, but we have
a known holder for it and until that's gone (making ->mnt_ns NULL), refcount
is guaranteed to remain positive. pipe_mnt is an example of such.
->mnt_ns == MNT_NS_INTERNAL, which is encoded as ERR_PTR(-1), thus the use of
IS_ERR_OR_NULL here (something I'd normally taken out and shot - use of that
primitive is a sign of lousy API or of a cargo-culted "defensive programming").
3) part of a temporary mount tree; not in anyone's namespace.
->mnt_ns points the tree in question, ->mnt_ns->seq == 0.
4) belongs to someone's namespace. ->mnt_ns points to that,
->mnt_ns->seq != 0. That's what we are looking for here.
It's kludges all the way down ;-/ Note that temporary tree can't become
a normal one or vice versa - mounts can get transferred to normal namespace,
but they will see ->mnt_ns reassigned to that. IOW, ->mnt_ns->seq can't
get changed without a change to ->mnt_ns. I suspect that the right way
to handle that would be to have that state stored as explicit flags.
All mounts are created (and destroyed) in state (1); state changes:
commit_tree() - (1) or (3) to (3) or (4)
umount_tree() - (3) or (4) to (1)
clone_private_mount() - (1) to (2)
open_detached_copy() - (1) to (3)
copy_mnt_ns() - (1) to (4)
mount_subtree() - (1) to (3)
fsmount() - (1) to (3)
init_mount_tree() - (1) to (4)
kern_mount() - (1) to (2)
kern_unmount{,_array}() - (2) to (1)
commit_tree() has a pathological call chain that has it
attach stuff to temporary tree; that's basically automount by lookup in
temporary namespace. It can distinguish it from the usual (adding to
normal namespace) by looking at the state of mountpoint we are attaching
to - or simply describe all cases as "(1) or (3) to whatever state the
mountpoint is".
One really hot path where we check (1) vs. (2,3,4) is
mntput_no_expire(), which is the initial reason behind the current
representation. However, read from ->mnt_flags is just as cheap as
that from ->mnt_ns and the same reasons that make READ_ONCE()
legitimate there would apply to ->mnt_flags as well.
We can't reuse MNT_INTERNAL for that, more's the pity -
it's used to mark the mounts (kern_mount()-created, mostly) that
need to destroyed synchronously on the final mntput(), with no
task_work_add() allowed (think of module_init() failing halfway through,
with kern_unmount() done to destroy the internal mounts already created;
we *really* don't want to delay that filesystem shutdown until insmod(2)
heads out to userland). Another headache is in LSM shite, as usual...
Anyway, sorting that out is definitely a separate story.
^ permalink raw reply [flat|nested] 75+ messages in thread
* RE: [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a new helper
2021-05-19 0:48 ` [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a new helper Al Viro
2021-05-19 8:07 ` Andy Shevchenko
@ 2021-07-07 7:52 ` Justin He
1 sibling, 0 replies; 75+ messages in thread
From: Justin He @ 2021-07-07 7:52 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, nd, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a
> new helper
>
> ... and leave the rename_lock/mount_lock handling in prepend_path()
> itself
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Reviewed-by: Jia He <justin.he@arm.com>
--
Cheers,
Justin (Jia He)
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 13/14] d_path: prepend_path() is unlikely to return non-zero
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
` (10 preceding siblings ...)
2021-05-19 0:48 ` [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a new helper Al Viro
@ 2021-05-19 0:49 ` Al Viro
2021-06-25 8:00 ` Justin He
2021-06-28 4:37 ` Justin He
2021-05-19 0:49 ` [PATCH 14/14] getcwd(2): clean up error handling Al Viro
2021-06-24 6:05 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Justin He
13 siblings, 2 replies; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:49 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index ba629879a4bf..8a9cd44f6689 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -187,7 +187,7 @@ char *__d_path(const struct path *path,
DECLARE_BUFFER(b, buf, buflen);
prepend(&b, "", 1);
- if (prepend_path(path, root, &b) > 0)
+ if (unlikely(prepend_path(path, root, &b) > 0))
return NULL;
return extract_string(&b);
}
@@ -199,7 +199,7 @@ char *d_absolute_path(const struct path *path,
DECLARE_BUFFER(b, buf, buflen);
prepend(&b, "", 1);
- if (prepend_path(path, &root, &b) > 1)
+ if (unlikely(prepend_path(path, &root, &b) > 1))
return ERR_PTR(-EINVAL);
return extract_string(&b);
}
@@ -396,7 +396,7 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned long, size)
DECLARE_BUFFER(b, page, PATH_MAX);
prepend(&b, "", 1);
- if (prepend_path(&pwd, &root, &b) > 0)
+ if (unlikely(prepend_path(&pwd, &root, &b) > 0))
prepend(&b, "(unreachable)", 13);
rcu_read_unlock();
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* RE: [PATCH 13/14] d_path: prepend_path() is unlikely to return non-zero
2021-05-19 0:49 ` [PATCH 13/14] d_path: prepend_path() is unlikely to return non-zero Al Viro
@ 2021-06-25 8:00 ` Justin He
2021-06-25 17:58 ` Al Viro
2021-06-28 4:37 ` Justin He
1 sibling, 1 reply; 75+ messages in thread
From: Justin He @ 2021-06-25 8:00 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
Hi Al
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 13/14] d_path: prepend_path() is unlikely to return non-
> zero
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> fs/d_path.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/d_path.c b/fs/d_path.c
> index ba629879a4bf..8a9cd44f6689 100644
> --- a/fs/d_path.c
> +++ b/fs/d_path.c
> @@ -187,7 +187,7 @@ char *__d_path(const struct path *path,
> DECLARE_BUFFER(b, buf, buflen);
>
> prepend(&b, "", 1);
> - if (prepend_path(path, root, &b) > 0)
> + if (unlikely(prepend_path(path, root, &b) > 0))
> return NULL;
> return extract_string(&b);
> }
> @@ -199,7 +199,7 @@ char *d_absolute_path(const struct path *path,
> DECLARE_BUFFER(b, buf, buflen);
>
> prepend(&b, "", 1);
> - if (prepend_path(path, &root, &b) > 1)
> + if (unlikely(prepend_path(path, &root, &b) > 1))
> return ERR_PTR(-EINVAL);
> return extract_string(&b);
> }
> @@ -396,7 +396,7 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned
> long, size)
> DECLARE_BUFFER(b, page, PATH_MAX);
>
> prepend(&b, "", 1);
> - if (prepend_path(&pwd, &root, &b) > 0)
> + if (unlikely(prepend_path(&pwd, &root, &b) > 0))
> prepend(&b, "(unreachable)", 13);
> rcu_read_unlock();
>
> --
> 2.11.0
I tested it with a debugging patch as follows:
diff --git a/fs/d_path.c b/fs/d_path.c
index aea254ac9e1f..8eecd04be7bb 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -210,6 +210,7 @@ static int prepend_path(const struct path *path,
b = *p;
read_seqbegin_or_lock(&rename_lock, &seq);
error = __prepend_path(path->dentry, real_mount(path->mnt), root, &b);
+ printk("prepend=%d",error);
if (!(seq & 1))
rcu_read_unlock();
if (need_seqretry(&rename_lock, seq)) {
Then the result seems a little different:
root@entos-ampere-02:~# dmesg |grep prepend=1 |wc -l
7417
root@entos-ampere-02:~# dmesg |grep prepend=0 |wc -l
772
The kernel is 5.13.0-rc2+ + this series + my '%pD' series
Any thoughts?
---
Cheers,
Jia He (Justin)
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 75+ messages in thread
* Re: [PATCH 13/14] d_path: prepend_path() is unlikely to return non-zero
2021-06-25 8:00 ` Justin He
@ 2021-06-25 17:58 ` Al Viro
2021-06-28 3:28 ` Justin He
0 siblings, 1 reply; 75+ messages in thread
From: Al Viro @ 2021-06-25 17:58 UTC (permalink / raw)
To: Justin He
Cc: Linus Torvalds, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
On Fri, Jun 25, 2021 at 08:00:49AM +0000, Justin He wrote:
> --- a/fs/d_path.c
> +++ b/fs/d_path.c
> @@ -210,6 +210,7 @@ static int prepend_path(const struct path *path,
> b = *p;
> read_seqbegin_or_lock(&rename_lock, &seq);
> error = __prepend_path(path->dentry, real_mount(path->mnt), root, &b);
> + printk("prepend=%d",error);
> if (!(seq & 1))
> rcu_read_unlock();
> if (need_seqretry(&rename_lock, seq)) {
>
> Then the result seems a little different:
> root@entos-ampere-02:~# dmesg |grep prepend=1 |wc -l
> 7417
> root@entos-ampere-02:~# dmesg |grep prepend=0 |wc -l
> 772
>
> The kernel is 5.13.0-rc2+ + this series + my '%pD' series
>
> Any thoughts?
On which loads? 1 here is "mount/dentry pair is in somebody
else's namespace or outside of the subtree we are chrooted
into". IOW, what's calling d_path() on your setup?
^ permalink raw reply [flat|nested] 75+ messages in thread
* RE: [PATCH 13/14] d_path: prepend_path() is unlikely to return non-zero
2021-06-25 17:58 ` Al Viro
@ 2021-06-28 3:28 ` Justin He
2021-06-28 4:14 ` Al Viro
0 siblings, 1 reply; 75+ messages in thread
From: Justin He @ 2021-06-28 3:28 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
Hi Al
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Saturday, June 26, 2021 1:59 AM
> To: Justin He <Justin.He@arm.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: Re: [PATCH 13/14] d_path: prepend_path() is unlikely to return
> non-zero
>
> On Fri, Jun 25, 2021 at 08:00:49AM +0000, Justin He wrote:
> > --- a/fs/d_path.c
> > +++ b/fs/d_path.c
> > @@ -210,6 +210,7 @@ static int prepend_path(const struct path *path,
> > b = *p;
> > read_seqbegin_or_lock(&rename_lock, &seq);
> > error = __prepend_path(path->dentry, real_mount(path->mnt), root,
> &b);
> > + printk("prepend=%d",error);
> > if (!(seq & 1))
> > rcu_read_unlock();
> > if (need_seqretry(&rename_lock, seq)) {
> >
> > Then the result seems a little different:
> > root@entos-ampere-02:~# dmesg |grep prepend=1 |wc -l
> > 7417
> > root@entos-ampere-02:~# dmesg |grep prepend=0 |wc -l
> > 772
> >
> > The kernel is 5.13.0-rc2+ + this series + my '%pD' series
> >
> > Any thoughts?
>
> On which loads? 1 here is "mount/dentry pair is in somebody
> else's namespace or outside of the subtree we are chrooted
> into". IOW, what's calling d_path() on your setup?
No special loads, merely collecting the results after kernel boots up.
To narrow down, I tested your branch [1] *without* my '%pD' series:
[1] https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=work.d_path
The result is the same after kernel boots up.
The call trace are as follows:
The prepend_path returns 1:
Call trace:
prepend_path+0x144/0x340
d_absolute_path+0x6c/0xb8
aa_path_name+0x1c0/0x3d8
profile_transition+0x90/0x908
apparmor_bprm_creds_for_exec+0x914/0xaf0
security_bprm_creds_for_exec+0x34/0x50
bprm_execve+0x178/0x668
do_execveat_common.isra.47+0x1b4/0x1c8
__arm64_sys_execve+0x44/0x58
invoke_syscall+0x54/0x110
el0_svc_common.constprop.2+0x5c/0xe0
do_el0_svc+0x34/0xa0
el0_svc+0x20/0x30
el0_sync_handler+0x88/0xb0
el0_sync+0x148/0x180
The prepend_path returns 0:
Call trace:
prepend_path+0x144/0x340
d_path+0x110/0x158
proc_pid_readlink+0xbc/0x1b8
vfs_readlink+0x14c/0x170
do_readlinkat+0x134/0x168
__arm64_sys_readlinkat+0x28/0x38
invoke_syscall+0x54/0x110
el0_svc_common.constprop.2+0x5c/0xe0
do_el0_svc+0x34/0xa0
el0_svc+0x20/0x30
el0_sync_handler+0x88/0xb0
el0_sync+0x148/0x180
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 75+ messages in thread
* Re: [PATCH 13/14] d_path: prepend_path() is unlikely to return non-zero
2021-06-28 3:28 ` Justin He
@ 2021-06-28 4:14 ` Al Viro
2021-06-28 4:36 ` Justin He
0 siblings, 1 reply; 75+ messages in thread
From: Al Viro @ 2021-06-28 4:14 UTC (permalink / raw)
To: Justin He
Cc: Linus Torvalds, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
On Mon, Jun 28, 2021 at 03:28:19AM +0000, Justin He wrote:
> > On which loads? 1 here is "mount/dentry pair is in somebody
> > else's namespace or outside of the subtree we are chrooted
> > into". IOW, what's calling d_path() on your setup?
>
> No special loads, merely collecting the results after kernel boots up.
>
> To narrow down, I tested your branch [1] *without* my '%pD' series:
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=work.d_path
>
> The result is the same after kernel boots up.
IOW, you get 1 in call from d_absolute_path(). And the same commit has
- if (prepend_path(path, &root, &b) > 1)
+ if (unlikely(prepend_path(path, &root, &b) > 1))
there. What's the problem?
If you want to check mispredictions, put printks at the statements
under those if (unlikely(...)) and see how often do they trigger...
^ permalink raw reply [flat|nested] 75+ messages in thread
* RE: [PATCH 13/14] d_path: prepend_path() is unlikely to return non-zero
2021-06-28 4:14 ` Al Viro
@ 2021-06-28 4:36 ` Justin He
0 siblings, 0 replies; 75+ messages in thread
From: Justin He @ 2021-06-28 4:36 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
Hi Al
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Monday, June 28, 2021 12:14 PM
> To: Justin He <Justin.He@arm.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: Re: [PATCH 13/14] d_path: prepend_path() is unlikely to return
> non-zero
>
> On Mon, Jun 28, 2021 at 03:28:19AM +0000, Justin He wrote:
>
> > > On which loads? 1 here is "mount/dentry pair is in somebody
> > > else's namespace or outside of the subtree we are chrooted
> > > into". IOW, what's calling d_path() on your setup?
> >
> > No special loads, merely collecting the results after kernel boots up.
> >
> > To narrow down, I tested your branch [1] *without* my '%pD' series:
> > [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=work.d_
> path
> >
> > The result is the same after kernel boots up.
>
> IOW, you get 1 in call from d_absolute_path(). And the same commit has
> - if (prepend_path(path, &root, &b) > 1)
> + if (unlikely(prepend_path(path, &root, &b) > 1))
> there. What's the problem?
>
Ah, sorry for the mistake.
--
Cheers,
Justin (Jia He)
> If you want to check mispredictions, put printks at the statements
> under those if (unlikely(...)) and see how often do they trigger...
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 75+ messages in thread
* RE: [PATCH 13/14] d_path: prepend_path() is unlikely to return non-zero
2021-05-19 0:49 ` [PATCH 13/14] d_path: prepend_path() is unlikely to return non-zero Al Viro
2021-06-25 8:00 ` Justin He
@ 2021-06-28 4:37 ` Justin He
1 sibling, 0 replies; 75+ messages in thread
From: Justin He @ 2021-06-28 4:37 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 13/14] d_path: prepend_path() is unlikely to return non-
> zero
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Reviewed-by: Jia He <justin.he@arm.com>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 75+ messages in thread
* [PATCH 14/14] getcwd(2): clean up error handling
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
` (11 preceding siblings ...)
2021-05-19 0:49 ` [PATCH 13/14] d_path: prepend_path() is unlikely to return non-zero Al Viro
@ 2021-05-19 0:49 ` Al Viro
2021-07-07 8:03 ` Justin He
2021-06-24 6:05 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Justin He
13 siblings, 1 reply; 75+ messages in thread
From: Al Viro @ 2021-05-19 0:49 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jia He, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Eric W . Biederman, Darrick J. Wong, Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
fs/d_path.c | 29 ++++++++++++-----------------
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/fs/d_path.c b/fs/d_path.c
index 8a9cd44f6689..23a53f7b5c71 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -390,9 +390,11 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned long, size)
rcu_read_lock();
get_fs_root_and_pwd_rcu(current->fs, &root, &pwd);
- error = -ENOENT;
- if (!d_unlinked(pwd.dentry)) {
- unsigned long len;
+ if (unlikely(d_unlinked(pwd.dentry))) {
+ rcu_read_unlock();
+ error = -ENOENT;
+ } else {
+ unsigned len;
DECLARE_BUFFER(b, page, PATH_MAX);
prepend(&b, "", 1);
@@ -400,23 +402,16 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned long, size)
prepend(&b, "(unreachable)", 13);
rcu_read_unlock();
- if (b.len < 0) {
- error = -ENAMETOOLONG;
- goto out;
- }
-
- error = -ERANGE;
len = PATH_MAX - b.len;
- if (len <= size) {
+ if (unlikely(len > PATH_MAX))
+ error = -ENAMETOOLONG;
+ else if (unlikely(len > size))
+ error = -ERANGE;
+ else if (copy_to_user(buf, b.buf, len))
+ error = -EFAULT;
+ else
error = len;
- if (copy_to_user(buf, b.buf, len))
- error = -EFAULT;
- }
- } else {
- rcu_read_unlock();
}
-
-out:
__putname(page);
return error;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 75+ messages in thread
* RE: [PATCH 14/14] getcwd(2): clean up error handling
2021-05-19 0:49 ` [PATCH 14/14] getcwd(2): clean up error handling Al Viro
@ 2021-07-07 8:03 ` Justin He
0 siblings, 0 replies; 75+ messages in thread
From: Justin He @ 2021-07-07 8:03 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel, nd
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 14/14] getcwd(2): clean up error handling
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
Reviewed-by: Jia He <justin.he@arm.com>
--
Cheers,
Justin (Jia He)
^ permalink raw reply [flat|nested] 75+ messages in thread
* RE: [PATCH 01/14] d_path: "\0" is {0,0}, not {0}
2021-05-19 0:48 ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
` (12 preceding siblings ...)
2021-05-19 0:49 ` [PATCH 14/14] getcwd(2): clean up error handling Al Viro
@ 2021-06-24 6:05 ` Justin He
13 siblings, 0 replies; 75+ messages in thread
From: Justin He @ 2021-06-24 6:05 UTC (permalink / raw)
To: Al Viro, Linus Torvalds
Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
Rasmus Villemoes, Jonathan Corbet, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Eric W . Biederman, Darrick J. Wong,
Peter Zijlstra (Intel),
Ira Weiny, Eric Biggers, Ahmed S. Darwish,
open list:DOCUMENTATION, Linux Kernel Mailing List, linux-s390,
linux-fsdevel
> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Wednesday, May 19, 2021 8:49 AM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Justin He <Justin.He@arm.com>; 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>; Heiko
> Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian
> Borntraeger <borntraeger@de.ibm.com>; Eric W . Biederman
> <ebiederm@xmission.com>; Darrick J. Wong <darrick.wong@oracle.com>; Peter
> Zijlstra (Intel) <peterz@infradead.org>; Ira Weiny <ira.weiny@intel.com>;
> Eric Biggers <ebiggers@google.com>; Ahmed S. Darwish
> <a.darwish@linutronix.de>; open list:DOCUMENTATION <linux-
> doc@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; linux-s390 <linux-s390@vger.kernel.org>; linux-
> fsdevel <linux-fsdevel@vger.kernel.org>
> Subject: [PATCH 01/14] d_path: "\0" is {0,0}, not {0}
>
> Single-element array consisting of one NUL is spelled ""...
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Reviewed-by: Jia He <justin.he@arm.com>
--
Cheers,
Justin (Jia He)
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 75+ messages in thread