linux-ext4 - [PATCH] ext4: Fix the locking with respect to ext3 to ext4 migrate.

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [day] [month] [year] [list]
Message-Id: <1205214413-8267-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
Date:	Tue, 11 Mar 2008 11:16:53 +0530
From:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
To:	cmm@...ibm.com, tytso@....edu
Cc:	linux-ext4@...r.kernel.org,
	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
Subject: [PATCH] ext4: Fix the locking with respect to ext3 to ext4 migrate.

Ext3 to Ext4 migrate takes inode->i_mutex lock to make sure write
and truncate of the original inode doesn't happen during the migrate.
But a write to mmap area mapping holes of the file can still happen.
Such writes results in new block allocation and changes to the i_data
field of inode. Add i_migrate_sem read write semaphore to protect against
this. The semaphore is taken in read mode when trying to write to the
mmap area. This happens only once via page_mkwrite call back. Taking
the semaphore in read mode ensures that multiple mmap write can still
happen. In the migrate code we take the semaphore in write mode.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@...ux.vnet.ibm.com>
---
 fs/ext4/inode.c           |   17 +++++++++++++----
 fs/ext4/migrate.c         |    7 +++++++
 fs/ext4/super.c           |    1 +
 include/linux/ext4_fs_i.h |   12 ++++++++++++
 4 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 059f2fc..4a61ce5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3496,15 +3496,24 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
 
 int ext4_page_mkwrite(struct vm_area_struct *vma, struct page *page)
 {
+	int retval;
+	struct inode *inode = vma->vm_file->f_path.dentry->d_inode;
 	/*
 	 * if ext4_get_block resulted in a split of an uninitialized extent,
 	 * in file system full case, we will have to take the journal write
 	 * access and zero out the page. The journal handle get initialized
 	 * in ext4_get_block.
 	 */
-	/* FIXME!! should we take inode->i_mutex ? Currently we can't because
-	 * it has a circular locking dependency with DIO. But migrate expect
-	 * i_mutex to ensure no i_data changes
+	/* we need to make sure we don't allow migrate to happen. This is
+	 * needed because a write to area mapping holes can result in block
+	 * allocation and update of i_data field. We can't take inode->i_mutex
+	 * here because it changes the locking order with rspect to directIO.
+	 * directIO expect i_mutex -> mmap_sem. We are here already holding
+	 * mmap_sem
 	 */
-	return block_page_mkwrite(vma, page, ext4_get_block);
+	down_read(&(EXT4_I(inode)->i_migrate_sem));
+	retval =  block_page_mkwrite(vma, page, ext4_get_block);
+	up_read(&(EXT4_I(inode)->i_migrate_sem));
+
+	return retval;
 }
diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
index 5c1e27d..a3fbd9f 100644
--- a/fs/ext4/migrate.c
+++ b/fs/ext4/migrate.c
@@ -508,6 +508,12 @@ int ext4_ext_migrate(struct inode *inode, struct file *filp,
 	 * switch the inode format to prevent read.
 	 */
 	mutex_lock(&(inode->i_mutex));
+	/*
+	 * prevent a mmap write to holes
+	 * i_mutex is ranked above i_migrate_sem.
+	 * The order is i_mutex -> mmap_sem -> i_migrate_sem
+	 */
+	down_write(&(EXT4_I(inode)->i_migrate_sem));
 	handle = ext4_journal_start(inode, 1);
 
 	ei = EXT4_I(inode);
@@ -593,6 +599,7 @@ err_out:
 	tmp_inode->i_nlink = 0;
 
 	ext4_journal_stop(handle);
+	up_write(&(EXT4_I(inode)->i_migrate_sem));
 	mutex_unlock(&(inode->i_mutex));
 
 	if (tmp_inode)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index bc385c4..5d6e9b2 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -599,6 +599,7 @@ static void init_once(struct kmem_cache *cachep, void *foo)
 #endif
 	init_rwsem(&ei->i_data_sem);
 	inode_init_once(&ei->vfs_inode);
+	init_rwsem(&ei->i_migrate_sem);
 }
 
 static int init_inodecache(void)
diff --git a/include/linux/ext4_fs_i.h b/include/linux/ext4_fs_i.h
index d5508d3..96c0b4f 100644
--- a/include/linux/ext4_fs_i.h
+++ b/include/linux/ext4_fs_i.h
@@ -162,6 +162,18 @@ struct ext4_inode_info {
 	/* mballoc */
 	struct list_head i_prealloc_list;
 	spinlock_t i_prealloc_lock;
+
+	/* When doing migrate we need to ensure that the i_data field
+	 * doesn't change. With respect to write and truncate we can ensure
+	 * the same by taking inode->i_mutex. But a write to mmap area
+	 * mapping holes doesn't take i_mutex since it doesn't change the
+	 * i_size. We also can't take i_data_sem because we would like to
+	 * extend/restart the journal and locking order prevents us from
+	 * restarting journal within i_data_sem. This will be taken in
+	 * page_mkwrite in the read mode and migrate will take it in the
+	 * write mode.
+	 */
+	struct rw_semaphore i_migrate_sem;
 };
 
 #endif	/* _LINUX_EXT4_FS_I */
-- 
1.5.4.4.481.g5075.dirty

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html