On Tue, 22 Mar 2011, Miklos Szeredi wrote:
> > What do you mean, before? It's not atomic... What happens if e.g.
> > you get
> >
> > A: decided to do copy_up_locked
> > blocked on i_mutex
> >
> > B: did copy_up
> > did rename(), complete with d_move()
> > did unlink() in new place
> >
> > A: got CPU back, got i_mutex
>
> Here it can check if the file was copied up or not. OK, I see the
> code doesn't quite get that right.
>
> Patch below would fix it, I think.
Patch is correct, after all. More cleanups in that area below, as
well as analysis of rename vs. copy up race.
Thanks for taking a look.
Miklos
diff --git a/fs/overlayfs/overlayfs.c b/fs/overlayfs/overlayfs.c
index e7fcbde..b97a481 100644
--- a/fs/overlayfs/overlayfs.c
+++ b/fs/overlayfs/overlayfs.c
@@ -1166,15 +1166,8 @@ static int ovl_copy_up_locked(struct dentry *upperdir, struct dentry *dentry,
newpath.mnt = ofs->upper_mnt;
newpath.dentry = ovl_upper_create(upperdir, dentry, stat, link);
- if (IS_ERR(newpath.dentry)) {
- err = PTR_ERR(newpath.dentry);
-
- /* Already copied up? */
- if (err == -EEXIST && ovl_path_type(dentry) != OVL_PATH_LOWER)
- return 0;
-
- return err;
- }
+ if (IS_ERR(newpath.dentry))
+ return PTR_ERR(newpath.dentry);
if (S_ISREG(stat->mode)) {
err = ovl_copy_up_data(lowerpath, &newpath, stat->size);
@@ -1218,6 +1211,21 @@ err_remove:
return err;
}
+/*
+ * Copy up a single dentry
+ *
+ * Directory renames only allowed on "pure upper" (already created on
+ * upper filesystem, never copied up). Directories which are on lower or
+ * are merged may not be renamed. For these -EXDEV is returned and
+ * userspace has to deal with it. This means, when copying up a
+ * directory we can rely on it and ancestors being stable.
+ *
+ * Non-directory renames start with copy up of source if necessary. The
+ * actual rename will only proceed once the copy up was successful. Copy
+ * up uses upper parent i_mutex for exclusion. Since rename can change
+ * d_parent it is possible that the copy up will lock the old parent. At
+ * that point the file will have already been copied up anyway.
+ */
static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
struct path *lowerpath, struct kstat *stat)
{
@@ -1264,13 +1272,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
old_cred = override_creds(override_cred);
mutex_lock_nested(&upperdir->d_inode->i_mutex, I_MUTEX_PARENT);
- /*
- * Using upper filesystem locking to protect against copy up
- * racing with rename (rename means the copy up was already
- * successful).
- */
- if (dentry->d_parent != parent) {
- WARN_ON((ovl_path_type(dentry) == OVL_PATH_LOWER));
+ if (ovl_path_type(dentry) != OVL_PATH_LOWER) {
err = 0;
} else {
err = ovl_copy_up_locked(upperdir, dentry, lowerpath,
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
