ovl: untangle copy up call chain
Subsystems (2)
Amir Goldstein
maintainerPATCH 1/1 ovl: untangle copy up call chain
In an attempt to dedup ~100 LOC, we ended up creating a tangled call chain, whose branches merge and diverge in several points according to the immutable c->tmpfile copy up mode.
This call chain was hard to analyse for locking correctness because the locking requirements for the c->tmpfile flow were very different from the locking requirements for the !c->tmpfile flow (i.e. directory vs. regulare file copy up).
Split the copy up helpers of the c->tmpfile flow from those of the !c->tmpfile (i.e. workdir) flow and remove the c->tmpfile mode from copy up context.
---
Miklos,
This refactoring should not be changing logic.
There is one semantic correctness change which I did not bother to split:
< temp = ovl_do_tmpfile(c->workdir, c->stat.mode);
Quoted 1 line
temp = ovl_do_tmpfile(c->destdir, c->stat.mode);
It passed the quick xfstests.
I am a bit concerned about test coverage for upper fs with no O_TMPFILE support. I doubt anyone is testing that configuration??
In retrospect, we should not have allowed enabling new features without both O_TMPFILE and d_type support in upper fs.
Maybe it is not too later for that? If anybody is really running overlayfs on semi-supported upper fs, is it highly unlikely that they are using new features (does anyone?).
I will post a proposal.
Thanks,
Amir.
fs/overlayfs/copy_up.c | 148 ++++++++++++++++++++++++++++++----------- 1 file changed, 110 insertions(+), 38 deletions(-)
fs/overlayfs/copy_up.c +110 -38
@@ -395,7 +395,6 @@ struct ovl_copy_up_ctx {struct dentry *destdir;struct qstr destname;struct dentry *workdir; -bool tmpfile;bool origin;bool indexed;bool metacopy;
@@ -440,8 +439,14 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)return err; } -static int ovl_install_temp (struct ovl_copy_up_ctx *c,struct dentry *temp, -struct dentry **newdentry) +/* +* Move temp file from workdir into place on upper dir. +* Used when copying up directories and when upper fs doesn't support O_TMPFILE. +* +* Caller must hold ovl_lock_rename_workdir(). +*/ +static int ovl_rename_temp (struct ovl_copy_up_ctx *c,struct dentry *temp, +struct dentry **newdentry) {int err;struct dentry *upper;
@@ -451,19 +456,40 @@ static int ovl_install_temp(struct ovl_copy_up_ctx *c, struct dentry *temp,if (IS_ERR (upper))return PTR_ERR (upper); -if (c->tmpfile) - err =ovl_do_link (temp, udir, upper); -else - err =ovl_do_rename (d_inode (c->workdir), temp, udir, upper,0 ); - + err =ovl_do_rename (d_inode (c->workdir), temp, udir, upper,0 );if (!err) - *newdentry =dget (c->tmpfile ? upper : temp); + *newdentry =dget (temp);dput (upper);return err; } -static struct dentry *ovl_get_tmpfile (struct ovl_copy_up_ctx *c) +/* Link O_TMPFILE into place on upper dir */ +static int ovl_link_tmpfile (struct ovl_copy_up_ctx *c,struct dentry *temp, +struct dentry **newdentry) +{ +int err; +struct dentry *upper =NULL ; +struct inode *udir =d_inode (c->destdir); + +inode_lock_nested (udir,I_MUTEX_PARENT ); + upper =lookup_one_len (c->destname.name, c->destdir, c->destname.len); + err =PTR_ERR (upper); +if (IS_ERR (upper)) + goto out; + + err =ovl_do_link (temp, udir, upper); +if (err) + goto out; + *newdentry =dget (upper); +out: +dput (upper); +inode_unlock (udir); + +return err; +} + +static struct dentry *ovl_get_workdir_temp (struct ovl_copy_up_ctx *c) {int err;struct dentry *temp;
@@ -484,10 +510,32 @@ static struct dentry *ovl_get_tmpfile(struct ovl_copy_up_ctx *c)if (new_creds) old_creds =override_creds (new_creds); -if (c->tmpfile) - temp =ovl_do_tmpfile (c->workdir, c->stat.mode); -else - temp =ovl_create_temp (c->workdir, &cattr); + temp =ovl_create_temp (c->workdir, &cattr); +out: +if (new_creds) { +revert_creds (old_creds); +put_cred (new_creds); + } + +return temp; +} + +static struct dentry *ovl_get_tmpfile (struct ovl_copy_up_ctx *c) +{ +int err; +struct dentry *temp; +const struct cred *old_creds =NULL ; +struct cred *new_creds =NULL ; + + err =security_inode_copy_up (c->dentry, &new_creds); + temp =ERR_PTR (err); +if (err <0 ) + goto out; + +if (new_creds) + old_creds =override_creds (new_creds); + + temp =ovl_do_tmpfile (c->destdir, c->stat.mode); out:if (new_creds) {revert_creds (old_creds);
@@ -548,17 +596,25 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)return err; } -static int ovl_copy_up_locked (struct ovl_copy_up_ctx *c) +/* +* Copyup using workdir to prepare temp file. +* Used when copying up directories and when upper fs doesn't support O_TMPFILE. +*/ +static int ovl_copy_up_workdir (struct ovl_copy_up_ctx *c) { -struct inode *udir = c->destdir->d_inode;struct inode *inode;struct dentry *newdentry =NULL ;struct dentry *temp;int err; - temp =ovl_get_tmpfile (c); + err =ovl_lock_rename_workdir (c->workdir, c->destdir); +if (err) +return err; + + temp =ovl_get_workdir_temp (c); + err =PTR_ERR (temp);if (IS_ERR (temp)) -return PTR_ERR (temp); + goto out; err =ovl_copy_up_inode (c, temp);if (err)
@@ -570,13 +626,7 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c) goto out; } -if (c->tmpfile) { -inode_lock_nested (udir,I_MUTEX_PARENT ); - err =ovl_install_temp (c, temp, &newdentry); -inode_unlock (udir); - }else { - err =ovl_install_temp (c, temp, &newdentry); - } + err =ovl_rename_temp (c, temp, &newdentry);if (err) goto out;
@@ -588,13 +638,43 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)ovl_set_flag (OVL_WHITEOUTS , inode); out: -if (err && !c->tmpfile) +unlock_rename (c->workdir, c->destdir); +if (err)ovl_cleanup (d_inode (c->workdir), temp);dput (temp);return err; } +/* Copyup using O_TMPFILE which does not require cross dir locking */ +static int ovl_copy_up_tmpfile (struct ovl_copy_up_ctx *c) +{ +struct dentry *newdentry =NULL ; +struct dentry *temp; +int err; + + temp =ovl_get_tmpfile (c); +if (IS_ERR (temp)) +return PTR_ERR (temp); + + err =ovl_copy_up_inode (c, temp); +if (err) + goto out; + + err =ovl_link_tmpfile (c, temp, &newdentry); +if (err) + goto out; + +if (!c->metacopy) +ovl_set_upperdata (d_inode (c->dentry)); +ovl_inode_update (d_inode (c->dentry), newdentry); + +out: +dput (temp); +return err; + +} +/* * Copy up a single dentry *
@@ -646,18 +726,10 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c) }/* Should we copyup with O_TMPFILE or with workdir? */ -if (S_ISREG (c->stat.mode) && ofs->tmpfile) { - c->tmpfile = true; - err =ovl_copy_up_locked (c); - }else { - err =ovl_lock_rename_workdir (c->workdir, c->destdir); -if (!err) { - err =ovl_copy_up_locked (c); -unlock_rename (c->workdir, c->destdir); - } - } - - +if (S_ISREG (c->stat.mode) && ofs->tmpfile) + err =ovl_copy_up_tmpfile (c); +else + err =ovl_copy_up_workdir (c);if (err) goto out;
Signature · 1 line
2.17.1
Al Viro
maintainerfs/overlayfs/copy_up.c static int ovl_install_temp(struct ovl_copy_up_ctx *c, struct dentry *temp, · line 479
Quoted 12 lines · Amir Goldstein
fs/overlayfs/copy_up.c +12
-0+static int ovl_link_tmpfile (struct ovl_copy_up_ctx *c,struct dentry *temp, +struct dentry **newdentry) +{ +int err; +struct dentry *upper =NULL ; +struct inode *udir =d_inode (c->destdir); + +inode_lock_nested (udir,I_MUTEX_PARENT ); + upper =lookup_one_len (c->destname.name, c->destdir, c->destname.len); + err =PTR_ERR (upper); +if (IS_ERR (upper)) + goto out;
dput(ERR_PTR(something)) will immediately oops on you...
Amir Goldstein
maintainerQuoted 16 lines · Al Viro
fs/overlayfs/copy_up.c static int ovl_install_temp(struct ovl_copy_up_ctx *c, struct dentry *temp, · line 479
Quoted 12 lines · Amir Goldstein
fs/overlayfs/copy_up.c +12
-0+static int ovl_link_tmpfile (struct ovl_copy_up_ctx *c,struct dentry *temp, +struct dentry **newdentry) +{ +int err; +struct dentry *upper =NULL ; +struct inode *udir =d_inode (c->destdir); + +inode_lock_nested (udir,I_MUTEX_PARENT ); + upper =lookup_one_len (c->destname.name, c->destdir, c->destname.len); + err =PTR_ERR (upper); +if (IS_ERR (upper)) + goto out;dput(ERR_PTR(something)) will immediately oops on you...
And that's not the only braino in this patch:
+ unlock_rename(c->workdir, c->destdir);
+ if (err)
ovl_cleanup(d_inode(c->workdir), temp);
Thanks for the review,
Amir.