Re: [PATCH] vfs: avoid recopying filename in getname_flags

[Boqun Feng]

Ping.
Any opinion?

Thanks,
Boqun Feng

On Wed, Feb 25, 2015 at 8:31 PM, Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
> In the current implementation of getname_flags, filename in the
> user-space will be recopied if it takes more space that
> EMBEDDED_NAME_MAX, however, at this moment, EMBEDDED_NAME_MAX bytes of
> the filename are already copied into kernel space, the only reason why
> the recopy is needed is that "kname" needs to be relocated.
>
> And the recopy can be avoided if we change the memory layout of the
> "names_cache" allocation. By putting the struct "filename" at the tail
> of the allocation instead of the head, relocation of kname is avoided.
>
> Once putting the struct at the tail, each byte in the user space will be
> copied only one time, so the recopy is avoided and code is more clear.
>
> Of course, other functions aware of the layout of the names_cache
> allocation, i.e., getname_kernel and putname also need to be modified to
> adapt to the new layout.
>
> This patch is based on v4.0-rc1.
>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: Paul Moore <pmoore@xxxxxxxxxx>
> Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> ---
>  fs/namei.c | 50 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index c83145a..3be372b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -133,21 +133,20 @@ getname_flags(const char __user *filename, int flags, int *empty)
>         if (result)
>                 return result;
>
> -       result = __getname();
> -       if (unlikely(!result))
> +       kname = __getname();
> +       if (unlikely(!kname))
>                 return ERR_PTR(-ENOMEM);
> -       result->refcnt = 1;
>
>         /*
>          * First, try to embed the struct filename inside the names_cache
>          * allocation
>          */
> -       kname = (char *)result + sizeof(*result);
> +       result = (struct filename *)(kname + EMBEDDED_NAME_MAX);
>         result->name = kname;
>         result->separate = false;
> +       result->refcnt = 1;
>         max = EMBEDDED_NAME_MAX;
>
> -recopy:
>         len = strncpy_from_user(kname, filename, max);
>         if (unlikely(len < 0)) {
>                 err = ERR_PTR(len);
> @@ -157,23 +156,34 @@ recopy:
>         /*
>          * Uh-oh. We have a name that's approaching PATH_MAX. Allocate a
>          * separate struct filename so we can dedicate the entire
> -        * names_cache allocation for the pathname, and re-do the copy from
> +        * names_cache allocation for the pathname, and continue the copy from
>          * userland.
>          */
> -       if (len == EMBEDDED_NAME_MAX && max == EMBEDDED_NAME_MAX) {
> -               kname = (char *)result;
> -
> +       if (len == EMBEDDED_NAME_MAX) {
>                 result = kzalloc(sizeof(*result), GFP_KERNEL);
>                 if (!result) {
>                         err = ERR_PTR(-ENOMEM);
> -                       result = (struct filename *)kname;
> +                       result = (struct filename *)(kname + EMBEDDED_NAME_MAX);
>                         goto error;
>                 }
>                 result->name = kname;
>                 result->separate = true;
>                 result->refcnt = 1;
> -               max = PATH_MAX;
> -               goto recopy;
> +               max = PATH_MAX - EMBEDDED_NAME_MAX;
> +               /* we can't simply add the number of rest chars we copy to len,
> +                * since strncpy_from_user may return negative to indicate
> +                * something is wrong, so we do the addition later, after
> +                * strncpy_from_user succeeds, and we know we already copy
> +                * EMBEDDED_NAME_MAX chars.
> +                */
> +               len = strncpy_from_user(kname + EMBEDDED_NAME_MAX,
> +                               filename + EMBEDDED_NAME_MAX, max);
> +
> +               if (unlikely(len < 0)) {
> +                       err = ERR_PTR(len);
> +                       goto error;
> +               }
> +               len += EMBEDDED_NAME_MAX;
>         }
>
>         /* The empty path is special. */
> @@ -209,28 +219,30 @@ struct filename *
>  getname_kernel(const char * filename)
>  {
>         struct filename *result;
> +       char *kname;
>         int len = strlen(filename) + 1;
>
> -       result = __getname();
> -       if (unlikely(!result))
> +       kname = __getname();
> +       if (unlikely(!kname))
>                 return ERR_PTR(-ENOMEM);
>
>         if (len <= EMBEDDED_NAME_MAX) {
> -               result->name = (char *)(result) + sizeof(*result);
> +               result = (struct filename *)(kname + EMBEDDED_NAME_MAX);
> +               result->name = kname;
>                 result->separate = false;
>         } else if (len <= PATH_MAX) {
>                 struct filename *tmp;
>
>                 tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
>                 if (unlikely(!tmp)) {
> -                       __putname(result);
> +                       __putname(kname);
>                         return ERR_PTR(-ENOMEM);
>                 }
> -               tmp->name = (char *)result;
> +               tmp->name = kname;
>                 tmp->separate = true;
>                 result = tmp;
>         } else {
> -               __putname(result);
> +               __putname(kname);
>                 return ERR_PTR(-ENAMETOOLONG);
>         }
>         memcpy((char *)result->name, filename, len);
> @@ -253,7 +265,7 @@ void putname(struct filename *name)
>                 __putname(name->name);
>                 kfree(name);
>         } else
> -               __putname(name);
> +               __putname(name->name);
>  }
>
>  static int check_acl(struct inode *inode, int mask)
> --
> 2.3.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/