linux-kernel - Re: [PATCH 0/6 v7] overlay filesystem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <E1Q2KuI-0004o5-NB@pomaz-ex.szeredi.hu>
Date:	Wed, 23 Mar 2011 11:03:14 +0100
From:	Miklos Szeredi <miklos@...redi.hu>
To:	viro@...IV.linux.org.uk
CC:	torvalds@...ux-foundation.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	apw@...onical.com, nbd@...nwrt.org, neilb@...e.de
Subject: Re: [PATCH 0/6 v7] overlay filesystem - request for inclusion

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/