Re: [PATCH v13 26/28] ovl: Re-check redirect xattr during inode initialization

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 29, 2018 at 10:38 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> So far redirect could be placed on directories only and now it can be
> placed on regular files as well. Also it could be completely removed
> when a metacopy copy up file's data is copied up. That means if a redirect
> is present during ovl_lookup(), it could be gone by the time ovl_get_inode()
> happens.
>

There is a bit of a mess in the assumptions.

If the inode is pure upper or indexed origin, than the alleged race ends up
in !(inode->i_state & I_NEW) and you discard redirect anyway.

If the inode is non-indexed copyup, then it is a different inode on disk
and different struct ovl_inode in memory than the inode of the copy up
we are allegedly racing with (they are broken hardlinks), so there is no
issue.

> Or it is possible that ovl_lookup() does not see a redirect and a rename
> is taking place on a hard link and that places a redirect. And by the
> time ovl_lookup() calls ovl_get_inode(), it sets ovl_inode->redirect = NULL
> (Assume inode got flushed out of cache and was allocated new).

Same as above.

I am not saying there are no races between lookup and rename/link,
but IMO the text above does not describe them or proves that they exist.

>
> IOW, because we check and process redirect without locks in ovl_lookup(),
> many possibilities open up for regular files. So for such cases, do not
> use the redirect provided by the caller. Instead query it and install
> in ovl_inode->redirect.
>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
>  fs/overlayfs/inode.c     | 19 ++++++++++++++++++-
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/util.c      | 42 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 3dccfa1ee123..6a0c85699024 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -694,6 +694,7 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
>         bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry, index);
>         bool is_dir, metacopy = false;
>         int err = -ENOMEM;
> +       char *new_redirect = NULL;
>
>         if (!realinode)
>                 realinode = d_inode(lowerdentry);
> @@ -754,7 +755,18 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
>         if (upperdentry && !metacopy)
>                 ovl_set_flag(OVL_UPPERDATA, inode);
>
> -       OVL_I(inode)->redirect = redirect;
> +       if (!metacopy) {
> +               OVL_I(inode)->redirect = redirect;
> +               redirect = NULL;
> +       } else if (upperdentry) {
> +               new_redirect = ovl_get_redirect_xattr(upperdentry);
> +               if (IS_ERR(new_redirect)) {
> +                       err = PTR_ERR(new_redirect);
> +                       goto out_err_inode;
> +               }
> +               OVL_I(inode)->redirect = new_redirect;
> +               new_redirect = NULL;
> +       }
>
>         /* Check for non-merge dir that may have whiteouts */
>         if (is_dir) {
> @@ -764,11 +776,16 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
>                 }
>         }
>
> +       kfree(redirect);
>         if (inode->i_state & I_NEW)
>                 unlock_new_inode(inode);
>  out:
>         return inode;
>
> +out_err_inode:
> +       if (inode->i_state & I_NEW)
> +               unlock_new_inode(inode);
> +       iput(inode);
>  out_err:
>         inode = ERR_PTR(err);
>         goto out;
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 429713653b3b..a3bee7619fbb 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -279,6 +279,7 @@ void ovl_nlink_end_locked(struct dentry *dentry);
>  int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
>  int ovl_check_metacopy_xattr(struct dentry *dentry);
>  bool ovl_is_metacopy_dentry(struct dentry *dentry);
> +char *ovl_get_redirect_xattr(struct dentry *dentry);
>
>  static inline bool ovl_is_impuredir(struct dentry *dentry)
>  {
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 961d65bd25c9..3d090b6f9fc2 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -833,3 +833,45 @@ bool ovl_is_metacopy_dentry(struct dentry *dentry)
>
>         return (oe->numlower > 1);
>  }
> +
> +char *ovl_get_redirect_xattr(struct dentry *dentry)
> +{
> +       int res;
> +       char *s, *next, *buf = NULL;
> +
> +       res = vfs_getxattr(dentry, OVL_XATTR_REDIRECT, NULL, 0);
> +       if (res < 0) {
> +               if (res == -ENODATA || res == -EOPNOTSUPP)
> +                       return NULL;
> +               return ERR_PTR(res);
> +       }
> +
> +       buf = kzalloc(res + 1, GFP_KERNEL);
> +       if (!buf)
> +               return ERR_PTR(-ENOMEM);
> +
> +       res = vfs_getxattr(dentry, OVL_XATTR_REDIRECT, buf, res);
> +       if (res < 0) {
> +               kfree(buf);
> +               return ERR_PTR(res);
> +        }
> +       if (res == 0)
> +               goto invalid;
> +
> +       if (buf[0] == '/') {
> +               for (s = buf; *s++ == '/'; s = next) {
> +                       next = strchrnul(s, '/');
> +                       if (s == next)
> +                               goto invalid;
> +               }
> +       } else {
> +               if (strchr(buf, '/') != NULL)
> +                       goto invalid;
> +       }
> +
> +       return buf;
> +invalid:
> +       pr_warn_ratelimited("overlayfs: invalid redirect (%s)\n", buf);
> +       kfree(buf);
> +       return ERR_PTR(-EINVAL);
> +}
> --
> 2.13.6
>

If you really end up needing this helper, you should use it from lookup as well.

Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux