From 8b472d739b2ddd8ab7fb278874f696cd95b25a5e Mon Sep 17 00:00:00 2001 From: Curt Wohlgemuth Date: Sat, 3 Apr 2010 16:45:06 -0400 Subject: ext4: Fix possible lost inode write in no journal mode In the no-journal case, ext4_write_inode() will fetch the bh and call sync_dirty_buffer() on it. However, if the bh has already been written and the bh reclaimed for some other purpose, AND if the inode is the only one in the inode table block in use, then ext4_get_inode_loc() will not read the inode table block from disk, but as an optimization, fill the block with zero's assuming that its caller will copy in the on-disk version of the inode. This is not done by ext4_write_inode(), so the contents of the inode can simply get lost. The fix is to use __ext4_get_inode_loc() with in_mem set to 0, instead of ext4_get_inode_loc(). Long term the API needs to be fixed so it's obvious why latter is not safe. Addresses-Google-Bug: #2526446 Signed-off-by: Curt Wohlgemuth Signed-off-by: "Theodore Ts'o" --- fs/ext4/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/ext4/inode.c') diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 11119e07233b..87e3c70d0692 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -5374,7 +5374,7 @@ int ext4_write_inode(struct inode *inode, struct writeback_control *wbc) } else { struct ext4_iloc iloc; - err = ext4_get_inode_loc(inode, &iloc); + err = __ext4_get_inode_loc(inode, &iloc, 0); if (err) return err; if (wbc->sync_mode == WB_SYNC_ALL) -- cgit v1.2.3 From fd2dd9fbaf9e498ec63eef298921e36556f7214c Mon Sep 17 00:00:00 2001 From: Curt Wohlgemuth Date: Sat, 3 Apr 2010 17:44:16 -0400 Subject: ext4: Fix buffer head leaks after calls to ext4_get_inode_loc() Calls to ext4_get_inode_loc() returns with a reference to a buffer head in iloc->bh. The callers of this function in ext4_write_inode() when in no journal mode and in ext4_xattr_fiemap() don't release the buffer head after using it. Addresses-Google-Bug: #2548165 Signed-off-by: Curt Wohlgemuth Signed-off-by: "Theodore Ts'o" --- fs/ext4/extents.c | 1 + fs/ext4/inode.c | 1 + 2 files changed, 2 insertions(+) (limited to 'fs/ext4/inode.c') diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 94c8ee81f5e1..236b834b4ca8 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -3879,6 +3879,7 @@ static int ext4_xattr_fiemap(struct inode *inode, physical += offset; length = EXT4_SB(inode->i_sb)->s_inode_size - offset; flags |= FIEMAP_EXTENT_DATA_INLINE; + brelse(iloc.bh); } else { /* external block */ physical = EXT4_I(inode)->i_file_acl << blockbits; length = inode->i_sb->s_blocksize; diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 87e3c70d0692..ba1eee847e32 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -5385,6 +5385,7 @@ int ext4_write_inode(struct inode *inode, struct writeback_control *wbc) (unsigned long long)iloc.bh->b_blocknr); err = -EIO; } + brelse(iloc.bh); } return err; } -- cgit v1.2.3 From 12755627bdcddcdb30a1bfb9a09395a52b1d6838 Mon Sep 17 00:00:00 2001 From: Dmitry Monakhov Date: Thu, 8 Apr 2010 22:04:20 +0400 Subject: quota: unify quota init condition in setattr Quota must being initialized if size or uid/git changes requested. But initialization performed in two different places: in case of i_size file system is responsible for dquot init , but in case of uid/gid init will be called internally in dquot_transfer(). This ambiguity makes code harder to understand. Let's move this logic to one common helper function. Signed-off-by: Dmitry Monakhov Signed-off-by: Jan Kara --- fs/ext2/inode.c | 2 +- fs/ext3/inode.c | 2 +- fs/ext4/inode.c | 2 +- fs/jfs/file.c | 2 +- fs/ocfs2/file.c | 4 ++-- fs/quota/dquot.c | 5 ++--- fs/reiserfs/inode.c | 3 ++- fs/udf/file.c | 2 +- fs/ufs/truncate.c | 8 ++++---- include/linux/quotaops.h | 8 ++++++++ 10 files changed, 23 insertions(+), 15 deletions(-) (limited to 'fs/ext4/inode.c') diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c index b90c3bf6e9ba..527c46d9bc1f 100644 --- a/fs/ext2/inode.c +++ b/fs/ext2/inode.c @@ -1466,7 +1466,7 @@ int ext2_setattr(struct dentry *dentry, struct iattr *iattr) if (error) return error; - if (iattr->ia_valid & ATTR_SIZE) + if (is_quota_modification(inode, iattr)) dquot_initialize(inode); if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) || (iattr->ia_valid & ATTR_GID && iattr->ia_gid != inode->i_gid)) { diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c index ea33bdf0a300..735f0190ec2a 100644 --- a/fs/ext3/inode.c +++ b/fs/ext3/inode.c @@ -3151,7 +3151,7 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr) if (error) return error; - if (ia_valid & ATTR_SIZE) + if (is_quota_modification(inode, attr)) dquot_initialize(inode); if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) || (ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) { diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 81d605412844..3e0f6af9d08d 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -5425,7 +5425,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr) if (error) return error; - if (ia_valid & ATTR_SIZE) + if (is_quota_modification(inode, attr)) dquot_initialize(inode); if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) || (ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) { diff --git a/fs/jfs/file.c b/fs/jfs/file.c index 14ba982b3f24..85d9ec659225 100644 --- a/fs/jfs/file.c +++ b/fs/jfs/file.c @@ -98,7 +98,7 @@ int jfs_setattr(struct dentry *dentry, struct iattr *iattr) if (rc) return rc; - if (iattr->ia_valid & ATTR_SIZE) + if (is_quota_modification(inode, iattr)) dquot_initialize(inode); if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) || (iattr->ia_valid & ATTR_GID && iattr->ia_gid != inode->i_gid)) { diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c index f74f1400eccd..e127c53ec2e7 100644 --- a/fs/ocfs2/file.c +++ b/fs/ocfs2/file.c @@ -966,10 +966,10 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr) if (status) return status; + if (is_quota_modification(inode, attr)) + dquot_initialize(inode); size_change = S_ISREG(inode->i_mode) && attr->ia_valid & ATTR_SIZE; if (size_change) { - dquot_initialize(inode); - status = ocfs2_rw_lock(inode, 1); if (status < 0) { mlog_errno(status); diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index b1a5036560a9..1056a21f0300 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -1822,10 +1822,9 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr) mask |= 1 << GRPQUOTA; chid[GRPQUOTA] = iattr->ia_gid; } - if (sb_any_quota_active(inode->i_sb) && !IS_NOQUOTA(inode)) { - dquot_initialize(inode); + if (sb_any_quota_active(inode->i_sb) && !IS_NOQUOTA(inode)) return __dquot_transfer(inode, chid, mask); - } + return 0; } EXPORT_SYMBOL(dquot_transfer); diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c index dc2c65e04853..0f22fdaf54ac 100644 --- a/fs/reiserfs/inode.c +++ b/fs/reiserfs/inode.c @@ -3076,9 +3076,10 @@ int reiserfs_setattr(struct dentry *dentry, struct iattr *attr) ia_valid = attr->ia_valid &= ~(ATTR_KILL_SUID|ATTR_KILL_SGID); depth = reiserfs_write_lock_once(inode->i_sb); - if (attr->ia_valid & ATTR_SIZE) { + if (is_quota_modification(inode, attr)) dquot_initialize(inode); + if (attr->ia_valid & ATTR_SIZE) { /* version 2 items will be caught by the s_maxbytes check ** done for us in vmtruncate */ diff --git a/fs/udf/file.c b/fs/udf/file.c index 4b6a46ccbf46..6ebc043f3a2a 100644 --- a/fs/udf/file.c +++ b/fs/udf/file.c @@ -227,7 +227,7 @@ int udf_setattr(struct dentry *dentry, struct iattr *iattr) if (error) return error; - if (iattr->ia_valid & ATTR_SIZE) + if (is_quota_modification(inode, iattr)) dquot_initialize(inode); if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) || diff --git a/fs/ufs/truncate.c b/fs/ufs/truncate.c index ee8db3e77bfe..f294c44577dc 100644 --- a/fs/ufs/truncate.c +++ b/fs/ufs/truncate.c @@ -518,18 +518,18 @@ int ufs_setattr(struct dentry *dentry, struct iattr *attr) if (error) return error; + if (is_quota_modification(inode, attr)) + dquot_initialize(inode); + if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) || (ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) { error = dquot_transfer(inode, attr); if (error) return error; } - if (ia_valid & ATTR_SIZE && - attr->ia_size != i_size_read(inode)) { + if (ia_valid & ATTR_SIZE && attr->ia_size != inode->i_size) { loff_t old_i_size = inode->i_size; - dquot_initialize(inode); - error = vmtruncate(inode, attr->ia_size); if (error) return error; diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h index 82c70c42d035..8a7818764a67 100644 --- a/include/linux/quotaops.h +++ b/include/linux/quotaops.h @@ -14,6 +14,14 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb) return &sb->s_dquot; } +/* i_mutex must being held */ +static inline bool is_quota_modification(struct inode *inode, struct iattr *ia) +{ + return (ia->ia_valid & ATTR_SIZE && ia->ia_size != inode->i_size) || + (ia->ia_valid & ATTR_UID && ia->ia_uid != inode->i_uid) || + (ia->ia_valid & ATTR_GID && ia->ia_gid != inode->i_gid); +} + #if defined(CONFIG_QUOTA) /* -- cgit v1.2.3