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: <E1P2rSX-0002hz-Lj@pomaz-ex.szeredi.hu>
Date:	Mon, 04 Oct 2010 22:16:29 +0200
From:	Miklos Szeredi <miklos@...redi.hu>
To:	Andy Whitcroft <apw@...onical.com>
CC:	miklos@...redi.hu, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: overlayfs: BUG in ovl_whiteout

On Mon, 4 Oct 2010, Andy Whitcroft wrote:
> On Mon, Oct 04, 2010 at 11:23:16AM +0200, Miklos Szeredi wrote:
> > On Fri, 1 Oct 2010, Andy Whitcroft wrote:
> > > I have been playing with overlayfs for a bit now and there seems to
> > > be something a little dodgy when unmounting and remounting an overlay.
> > > I modified some files in the overlay, immediatly unmounted the overlay and
> > > again immediatly remounted it.  On touching the same files in the overlay
> > > I triggered the following BUG (dmesg fragment at the bottom of this email).
> > > When trying to reproduce this I also triggered a hard hang.
> > 
> > Thanks for the bug report, Andy.
> > 
> > Please change that BUG_ON to a WARN_ON.  This will make this a
> > harmless error message in the log instead of a hang.
> 
> Applied your patch to move these to WARN_ON.  Now I get a WARN_ON after
> the remount.  Now that we don't explode I get the following error from
> the triggering command.  I assume that this is the rename which has
> triggered the issue:
> 
> W: Failed to fetch http://192.168.0.60:3142/archive.ubuntu.com/ubuntu/dists/maverick/Release rename failed, File exists (/var/lib/apt/lists/192.168.0.60:3142_archive.ubuntu.com_ubuntu_dists_maverick_Release -> /var/lib/apt/lists/192.168.0.60:3142_archive.ubuntu.com_ubuntu_dists_maverick_Release).
> 
> I note that the file does indeed exist as a real file in both layers.  I
> have not as yet managed to generate a naive test that also triggers
> this.

OK, my guess is that it's a 'rename to self' which was not properly
implemented.

Does the following patch make a difference?

Thanks,
Miklos

---
 fs/namei.c               |   19 ++++++++++++++-----
 fs/overlayfs/overlayfs.c |   19 ++++++++++++++++++-
 include/linux/fs.h       |    3 ++-
 3 files changed, 34 insertions(+), 7 deletions(-)

Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2010-10-04 19:40:20.000000000 +0200
+++ linux-2.6/fs/namei.c	2010-10-04 21:46:13.000000000 +0200
@@ -2620,6 +2620,17 @@ static int vfs_rename_other(struct inode
 	return error;
 }
 
+bool vfs_is_same_inode(struct dentry *d1, struct dentry *d2)
+{
+	BUG_ON(d1->d_sb != d2->d_sb);
+
+	if (d1->d_sb->s_op->is_same_inode)
+		return d1->d_sb->s_op->is_same_inode(d1, d2);
+	else
+		return d1->d_inode == d2->d_inode;
+}
+EXPORT_SYMBOL(vfs_is_same_inode);
+
 int vfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	       struct inode *new_dir, struct dentry *new_dentry)
 {
@@ -2627,11 +2638,9 @@ int vfs_rename(struct inode *old_dir, st
 	int is_dir = S_ISDIR(old_dentry->d_inode->i_mode);
 	const unsigned char *old_name;
 
-	if (old_dentry->d_inode == new_dentry->d_inode &&
-	    !(old_dir->i_sb->s_type->fs_flags & FS_RENAME_SELF_ALLOW)) {
- 		return 0;
-	}
- 
+	if (vfs_is_same_inode(old_dentry, new_dentry))
+		return 0;
+
 	error = may_delete(old_dir, old_dentry, is_dir);
 	if (error)
 		return error;
Index: linux-2.6/fs/overlayfs/overlayfs.c
===================================================================
--- linux-2.6.orig/fs/overlayfs/overlayfs.c	2010-10-04 19:40:20.000000000 +0200
+++ linux-2.6/fs/overlayfs/overlayfs.c	2010-10-04 21:00:05.000000000 +0200
@@ -1962,10 +1962,28 @@ static int ovl_statfs(struct dentry *den
 	return path.dentry->d_sb->s_op->statfs(path.dentry, buf);
 }
 
+static bool ovl_is_same_inode(struct dentry *d1, struct dentry *d2)
+{
+	struct dentry *upperd1;
+	struct dentry *upperd2;
+
+	upperd1 = ovl_dentry_upper(d1);
+	upperd2 = ovl_dentry_upper(d2);
+
+	if (upperd1 && upperd2)
+		return vfs_is_same_inode(upperd1, upperd2);
+
+	if (upperd1 || upperd2)
+		return false;
+
+	return vfs_is_same_inode(ovl_dentry_lower(d1), ovl_dentry_lower(d2));
+}
+
 static const struct super_operations ovl_super_operations = {
 	.put_super	= ovl_put_super,
 	.remount_fs	= ovl_remount_fs,
 	.statfs		= ovl_statfs,
+	.is_same_inode	= ovl_is_same_inode,
 };
 
 struct ovl_config {
@@ -2155,7 +2173,6 @@ static int ovl_get_sb(struct file_system
 static struct file_system_type ovl_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "overlayfs",
-	.fs_flags	= FS_RENAME_SELF_ALLOW,
 	.get_sb		= ovl_get_sb,
 	.kill_sb	= kill_anon_super,
 };
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2010-10-04 19:40:20.000000000 +0200
+++ linux-2.6/include/linux/fs.h	2010-10-04 21:47:40.000000000 +0200
@@ -179,7 +179,6 @@ struct inodes_stat_t {
 #define FS_RENAME_DOES_D_MOVE	32768	/* FS will handle d_move()
 					 * during rename() internally.
 					 */
-#define FS_RENAME_SELF_ALLOW	65536	/* Allow rename to same inode */
 
 /*
  * These are the fs-independent mount-flags: up to 32 flags are supported
@@ -1579,6 +1578,7 @@ struct super_operations {
 	ssize_t (*quota_write)(struct super_block *, int, const char *, size_t, loff_t);
 #endif
 	int (*bdev_try_to_free_page)(struct super_block*, struct page*, gfp_t);
+	bool (*is_same_inode)(struct dentry *, struct dentry *);
 };
 
 /*
@@ -1816,6 +1816,7 @@ extern int vfs_statfs(struct path *, str
 extern int statfs_by_dentry(struct dentry *, struct kstatfs *);
 extern int freeze_super(struct super_block *super);
 extern int thaw_super(struct super_block *super);
+extern bool vfs_is_same_inode(struct dentry *d1, struct dentry *d2);
 
 extern int current_umask(void);
 
--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ