[PATCH 0/11] ext4: Fix inode expansion code

[PATCH 0/11] ext4: Fix inode expansion code

[Jan Kara]

Hello,

this patch series fixes code expanding size of inode. That code has been
seriously buggy and I think it has never worked for other case than expanding
legacy 128-byte inodes to larger ones. This patch series fixes all the issues
I have found and passes xfstest test I have written to excercise the code.

First four patches in the series fix the most serious issues leading to
crashes, memory, and possibly filesystem corruption. I have tagged these for
stable kernels since 4.4 (where the new inode field has been introduced
and started triggering issues in the inode expansion code).

The fifth patch also fixes a functional issue where we may unnecessarily fail
to expand inode, or may move too many xattrs but this issue is not so serious
and I don't expect it to happen too much in practice (since we usually expand
only by a couple of bytes) so I didn't tag it for stable.

Remaining patches in the series cleanup the code by removing bogus checks,
unused variables, and factoring out the large inode expanding function into
several smaller ones.

								Honza

[PATCH 01/11] ext4: Fix xattr shifting when expanding inodes

[Jan Kara]

The code in ext4_expand_extra_isize_ea() treated new_extra_isize
argument sometimes as the desired target i_extra_isize and sometimes as
the amount by which we need to grow current i_extra_isize. These happen
to coincide when i_extra_isize is 0 which used to be the common case and
so nobody noticed this until recently when we added i_projid to the
inode and so i_extra_isize now needs to grow from 28 to 32 bytes.

The result of these bugs was that we sometimes unnecessarily decided to
move xattrs out of inode even if there was enough space and we often
ended up corrupting in-inode xattrs because arguments to
ext4_xattr_shift_entries() were just wrong. This could demonstrate
itself as BUG_ON in ext4_xattr_shift_entries() triggering.

Fix the problem by introducing new isize_diff variable and use it where
appropriate.

CC: <stable@vger.kernel.org> # 4.4.x-
Reported-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/xattr.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 39e9cfb1b371..cb1d7b4482de 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1353,11 +1353,13 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 	size_t min_offs, free;
 	int total_ino;
 	void *base, *start, *end;
-	int extra_isize = 0, error = 0, tried_min_extra_isize = 0;
+	int error = 0, tried_min_extra_isize = 0;
 	int s_min_extra_isize = le16_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_min_extra_isize);
+	int isize_diff;	/* How much do we need to grow i_extra_isize */
 
 	down_write(&EXT4_I(inode)->xattr_sem);
 retry:
+	isize_diff = new_extra_isize - EXT4_I(inode)->i_extra_isize;
 	if (EXT4_I(inode)->i_extra_isize >= new_extra_isize) {
 		up_write(&EXT4_I(inode)->xattr_sem);
 		return 0;
@@ -1382,7 +1384,7 @@ retry:
 		goto cleanup;
 
 	free = ext4_xattr_free_space(last, &min_offs, base, &total_ino);
-	if (free >= new_extra_isize) {
+	if (free >= isize_diff) {
 		entry = IFIRST(header);
 		ext4_xattr_shift_entries(entry,	EXT4_I(inode)->i_extra_isize
 				- new_extra_isize, (void *)raw_inode +
@@ -1414,7 +1416,7 @@ retry:
 		end = bh->b_data + bh->b_size;
 		min_offs = end - base;
 		free = ext4_xattr_free_space(first, &min_offs, base, NULL);
-		if (free < new_extra_isize) {
+		if (free < isize_diff) {
 			if (!tried_min_extra_isize && s_min_extra_isize) {
 				tried_min_extra_isize++;
 				new_extra_isize = s_min_extra_isize;
@@ -1428,7 +1430,7 @@ retry:
 		free = inode->i_sb->s_blocksize;
 	}
 
-	while (new_extra_isize > 0) {
+	while (isize_diff > 0) {
 		size_t offs, size, entry_size;
 		struct ext4_xattr_entry *small_entry = NULL;
 		struct ext4_xattr_info i = {
@@ -1459,7 +1461,7 @@ retry:
 			EXT4_XATTR_SIZE(le32_to_cpu(last->e_value_size)) +
 					EXT4_XATTR_LEN(last->e_name_len);
 			if (total_size <= free && total_size < min_total_size) {
-				if (total_size < new_extra_isize) {
+				if (total_size < isize_diff) {
 					small_entry = last;
 				} else {
 					entry = last;
@@ -1516,20 +1518,19 @@ retry:
 			goto cleanup;
 
 		entry = IFIRST(header);
-		if (entry_size + EXT4_XATTR_SIZE(size) >= new_extra_isize)
-			shift_bytes = new_extra_isize;
+		if (entry_size + EXT4_XATTR_SIZE(size) >= isize_diff)
+			shift_bytes = isize_diff;
 		else
 			shift_bytes = entry_size + size;
 		/* Adjust the offsets and shift the remaining entries ahead */
-		ext4_xattr_shift_entries(entry, EXT4_I(inode)->i_extra_isize -
-			shift_bytes, (void *)raw_inode +
-			EXT4_GOOD_OLD_INODE_SIZE + extra_isize + shift_bytes,
+		ext4_xattr_shift_entries(entry, -shift_bytes,
+			(void *)raw_inode + EXT4_GOOD_OLD_INODE_SIZE +
+			EXT4_I(inode)->i_extra_isize + shift_bytes,
 			(void *)header, total_ino - entry_size,
 			inode->i_sb->s_blocksize);
 
-		extra_isize += shift_bytes;
-		new_extra_isize -= shift_bytes;
-		EXT4_I(inode)->i_extra_isize = extra_isize;
+		isize_diff -= shift_bytes;
+		EXT4_I(inode)->i_extra_isize += shift_bytes;
 
 		i.name = b_entry_name;
 		i.value = buffer;
-- 
2.6.6

[PATCH 02/11] ext4: Fix xattr shifting when expanding inodes (2)

[Jan Kara]

When multiple xattrs need to be moved out of inode, we did not properly
recompute total size of xattr headers in the inode and the new header
position. Thus when moving the second and further xattr we asked
ext4_xattr_shift_entries() to move too much and from the wrong place,
resulting in possible xattr value corruption or general memory
corruption.

CC: <stable@vger.kernel.org> # 4.4.x-
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/xattr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index cb1d7b4482de..b18b1ff7cc27 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1516,6 +1516,7 @@ retry:
 		error = ext4_xattr_ibody_set(handle, inode, &i, is);
 		if (error)
 			goto cleanup;
+		total_ino -= entry_size;
 
 		entry = IFIRST(header);
 		if (entry_size + EXT4_XATTR_SIZE(size) >= isize_diff)
@@ -1526,11 +1527,11 @@ retry:
 		ext4_xattr_shift_entries(entry, -shift_bytes,
 			(void *)raw_inode + EXT4_GOOD_OLD_INODE_SIZE +
 			EXT4_I(inode)->i_extra_isize + shift_bytes,
-			(void *)header, total_ino - entry_size,
-			inode->i_sb->s_blocksize);
+			(void *)header, total_ino, inode->i_sb->s_blocksize);
 
 		isize_diff -= shift_bytes;
 		EXT4_I(inode)->i_extra_isize += shift_bytes;
+		header = IHDR(inode, raw_inode);
 
 		i.name = b_entry_name;
 		i.value = buffer;
-- 
2.6.6

[PATCH 03/11] ext4: Properly align shifted xattrs when expanding inodes

[Jan Kara]

We did not count with the padding of xattr value when computing desired
shift of xattrs in the inode when expanding i_extra_isize. As a result
we could create unaligned start of inline xattrs. Account for alignment
properly.

CC: <stable@vger.kernel.org> # 4.4.x-
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/xattr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index b18b1ff7cc27..c893f00b6dc0 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1522,7 +1522,7 @@ retry:
 		if (entry_size + EXT4_XATTR_SIZE(size) >= isize_diff)
 			shift_bytes = isize_diff;
 		else
-			shift_bytes = entry_size + size;
+			shift_bytes = entry_size + EXT4_XATTR_SIZE(size);
 		/* Adjust the offsets and shift the remaining entries ahead */
 		ext4_xattr_shift_entries(entry, -shift_bytes,
 			(void *)raw_inode + EXT4_GOOD_OLD_INODE_SIZE +
-- 
2.6.6

[PATCH 04/11] ext4: Avoid deadlock when expanding inode size

[Jan Kara]

When we need to move xattrs into external xattr block, we call
ext4_xattr_block_set() from ext4_expand_extra_isize_ea(). That may end
up calling ext4_mark_inode_dirty() again which will recurse back into
the inode expansion code leading to deadlocks.

Protect from recursion using EXT4_STATE_NO_EXPAND inode flag and move
its management into ext4_expand_extra_isize_ea() since its manipulation
is safe there (due to xattr_sem) from possible races with
ext4_xattr_set_handle() which plays with it as well.

CC: <stable@vger.kernel.org> # 4.4.x-
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c |  2 --
 fs/ext4/xattr.c | 19 +++++++++++++------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3131747199e1..c6ea25a190f8 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5466,8 +5466,6 @@ int ext4_mark_inode_dirty(handle_t *handle, struct inode *inode)
 						      sbi->s_want_extra_isize,
 						      iloc, handle);
 			if (ret) {
-				ext4_set_inode_state(inode,
-						     EXT4_STATE_NO_EXPAND);
 				if (mnt_count !=
 					le16_to_cpu(sbi->s_es->s_mnt_count)) {
 					ext4_warning(inode->i_sb,
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index c893f00b6dc0..2eb935ca5d9e 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1358,12 +1358,14 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 	int isize_diff;	/* How much do we need to grow i_extra_isize */
 
 	down_write(&EXT4_I(inode)->xattr_sem);
+	/*
+	 * Set EXT4_STATE_NO_EXPAND to avoid recursion when marking inode dirty
+	 */
+	ext4_set_inode_state(inode, EXT4_STATE_NO_EXPAND);
 retry:
 	isize_diff = new_extra_isize - EXT4_I(inode)->i_extra_isize;
-	if (EXT4_I(inode)->i_extra_isize >= new_extra_isize) {
-		up_write(&EXT4_I(inode)->xattr_sem);
-		return 0;
-	}
+	if (EXT4_I(inode)->i_extra_isize >= new_extra_isize)
+		goto out;
 
 	header = IHDR(inode, raw_inode);
 	entry = IFIRST(header);
@@ -1392,8 +1394,7 @@ retry:
 				(void *)header, total_ino,
 				inode->i_sb->s_blocksize);
 		EXT4_I(inode)->i_extra_isize = new_extra_isize;
-		error = 0;
-		goto cleanup;
+		goto out;
 	}
 
 	/*
@@ -1553,6 +1554,8 @@ retry:
 		kfree(bs);
 	}
 	brelse(bh);
+out:
+	ext4_clear_inode_state(inode, EXT4_STATE_NO_EXPAND);
 	up_write(&EXT4_I(inode)->xattr_sem);
 	return 0;
 
@@ -1564,6 +1567,10 @@ cleanup:
 	kfree(is);
 	kfree(bs);
 	brelse(bh);
+	/*
+	 * We deliberately leave EXT4_STATE_NO_EXPAND set here since inode
+	 * size expansion failed.
+	 */
 	up_write(&EXT4_I(inode)->xattr_sem);
 	return error;
 }
-- 
2.6.6

[PATCH 05/11] ext4: Fixup free space calculations when expanding inodes

[Jan Kara]

Conditions checking whether there is enough free space in an xattr block
and when xattr is large enough to make enough space in the inode forgot
to account for the fact that inode need not be completely filled up with
xattrs. Thus we could move unnecessarily many xattrs out of inode or
even falsely claim there is not enough space to expand the inode. We
also forgot to update the amount of free space in xattr block when moving
more xattrs and thus could decide to move too big xattr resulting in
unexpected failure.

Fix these problems by properly updating free space in the inode and
xattr block as we move xattrs. To simplify the math, avoid shifting
xattrs after removing each one xattr and instead just shift xattrs only
once there is enough free space in the inode.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/xattr.c | 58 ++++++++++++++++++++++++---------------------------------
 1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 2eb935ca5d9e..22d2ebcd1f09 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1350,7 +1350,8 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 	struct ext4_xattr_ibody_find *is = NULL;
 	struct ext4_xattr_block_find *bs = NULL;
 	char *buffer = NULL, *b_entry_name = NULL;
-	size_t min_offs, free;
+	size_t min_offs;
+	size_t ifree, bfree;
 	int total_ino;
 	void *base, *start, *end;
 	int error = 0, tried_min_extra_isize = 0;
@@ -1385,17 +1386,9 @@ retry:
 	if (error)
 		goto cleanup;
 
-	free = ext4_xattr_free_space(last, &min_offs, base, &total_ino);
-	if (free >= isize_diff) {
-		entry = IFIRST(header);
-		ext4_xattr_shift_entries(entry,	EXT4_I(inode)->i_extra_isize
-				- new_extra_isize, (void *)raw_inode +
-				EXT4_GOOD_OLD_INODE_SIZE + new_extra_isize,
-				(void *)header, total_ino,
-				inode->i_sb->s_blocksize);
-		EXT4_I(inode)->i_extra_isize = new_extra_isize;
-		goto out;
-	}
+	ifree = ext4_xattr_free_space(last, &min_offs, base, &total_ino);
+	if (ifree >= isize_diff)
+		goto shift;
 
 	/*
 	 * Enough free space isn't available in the inode, check if
@@ -1416,8 +1409,8 @@ retry:
 		first = BFIRST(bh);
 		end = bh->b_data + bh->b_size;
 		min_offs = end - base;
-		free = ext4_xattr_free_space(first, &min_offs, base, NULL);
-		if (free < isize_diff) {
+		bfree = ext4_xattr_free_space(first, &min_offs, base, NULL);
+		if (bfree + ifree < isize_diff) {
 			if (!tried_min_extra_isize && s_min_extra_isize) {
 				tried_min_extra_isize++;
 				new_extra_isize = s_min_extra_isize;
@@ -1428,10 +1421,10 @@ retry:
 			goto cleanup;
 		}
 	} else {
-		free = inode->i_sb->s_blocksize;
+		bfree = inode->i_sb->s_blocksize;
 	}
 
-	while (isize_diff > 0) {
+	while (isize_diff > ifree) {
 		size_t offs, size, entry_size;
 		struct ext4_xattr_entry *small_entry = NULL;
 		struct ext4_xattr_info i = {
@@ -1439,7 +1432,6 @@ retry:
 			.value_len = 0,
 		};
 		unsigned int total_size;  /* EA entry size + value size */
-		unsigned int shift_bytes; /* No. of bytes to shift EAs by? */
 		unsigned int min_total_size = ~0U;
 
 		is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_NOFS);
@@ -1461,8 +1453,9 @@ retry:
 			total_size =
 			EXT4_XATTR_SIZE(le32_to_cpu(last->e_value_size)) +
 					EXT4_XATTR_LEN(last->e_name_len);
-			if (total_size <= free && total_size < min_total_size) {
-				if (total_size < isize_diff) {
+			if (total_size <= bfree &&
+			    total_size < min_total_size) {
+				if (total_size + ifree < isize_diff) {
 					small_entry = last;
 				} else {
 					entry = last;
@@ -1491,6 +1484,7 @@ retry:
 		offs = le16_to_cpu(entry->e_value_offs);
 		size = le32_to_cpu(entry->e_value_size);
 		entry_size = EXT4_XATTR_LEN(entry->e_name_len);
+		total_size = entry_size + EXT4_XATTR_SIZE(size);
 		i.name_index = entry->e_name_index,
 		buffer = kmalloc(EXT4_XATTR_SIZE(size), GFP_NOFS);
 		b_entry_name = kmalloc(entry->e_name_len + 1, GFP_NOFS);
@@ -1518,21 +1512,8 @@ retry:
 		if (error)
 			goto cleanup;
 		total_ino -= entry_size;
-
-		entry = IFIRST(header);
-		if (entry_size + EXT4_XATTR_SIZE(size) >= isize_diff)
-			shift_bytes = isize_diff;
-		else
-			shift_bytes = entry_size + EXT4_XATTR_SIZE(size);
-		/* Adjust the offsets and shift the remaining entries ahead */
-		ext4_xattr_shift_entries(entry, -shift_bytes,
-			(void *)raw_inode + EXT4_GOOD_OLD_INODE_SIZE +
-			EXT4_I(inode)->i_extra_isize + shift_bytes,
-			(void *)header, total_ino, inode->i_sb->s_blocksize);
-
-		isize_diff -= shift_bytes;
-		EXT4_I(inode)->i_extra_isize += shift_bytes;
-		header = IHDR(inode, raw_inode);
+		ifree += total_size;
+		bfree -= total_size;
 
 		i.name = b_entry_name;
 		i.value = buffer;
@@ -1553,6 +1534,15 @@ retry:
 		kfree(is);
 		kfree(bs);
 	}
+
+shift:
+	/* Adjust the offsets and shift the remaining entries ahead */
+	entry = IFIRST(header);
+	ext4_xattr_shift_entries(entry,	EXT4_I(inode)->i_extra_isize
+			- new_extra_isize, (void *)raw_inode +
+			EXT4_GOOD_OLD_INODE_SIZE + new_extra_isize,
+			(void *)header, total_ino, inode->i_sb->s_blocksize);
+	EXT4_I(inode)->i_extra_isize = new_extra_isize;
 	brelse(bh);
 out:
 	ext4_clear_inode_state(inode, EXT4_STATE_NO_EXPAND);
-- 
2.6.6

[PATCH 06/11] ext4: Check that external xattr value block is 0

[Jan Kara]

Currently we don't support xattrs with values stored out of line. Check
for that in ext4_xattr_check_names() to make sure we never work with
such xattrs since not all the code counts with that resulting is possible
weird corruption issues.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/xattr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 22d2ebcd1f09..f845cb7c6623 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -199,6 +199,8 @@ ext4_xattr_check_names(struct ext4_xattr_entry *entry, void *end,
 	}
 
 	while (!IS_LAST_ENTRY(entry)) {
+		if (entry->e_value_block != 0)
+			return -EFSCORRUPTED;
 		if (entry->e_value_size != 0 &&
 		    (value_start + le16_to_cpu(entry->e_value_offs) <
 		     (void *)e + sizeof(__u32) ||
-- 
2.6.6

[PATCH 07/11] ext4: Remove checks for e_value_block

[Jan Kara]

Currently we don't support xattrs with e_value_block set. We don't allow
them to pass initial xattr check so there's no point for checking for
this later. Since these tests were untested, bugs were creeping in and
not all places which should have checked were checking e_value_block
anyway.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/xattr.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index f845cb7c6623..1447860b61ec 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -643,7 +643,7 @@ static size_t ext4_xattr_free_space(struct ext4_xattr_entry *last,
 				    size_t *min_offs, void *base, int *total)
 {
 	for (; !IS_LAST_ENTRY(last); last = EXT4_XATTR_NEXT(last)) {
-		if (!last->e_value_block && last->e_value_size) {
+		if (last->e_value_size) {
 			size_t offs = le16_to_cpu(last->e_value_offs);
 			if (offs < *min_offs)
 				*min_offs = offs;
@@ -663,7 +663,7 @@ ext4_xattr_set_entry(struct ext4_xattr_info *i, struct ext4_xattr_search *s)
 	/* Compute min_offs and last. */
 	last = s->first;
 	for (; !IS_LAST_ENTRY(last); last = EXT4_XATTR_NEXT(last)) {
-		if (!last->e_value_block && last->e_value_size) {
+		if (last->e_value_size) {
 			size_t offs = le16_to_cpu(last->e_value_offs);
 			if (offs < min_offs)
 				min_offs = offs;
@@ -671,7 +671,7 @@ ext4_xattr_set_entry(struct ext4_xattr_info *i, struct ext4_xattr_search *s)
 	}
 	free = min_offs - ((void *)last - s->base) - sizeof(__u32);
 	if (!s->not_found) {
-		if (!s->here->e_value_block && s->here->e_value_size) {
+		if (s->here->e_value_size) {
 			size_t size = le32_to_cpu(s->here->e_value_size);
 			free += EXT4_XATTR_SIZE(size);
 		}
@@ -693,7 +693,7 @@ ext4_xattr_set_entry(struct ext4_xattr_info *i, struct ext4_xattr_search *s)
 		s->here->e_name_len = name_len;
 		memcpy(s->here->e_name, i->name, name_len);
 	} else {
-		if (!s->here->e_value_block && s->here->e_value_size) {
+		if (s->here->e_value_size) {
 			void *first_val = s->base + min_offs;
 			size_t offs = le16_to_cpu(s->here->e_value_offs);
 			void *val = s->base + offs;
@@ -727,8 +727,7 @@ ext4_xattr_set_entry(struct ext4_xattr_info *i, struct ext4_xattr_search *s)
 			last = s->first;
 			while (!IS_LAST_ENTRY(last)) {
 				size_t o = le16_to_cpu(last->e_value_offs);
-				if (!last->e_value_block &&
-				    last->e_value_size && o < offs)
+				if (last->e_value_size && o < offs)
 					last->e_value_offs =
 						cpu_to_le16(o + size);
 				last = EXT4_XATTR_NEXT(last);
@@ -1327,7 +1326,7 @@ static void ext4_xattr_shift_entries(struct ext4_xattr_entry *entry,
 
 	/* Adjust the value offsets of the entries */
 	for (; !IS_LAST_ENTRY(last); last = EXT4_XATTR_NEXT(last)) {
-		if (!last->e_value_block && last->e_value_size) {
+		if (last->e_value_size) {
 			new_offs = le16_to_cpu(last->e_value_offs) +
 							value_offs_shift;
 			BUG_ON(new_offs + le32_to_cpu(last->e_value_size)
@@ -1726,7 +1725,7 @@ static inline void ext4_xattr_hash_entry(struct ext4_xattr_header *header,
 		       *name++;
 	}
 
-	if (entry->e_value_block == 0 && entry->e_value_size != 0) {
+	if (entry->e_value_size != 0) {
 		__le32 *value = (__le32 *)((char *)header +
 			le16_to_cpu(entry->e_value_offs));
 		for (n = (le32_to_cpu(entry->e_value_size) +
-- 
2.6.6

[PATCH 08/11] ext4: Replace bogus assertion in ext4_xattr_shift_entries()

[Jan Kara]

We were checking whether computed offsets do not exceed end of block in
ext4_xattr_shift_entries(). However this does not make sense since we
always only decrease offsets. So replace that assertion with a check
whether we really decrease xattrs value offsets.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/xattr.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 1447860b61ec..82b025c977fc 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1319,18 +1319,19 @@ retry:
  */
 static void ext4_xattr_shift_entries(struct ext4_xattr_entry *entry,
 				     int value_offs_shift, void *to,
-				     void *from, size_t n, int blocksize)
+				     void *from, size_t n)
 {
 	struct ext4_xattr_entry *last = entry;
 	int new_offs;
 
+	/* We always shift xattr headers further thus offsets get lower */
+	BUG_ON(value_offs_shift > 0);
+
 	/* Adjust the value offsets of the entries */
 	for (; !IS_LAST_ENTRY(last); last = EXT4_XATTR_NEXT(last)) {
 		if (last->e_value_size) {
 			new_offs = le16_to_cpu(last->e_value_offs) +
 							value_offs_shift;
-			BUG_ON(new_offs + le32_to_cpu(last->e_value_size)
-				 > blocksize);
 			last->e_value_offs = cpu_to_le16(new_offs);
 		}
 	}
@@ -1542,7 +1543,7 @@ shift:
 	ext4_xattr_shift_entries(entry,	EXT4_I(inode)->i_extra_isize
 			- new_extra_isize, (void *)raw_inode +
 			EXT4_GOOD_OLD_INODE_SIZE + new_extra_isize,
-			(void *)header, total_ino, inode->i_sb->s_blocksize);
+			(void *)header, total_ino);
 	EXT4_I(inode)->i_extra_isize = new_extra_isize;
 	brelse(bh);
 out:
-- 
2.6.6

[PATCH 09/11] ext4: Factor out xattr moving

[Jan Kara]

Factor out function for moving xattrs from inode into external xattr
block from ext4_expand_extra_isize_ea(). That function is already quite
long and factoring out this rather standalone functionality helps
readability.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/xattr.c | 159 ++++++++++++++++++++++++++++++--------------------------
 1 file changed, 85 insertions(+), 74 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 82b025c977fc..8f582ae1032d 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1340,6 +1340,84 @@ static void ext4_xattr_shift_entries(struct ext4_xattr_entry *entry,
 }
 
 /*
+ * Move xattr pointed to by 'entry' from inode into external xattr block
+ */
+static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
+				    struct ext4_inode *raw_inode,
+				    struct ext4_xattr_entry *entry)
+{
+	struct ext4_xattr_ibody_find *is = NULL;
+	struct ext4_xattr_block_find *bs = NULL;
+	char *buffer = NULL, *b_entry_name = NULL;
+	size_t value_offs, value_size;
+	struct ext4_xattr_info i = {
+		.value = NULL,
+		.value_len = 0,
+		.name_index = entry->e_name_index,
+	};
+	struct ext4_xattr_ibody_header *header = IHDR(inode, raw_inode);
+	int error;
+
+	value_offs = le16_to_cpu(entry->e_value_offs);
+	value_size = le32_to_cpu(entry->e_value_size);
+
+	is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_NOFS);
+	bs = kzalloc(sizeof(struct ext4_xattr_block_find), GFP_NOFS);
+	buffer = kmalloc(value_size, GFP_NOFS);
+	b_entry_name = kmalloc(entry->e_name_len + 1, GFP_NOFS);
+	if (!is || !bs || !buffer || !b_entry_name) {
+		error = -ENOMEM;
+		goto out;
+	}
+
+	is->s.not_found = -ENODATA;
+	bs->s.not_found = -ENODATA;
+	is->iloc.bh = NULL;
+	bs->bh = NULL;
+
+	/* Save the entry name and the entry value */
+	memcpy(buffer, (void *)IFIRST(header) + value_offs, value_size);
+	memcpy(b_entry_name, entry->e_name, entry->e_name_len);
+	b_entry_name[entry->e_name_len] = '\0';
+	i.name = b_entry_name;
+
+	error = ext4_get_inode_loc(inode, &is->iloc);
+	if (error)
+		goto out;
+
+	error = ext4_xattr_ibody_find(inode, &i, is);
+	if (error)
+		goto out;
+
+	/* Remove the chosen entry from the inode */
+	error = ext4_xattr_ibody_set(handle, inode, &i, is);
+	if (error)
+		goto out;
+
+	i.name = b_entry_name;
+	i.value = buffer;
+	i.value_len = value_size;
+	error = ext4_xattr_block_find(inode, &i, bs);
+	if (error)
+		goto out;
+
+	/* Add entry which was removed from the inode into the block */
+	error = ext4_xattr_block_set(handle, inode, &i, bs);
+	if (error)
+		goto out;
+	error = 0;
+out:
+	kfree(b_entry_name);
+	kfree(buffer);
+	if (is)
+		brelse(is->iloc.bh);
+	kfree(is);
+	kfree(bs);
+
+	return error;
+}
+
+/*
  * Expand an inode by new_extra_isize bytes when EAs are present.
  * Returns 0 on success or negative error number on failure.
  */
@@ -1349,9 +1427,6 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 	struct ext4_xattr_ibody_header *header;
 	struct ext4_xattr_entry *entry, *last, *first;
 	struct buffer_head *bh = NULL;
-	struct ext4_xattr_ibody_find *is = NULL;
-	struct ext4_xattr_block_find *bs = NULL;
-	char *buffer = NULL, *b_entry_name = NULL;
 	size_t min_offs;
 	size_t ifree, bfree;
 	int total_ino;
@@ -1427,27 +1502,11 @@ retry:
 	}
 
 	while (isize_diff > ifree) {
-		size_t offs, size, entry_size;
 		struct ext4_xattr_entry *small_entry = NULL;
-		struct ext4_xattr_info i = {
-			.value = NULL,
-			.value_len = 0,
-		};
-		unsigned int total_size;  /* EA entry size + value size */
+		unsigned int entry_size;	/* EA entry size */
+		unsigned int total_size;	/* EA entry size + value size */
 		unsigned int min_total_size = ~0U;
 
-		is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_NOFS);
-		bs = kzalloc(sizeof(struct ext4_xattr_block_find), GFP_NOFS);
-		if (!is || !bs) {
-			error = -ENOMEM;
-			goto cleanup;
-		}
-
-		is->s.not_found = -ENODATA;
-		bs->s.not_found = -ENODATA;
-		is->iloc.bh = NULL;
-		bs->bh = NULL;
-
 		last = IFIRST(header);
 		/* Find the entry best suited to be pushed into EA block */
 		entry = NULL;
@@ -1474,8 +1533,6 @@ retry:
 				    s_min_extra_isize) {
 					tried_min_extra_isize++;
 					new_extra_isize = s_min_extra_isize;
-					kfree(is); is = NULL;
-					kfree(bs); bs = NULL;
 					brelse(bh);
 					goto retry;
 				}
@@ -1483,58 +1540,18 @@ retry:
 				goto cleanup;
 			}
 		}
-		offs = le16_to_cpu(entry->e_value_offs);
-		size = le32_to_cpu(entry->e_value_size);
-		entry_size = EXT4_XATTR_LEN(entry->e_name_len);
-		total_size = entry_size + EXT4_XATTR_SIZE(size);
-		i.name_index = entry->e_name_index,
-		buffer = kmalloc(EXT4_XATTR_SIZE(size), GFP_NOFS);
-		b_entry_name = kmalloc(entry->e_name_len + 1, GFP_NOFS);
-		if (!buffer || !b_entry_name) {
-			error = -ENOMEM;
-			goto cleanup;
-		}
-		/* Save the entry name and the entry value */
-		memcpy(buffer, (void *)IFIRST(header) + offs,
-		       EXT4_XATTR_SIZE(size));
-		memcpy(b_entry_name, entry->e_name, entry->e_name_len);
-		b_entry_name[entry->e_name_len] = '\0';
-		i.name = b_entry_name;
-
-		error = ext4_get_inode_loc(inode, &is->iloc);
-		if (error)
-			goto cleanup;
 
-		error = ext4_xattr_ibody_find(inode, &i, is);
+		entry_size = EXT4_XATTR_LEN(entry->e_name_len);
+		total_size = entry_size +
+			EXT4_XATTR_SIZE(le32_to_cpu(entry->e_value_size));
+		error = ext4_xattr_move_to_block(handle, inode, raw_inode,
+						 entry);
 		if (error)
 			goto cleanup;
 
-		/* Remove the chosen entry from the inode */
-		error = ext4_xattr_ibody_set(handle, inode, &i, is);
-		if (error)
-			goto cleanup;
 		total_ino -= entry_size;
 		ifree += total_size;
 		bfree -= total_size;
-
-		i.name = b_entry_name;
-		i.value = buffer;
-		i.value_len = size;
-		error = ext4_xattr_block_find(inode, &i, bs);
-		if (error)
-			goto cleanup;
-
-		/* Add entry which was removed from the inode into the block */
-		error = ext4_xattr_block_set(handle, inode, &i, bs);
-		if (error)
-			goto cleanup;
-		kfree(b_entry_name);
-		kfree(buffer);
-		b_entry_name = NULL;
-		buffer = NULL;
-		brelse(is->iloc.bh);
-		kfree(is);
-		kfree(bs);
 	}
 
 shift:
@@ -1552,12 +1569,6 @@ out:
 	return 0;
 
 cleanup:
-	kfree(b_entry_name);
-	kfree(buffer);
-	if (is)
-		brelse(is->iloc.bh);
-	kfree(is);
-	kfree(bs);
 	brelse(bh);
 	/*
 	 * We deliberately leave EXT4_STATE_NO_EXPAND set here since inode
-- 
2.6.6

[PATCH 10/11] ext4: Remove (almost) unused variables from ext4_expand_extra_isize_ea()

[Jan Kara]

'start' variable is completely unused in ext4_expand_extra_isize_ea().
Variable 'first' is used only once in one place. So just remove them.
Variables 'entry' and 'last' are only really used later in the function
inside a loop. Move their declarations there.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/xattr.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 8f582ae1032d..2ef687620205 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1425,12 +1425,11 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 			       struct ext4_inode *raw_inode, handle_t *handle)
 {
 	struct ext4_xattr_ibody_header *header;
-	struct ext4_xattr_entry *entry, *last, *first;
 	struct buffer_head *bh = NULL;
 	size_t min_offs;
 	size_t ifree, bfree;
 	int total_ino;
-	void *base, *start, *end;
+	void *base, *end;
 	int error = 0, tried_min_extra_isize = 0;
 	int s_min_extra_isize = le16_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_min_extra_isize);
 	int isize_diff;	/* How much do we need to grow i_extra_isize */
@@ -1446,24 +1445,22 @@ retry:
 		goto out;
 
 	header = IHDR(inode, raw_inode);
-	entry = IFIRST(header);
 
 	/*
 	 * Check if enough free space is available in the inode to shift the
 	 * entries ahead by new_extra_isize.
 	 */
 
-	base = start = entry;
+	base = IFIRST(header);
 	end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size;
 	min_offs = end - base;
-	last = entry;
 	total_ino = sizeof(struct ext4_xattr_ibody_header);
 
 	error = xattr_check_inode(inode, header, end);
 	if (error)
 		goto cleanup;
 
-	ifree = ext4_xattr_free_space(last, &min_offs, base, &total_ino);
+	ifree = ext4_xattr_free_space(base, &min_offs, base, &total_ino);
 	if (ifree >= isize_diff)
 		goto shift;
 
@@ -1483,10 +1480,10 @@ retry:
 			goto cleanup;
 		}
 		base = BHDR(bh);
-		first = BFIRST(bh);
 		end = bh->b_data + bh->b_size;
 		min_offs = end - base;
-		bfree = ext4_xattr_free_space(first, &min_offs, base, NULL);
+		bfree = ext4_xattr_free_space(BFIRST(bh), &min_offs, base,
+					      NULL);
 		if (bfree + ifree < isize_diff) {
 			if (!tried_min_extra_isize && s_min_extra_isize) {
 				tried_min_extra_isize++;
@@ -1502,14 +1499,14 @@ retry:
 	}
 
 	while (isize_diff > ifree) {
-		struct ext4_xattr_entry *small_entry = NULL;
+		struct ext4_xattr_entry *small_entry = NULL, *entry = NULL;
+		struct ext4_xattr_entry *last;
 		unsigned int entry_size;	/* EA entry size */
 		unsigned int total_size;	/* EA entry size + value size */
 		unsigned int min_total_size = ~0U;
 
 		last = IFIRST(header);
 		/* Find the entry best suited to be pushed into EA block */
-		entry = NULL;
 		for (; !IS_LAST_ENTRY(last); last = EXT4_XATTR_NEXT(last)) {
 			total_size =
 			EXT4_XATTR_SIZE(le32_to_cpu(last->e_value_size)) +
@@ -1556,8 +1553,7 @@ retry:
 
 shift:
 	/* Adjust the offsets and shift the remaining entries ahead */
-	entry = IFIRST(header);
-	ext4_xattr_shift_entries(entry,	EXT4_I(inode)->i_extra_isize
+	ext4_xattr_shift_entries(IFIRST(header), EXT4_I(inode)->i_extra_isize
 			- new_extra_isize, (void *)raw_inode +
 			EXT4_GOOD_OLD_INODE_SIZE + new_extra_isize,
 			(void *)header, total_ino);
-- 
2.6.6

[PATCH 11/11] ext4: Factor out loop for freeing inode xattr space

[Jan Kara]

Move loop to make enough space in the inode from
ext4_expand_extra_isize_ea() into a separate function to make that
function smaller and better readable and also to avoid delaration of
variables inside a loop block.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/xattr.c | 121 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 69 insertions(+), 52 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 2ef687620205..c15d63389957 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1417,6 +1417,63 @@ out:
 	return error;
 }
 
+static int ext4_xattr_make_inode_space(handle_t *handle, struct inode *inode,
+				       struct ext4_inode *raw_inode,
+				       int isize_diff, size_t ifree,
+				       size_t bfree, int *total_ino)
+{
+	struct ext4_xattr_ibody_header *header = IHDR(inode, raw_inode);
+	struct ext4_xattr_entry *small_entry;
+	struct ext4_xattr_entry *entry;
+	struct ext4_xattr_entry *last;
+	unsigned int entry_size;	/* EA entry size */
+	unsigned int total_size;	/* EA entry size + value size */
+	unsigned int min_total_size;
+	int error;
+
+	while (isize_diff > ifree) {
+		entry = NULL;
+		small_entry = NULL;
+		min_total_size = ~0U;
+		last = IFIRST(header);
+		/* Find the entry best suited to be pushed into EA block */
+		for (; !IS_LAST_ENTRY(last); last = EXT4_XATTR_NEXT(last)) {
+			total_size =
+			EXT4_XATTR_SIZE(le32_to_cpu(last->e_value_size)) +
+					EXT4_XATTR_LEN(last->e_name_len);
+			if (total_size <= bfree &&
+			    total_size < min_total_size) {
+				if (total_size + ifree < isize_diff) {
+					small_entry = last;
+				} else {
+					entry = last;
+					min_total_size = total_size;
+				}
+			}
+		}
+
+		if (entry == NULL) {
+			if (small_entry == NULL)
+				return -ENOSPC;
+			entry = small_entry;
+		}
+
+		entry_size = EXT4_XATTR_LEN(entry->e_name_len);
+		total_size = entry_size +
+			EXT4_XATTR_SIZE(le32_to_cpu(entry->e_value_size));
+		error = ext4_xattr_move_to_block(handle, inode, raw_inode,
+						 entry);
+		if (error)
+			return error;
+
+		*total_ino -= entry_size;
+		ifree += total_size;
+		bfree -= total_size;
+	}
+
+	return 0;
+}
+
 /*
  * Expand an inode by new_extra_isize bytes when EAs are present.
  * Returns 0 on success or negative error number on failure.
@@ -1491,66 +1548,26 @@ retry:
 				brelse(bh);
 				goto retry;
 			}
-			error = -1;
+			error = -ENOSPC;
 			goto cleanup;
 		}
 	} else {
 		bfree = inode->i_sb->s_blocksize;
 	}
 
-	while (isize_diff > ifree) {
-		struct ext4_xattr_entry *small_entry = NULL, *entry = NULL;
-		struct ext4_xattr_entry *last;
-		unsigned int entry_size;	/* EA entry size */
-		unsigned int total_size;	/* EA entry size + value size */
-		unsigned int min_total_size = ~0U;
-
-		last = IFIRST(header);
-		/* Find the entry best suited to be pushed into EA block */
-		for (; !IS_LAST_ENTRY(last); last = EXT4_XATTR_NEXT(last)) {
-			total_size =
-			EXT4_XATTR_SIZE(le32_to_cpu(last->e_value_size)) +
-					EXT4_XATTR_LEN(last->e_name_len);
-			if (total_size <= bfree &&
-			    total_size < min_total_size) {
-				if (total_size + ifree < isize_diff) {
-					small_entry = last;
-				} else {
-					entry = last;
-					min_total_size = total_size;
-				}
-			}
-		}
-
-		if (entry == NULL) {
-			if (small_entry) {
-				entry = small_entry;
-			} else {
-				if (!tried_min_extra_isize &&
-				    s_min_extra_isize) {
-					tried_min_extra_isize++;
-					new_extra_isize = s_min_extra_isize;
-					brelse(bh);
-					goto retry;
-				}
-				error = -1;
-				goto cleanup;
-			}
+	error = ext4_xattr_make_inode_space(handle, inode, raw_inode,
+					    isize_diff, ifree, bfree,
+					    &total_ino);
+	if (error) {
+		if (error == -ENOSPC && !tried_min_extra_isize &&
+		    s_min_extra_isize) {
+			tried_min_extra_isize++;
+			new_extra_isize = s_min_extra_isize;
+			brelse(bh);
+			goto retry;
 		}
-
-		entry_size = EXT4_XATTR_LEN(entry->e_name_len);
-		total_size = entry_size +
-			EXT4_XATTR_SIZE(le32_to_cpu(entry->e_value_size));
-		error = ext4_xattr_move_to_block(handle, inode, raw_inode,
-						 entry);
-		if (error)
-			goto cleanup;
-
-		total_ino -= entry_size;
-		ifree += total_size;
-		bfree -= total_size;
+		goto cleanup;
 	}

Re: [PATCH 01/11] ext4: Fix xattr shifting when expanding inodes

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 4967 bytes --]

On Aug 3, 2016, at 4:39 AM, Jan Kara <jack@suse.cz> wrote:
> 
> The code in ext4_expand_extra_isize_ea() treated new_extra_isize
> argument sometimes as the desired target i_extra_isize and sometimes as
> the amount by which we need to grow current i_extra_isize. These happen
> to coincide when i_extra_isize is 0 which used to be the common case and
> so nobody noticed this until recently when we added i_projid to the
> inode and so i_extra_isize now needs to grow from 28 to 32 bytes.
> 
> The result of these bugs was that we sometimes unnecessarily decided to
> move xattrs out of inode even if there was enough space and we often
> ended up corrupting in-inode xattrs because arguments to
> ext4_xattr_shift_entries() were just wrong. This could demonstrate
> itself as BUG_ON in ext4_xattr_shift_entries() triggering.
> 
> Fix the problem by introducing new isize_diff variable and use it where
> appropriate.
> 
> CC: <stable@vger.kernel.org> # 4.4.x-
> Reported-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/ext4/xattr.c | 27 ++++++++++++++-------------
> 1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 39e9cfb1b371..cb1d7b4482de 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1353,11 +1353,13 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
> 	size_t min_offs, free;
> 	int total_ino;
> 	void *base, *start, *end;
> -	int extra_isize = 0, error = 0, tried_min_extra_isize = 0;
> +	int error = 0, tried_min_extra_isize = 0;
> 	int s_min_extra_isize = le16_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_min_extra_isize);
> +	int isize_diff;	/* How much do we need to grow i_extra_isize */
> 
> 	down_write(&EXT4_I(inode)->xattr_sem);
> retry:
> +	isize_diff = new_extra_isize - EXT4_I(inode)->i_extra_isize;
> 	if (EXT4_I(inode)->i_extra_isize >= new_extra_isize) {

Not a big deal, but either the isize_diff calculation could be moved
after the "if () {}" block, or the condition could be changed to:

	if (isize_diff <= 0) {

since isize_diff is otherwise unused if this condition is true.

You can add my

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Cheers, Andreas

> 		up_write(&EXT4_I(inode)->xattr_sem);
> 		return 0;
> @@ -1382,7 +1384,7 @@ retry:
> 		goto cleanup;
> 
> 	free = ext4_xattr_free_space(last, &min_offs, base, &total_ino);
> -	if (free >= new_extra_isize) {
> +	if (free >= isize_diff) {
> 		entry = IFIRST(header);
> 		ext4_xattr_shift_entries(entry,	EXT4_I(inode)->i_extra_isize
> 				- new_extra_isize, (void *)raw_inode +
> @@ -1414,7 +1416,7 @@ retry:
> 		end = bh->b_data + bh->b_size;
> 		min_offs = end - base;
> 		free = ext4_xattr_free_space(first, &min_offs, base, NULL);
> -		if (free < new_extra_isize) {
> +		if (free < isize_diff) {
> 			if (!tried_min_extra_isize && s_min_extra_isize) {
> 				tried_min_extra_isize++;
> 				new_extra_isize = s_min_extra_isize;
> @@ -1428,7 +1430,7 @@ retry:
> 		free = inode->i_sb->s_blocksize;
> 	}
> 
> -	while (new_extra_isize > 0) {
> +	while (isize_diff > 0) {
> 		size_t offs, size, entry_size;
> 		struct ext4_xattr_entry *small_entry = NULL;
> 		struct ext4_xattr_info i = {
> @@ -1459,7 +1461,7 @@ retry:
> 			EXT4_XATTR_SIZE(le32_to_cpu(last->e_value_size)) +
> 					EXT4_XATTR_LEN(last->e_name_len);
> 			if (total_size <= free && total_size < min_total_size) {
> -				if (total_size < new_extra_isize) {
> +				if (total_size < isize_diff) {
> 					small_entry = last;
> 				} else {
> 					entry = last;
> @@ -1516,20 +1518,19 @@ retry:
> 			goto cleanup;
> 
> 		entry = IFIRST(header);
> -		if (entry_size + EXT4_XATTR_SIZE(size) >= new_extra_isize)
> -			shift_bytes = new_extra_isize;
> +		if (entry_size + EXT4_XATTR_SIZE(size) >= isize_diff)
> +			shift_bytes = isize_diff;
> 		else
> 			shift_bytes = entry_size + size;
> 		/* Adjust the offsets and shift the remaining entries ahead */
> -		ext4_xattr_shift_entries(entry, EXT4_I(inode)->i_extra_isize -
> -			shift_bytes, (void *)raw_inode +
> -			EXT4_GOOD_OLD_INODE_SIZE + extra_isize + shift_bytes,
> +		ext4_xattr_shift_entries(entry, -shift_bytes,
> +			(void *)raw_inode + EXT4_GOOD_OLD_INODE_SIZE +
> +			EXT4_I(inode)->i_extra_isize + shift_bytes,
> 			(void *)header, total_ino - entry_size,
> 			inode->i_sb->s_blocksize);
> 
> -		extra_isize += shift_bytes;
> -		new_extra_isize -= shift_bytes;
> -		EXT4_I(inode)->i_extra_isize = extra_isize;
> +		isize_diff -= shift_bytes;
> +		EXT4_I(inode)->i_extra_isize += shift_bytes;
> 
> 		i.name = b_entry_name;
> 		i.value = buffer;
> --
> 2.6.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 01/11] ext4: Fix xattr shifting when expanding inodes

[Jan Kara]

On Wed 03-08-16 17:25:58, Andreas Dilger wrote:
> On Aug 3, 2016, at 4:39 AM, Jan Kara <jack@suse.cz> wrote:
> > 
> > The code in ext4_expand_extra_isize_ea() treated new_extra_isize
> > argument sometimes as the desired target i_extra_isize and sometimes as
> > the amount by which we need to grow current i_extra_isize. These happen
> > to coincide when i_extra_isize is 0 which used to be the common case and
> > so nobody noticed this until recently when we added i_projid to the
> > inode and so i_extra_isize now needs to grow from 28 to 32 bytes.
> > 
> > The result of these bugs was that we sometimes unnecessarily decided to
> > move xattrs out of inode even if there was enough space and we often
> > ended up corrupting in-inode xattrs because arguments to
> > ext4_xattr_shift_entries() were just wrong. This could demonstrate
> > itself as BUG_ON in ext4_xattr_shift_entries() triggering.
> > 
> > Fix the problem by introducing new isize_diff variable and use it where
> > appropriate.
> > 
> > CC: <stable@vger.kernel.org> # 4.4.x-
> > Reported-by: Dave Chinner <david@fromorbit.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > fs/ext4/xattr.c | 27 ++++++++++++++-------------
> > 1 file changed, 14 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> > index 39e9cfb1b371..cb1d7b4482de 100644
> > --- a/fs/ext4/xattr.c
> > +++ b/fs/ext4/xattr.c
> > @@ -1353,11 +1353,13 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
> > 	size_t min_offs, free;
> > 	int total_ino;
> > 	void *base, *start, *end;
> > -	int extra_isize = 0, error = 0, tried_min_extra_isize = 0;
> > +	int error = 0, tried_min_extra_isize = 0;
> > 	int s_min_extra_isize = le16_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_min_extra_isize);
> > +	int isize_diff;	/* How much do we need to grow i_extra_isize */
> > 
> > 	down_write(&EXT4_I(inode)->xattr_sem);
> > retry:
> > +	isize_diff = new_extra_isize - EXT4_I(inode)->i_extra_isize;
> > 	if (EXT4_I(inode)->i_extra_isize >= new_extra_isize) {
> 
> Not a big deal, but either the isize_diff calculation could be moved
> after the "if () {}" block, or the condition could be changed to:
> 
> 	if (isize_diff <= 0) {
> 
> since isize_diff is otherwise unused if this condition is true.
> 
> You can add my
> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Thanks for review. Yes, my plan was to use isize_diff in couple more places
in the function (including the following condition) but I didn't want to
introduce unnecessary churn in these initial patches for stable kernel. It
seems I forgot to do this in the followup cleanup patches but I guess it's
not worth a respin now. If there are more comments, I'll include this as
well.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 01/11] ext4: Fix xattr shifting when expanding inodes

[Theodore Ts'o]

On Wed, Aug 03, 2016 at 12:39:45PM +0200, Jan Kara wrote:
> The code in ext4_expand_extra_isize_ea() treated new_extra_isize
> argument sometimes as the desired target i_extra_isize and sometimes as
> the amount by which we need to grow current i_extra_isize. These happen
> to coincide when i_extra_isize is 0 which used to be the common case and
> so nobody noticed this until recently when we added i_projid to the
> inode and so i_extra_isize now needs to grow from 28 to 32 bytes.
> 
> The result of these bugs was that we sometimes unnecessarily decided to
> move xattrs out of inode even if there was enough space and we often
> ended up corrupting in-inode xattrs because arguments to
> ext4_xattr_shift_entries() were just wrong. This could demonstrate
> itself as BUG_ON in ext4_xattr_shift_entries() triggering.
> 
> Fix the problem by introducing new isize_diff variable and use it where
> appropriate.
> 
> CC: <stable@vger.kernel.org> # 4.4.x-
> Reported-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 02/11] ext4: Fix xattr shifting when expanding inodes (2)

[Theodore Ts'o]

On Wed, Aug 03, 2016 at 12:39:46PM +0200, Jan Kara wrote:
> When multiple xattrs need to be moved out of inode, we did not properly
> recompute total size of xattr headers in the inode and the new header
> position. Thus when moving the second and further xattr we asked
> ext4_xattr_shift_entries() to move too much and from the wrong place,
> resulting in possible xattr value corruption or general memory
> corruption.
> 
> CC: <stable@vger.kernel.org> # 4.4.x-
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 03/11] ext4: Properly align shifted xattrs when expanding inodes

[Theodore Ts'o]

On Wed, Aug 03, 2016 at 12:39:47PM +0200, Jan Kara wrote:
> We did not count with the padding of xattr value when computing desired
> shift of xattrs in the inode when expanding i_extra_isize. As a result
> we could create unaligned start of inline xattrs. Account for alignment
> properly.
> 
> CC: <stable@vger.kernel.org> # 4.4.x-
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 04/11] ext4: Avoid deadlock when expanding inode size

[Theodore Ts'o]

On Wed, Aug 03, 2016 at 12:39:48PM +0200, Jan Kara wrote:
> When we need to move xattrs into external xattr block, we call
> ext4_xattr_block_set() from ext4_expand_extra_isize_ea(). That may end
> up calling ext4_mark_inode_dirty() again which will recurse back into
> the inode expansion code leading to deadlocks.
> 
> Protect from recursion using EXT4_STATE_NO_EXPAND inode flag and move
> its management into ext4_expand_extra_isize_ea() since its manipulation
> is safe there (due to xattr_sem) from possible races with
> ext4_xattr_set_handle() which plays with it as well.
> 
> CC: <stable@vger.kernel.org> # 4.4.x-
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 05/11] ext4: Fixup free space calculations when expanding inodes

[Theodore Ts'o]

On Wed, Aug 03, 2016 at 12:39:49PM +0200, Jan Kara wrote:
> Conditions checking whether there is enough free space in an xattr block
> and when xattr is large enough to make enough space in the inode forgot
> to account for the fact that inode need not be completely filled up with
> xattrs. Thus we could move unnecessarily many xattrs out of inode or
> even falsely claim there is not enough space to expand the inode. We
> also forgot to update the amount of free space in xattr block when moving
> more xattrs and thus could decide to move too big xattr resulting in
> unexpected failure.
> 
> Fix these problems by properly updating free space in the inode and
> xattr block as we move xattrs. To simplify the math, avoid shifting
> xattrs after removing each one xattr and instead just shift xattrs only
> once there is enough free space in the inode.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 06/11] ext4: Check that external xattr value block is 0

[Theodore Ts'o]

On Wed, Aug 03, 2016 at 12:39:50PM +0200, Jan Kara wrote:
> Currently we don't support xattrs with values stored out of line. Check
> for that in ext4_xattr_check_names() to make sure we never work with
> such xattrs since not all the code counts with that resulting is possible
> weird corruption issues.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 07/11] ext4: Remove checks for e_value_block

[Theodore Ts'o]

On Wed, Aug 03, 2016 at 12:39:51PM +0200, Jan Kara wrote:
> Currently we don't support xattrs with e_value_block set. We don't allow
> them to pass initial xattr check so there's no point for checking for
> this later. Since these tests were untested, bugs were creeping in and
> not all places which should have checked were checking e_value_block
> anyway.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 08/11] ext4: Replace bogus assertion in ext4_xattr_shift_entries()

[Theodore Ts'o]

On Wed, Aug 03, 2016 at 12:39:52PM +0200, Jan Kara wrote:
> We were checking whether computed offsets do not exceed end of block in
> ext4_xattr_shift_entries(). However this does not make sense since we
> always only decrease offsets. So replace that assertion with a check
> whether we really decrease xattrs value offsets.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 09/11] ext4: Factor out xattr moving

[Theodore Ts'o]

On Wed, Aug 03, 2016 at 12:39:53PM +0200, Jan Kara wrote:
> Factor out function for moving xattrs from inode into external xattr
> block from ext4_expand_extra_isize_ea(). That function is already quite
> long and factoring out this rather standalone functionality helps
> readability.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 10/11] ext4: Remove (almost) unused variables from ext4_expand_extra_isize_ea()

[Theodore Ts'o]

On Wed, Aug 03, 2016 at 12:39:54PM +0200, Jan Kara wrote:
> 'start' variable is completely unused in ext4_expand_extra_isize_ea().
> Variable 'first' is used only once in one place. So just remove them.
> Variables 'entry' and 'last' are only really used later in the function
> inside a loop. Move their declarations there.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 11/11] ext4: Factor out loop for freeing inode xattr space

[Theodore Ts'o]

On Wed, Aug 03, 2016 at 12:39:55PM +0200, Jan Kara wrote:
> Move loop to make enough space in the inode from
> ext4_expand_extra_isize_ea() into a separate function to make that
> function smaller and better readable and also to avoid delaration of
> variables inside a loop block.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted