RE: [PATCH 07/14] d_path: lift -ENAMETOOLONG handling into callers of prepend_path()

[Justin He <Justin.He@arm.com>]

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.