On Fri, Mar 30, 2018 at 11:56:42AM +0300, Amir Goldstein wrote:
> 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.
Can't these also happen when I_NEW=true. I mean inode could be flushed
out of cache. Say one cpu is doing ovl_lookup() and thread got blocked
while other cpu did copy up of file on other cpu, removed redirect and
inode got flushed out of cache. Now cpu1 resumes execuction, creates
a new inode but it needs to re-check if redirect is still present or
not?
>
> 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.
Agreed that in case of broken hardlinks this race does not exist. But
do we really want to optimize it here?
>
> > 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.
>
I can try to give more details. But I think if inode gets flushed out
of cache, then we need to query redirect info again.
Vivek
> >
> > 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
