[PATCH 0/2] ext3 in-inode xattr handling fix for reserved inodes

[PATCH 0/2] ext3 in-inode xattr handling fix for reserved inodes

[Jan Kara]

  Hi,

  the first patch in this series fixes a bug where we could lose extended
attributes for ino 11 if lost+found was deleted and inode used again.
The second patch makes kernel accept in-inode xattrs even for reserved
inodes if they look sane as Andreas suggested. This way we could in theory
remove the special casing sometime in future. I'm personally not convinced
this is worth the effort since the time when we could remove the special
casing is at least several years in future (for example I switch to 2.6.16
or even older kernels on our test machines) but I'm throwing the patch
in so that people can express their opinions...

								Honza

[PATCH 1/2] ext3: Fix lost extented attributes for inode with ino == 11

[Jan Kara]

If a filesystem has inode size > 128 and someone deletes lost+found and
reuses inode 11 for some other file, extented attributes set for this
inode before umount will get lost after remounting the filesystem. This
is because extended attributes will get stored in an inode but ext3_iget
will ignore them due to workaround of a bug in an old mkfs.

Fix the problem by initializing i_extra_isize to 0 for freshly allocated
inodes where mkfs workaround in ext3_iget applies. This way these inodes
will always store extended attributes in a special block and no problems
occur.

The bug was spotted and a reproduction test provided by:
  Masayoshi MIZUMA <m.mizuma@jp.fujitsu.com>

CC: Andreas Dilger <adilger.kernel@dilger.ca>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext3/ialloc.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/ext3/ialloc.c b/fs/ext3/ialloc.c
index 4ab72db..9724aef 100644
--- a/fs/ext3/ialloc.c
+++ b/fs/ext3/ialloc.c
@@ -570,9 +570,14 @@ got:
 	ei->i_state_flags = 0;
 	ext3_set_inode_state(inode, EXT3_STATE_NEW);
 
-	ei->i_extra_isize =
-		(EXT3_INODE_SIZE(inode->i_sb) > EXT3_GOOD_OLD_INODE_SIZE) ?
-		sizeof(struct ext3_inode) - EXT3_GOOD_OLD_INODE_SIZE : 0;
+	/* See comment in ext3_iget for explanation */
+	if (ino >= EXT3_FIRST_INO(sb) + 1 &&
+	    EXT3_INODE_SIZE(sb) > EXT3_GOOD_OLD_INODE_SIZE) {
+		ei->i_extra_isize =
+			sizeof(struct ext3_inode) - EXT3_GOOD_OLD_INODE_SIZE;
+	} else {
+		ei->i_extra_isize = 0;
+	}
 
 	ret = inode;
 	dquot_initialize(inode);
-- 
1.6.4.2

[PATCH 2/2] ext3: Accept in-inode xattrs for reserved inodes

[Jan Kara]

So far we just ignore any in-inode xattrs in reserved inodes (with inode
number <= EXT3_FIRST_INO + 1) to workaround a bug in old mke2fs (<= 1.37)
which forgot to zero such inodes when inode size was greater than
EXT3_GOOD_OLD_INODE_SIZE. This kernel behavior prevents us from ever
using in-inode xattrs for such inodes because they would be ignored by
older kernels.

So make kernel accept data in extended inode space even for reserved inodes
if they look sane but never store data there. This way, we can remove all
the special cases once kernels without this handling are old enough.

CC: Andreas Dilger <adilger.kernel@dilger.ca>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext3/inode.c |   37 ++++++++++++++++++++++++-------------
 1 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index 5e0faf4..1f3e83f 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -2914,24 +2914,35 @@ struct inode *ext3_iget(struct super_block *sb, unsigned long ino)
 		atomic_set(&ei->i_datasync_tid, tid);
 	}
 
-	if (inode->i_ino >= EXT3_FIRST_INO(inode->i_sb) + 1 &&
-	    EXT3_INODE_SIZE(inode->i_sb) > EXT3_GOOD_OLD_INODE_SIZE) {
-		/*
-		 * When mke2fs creates big inodes it does not zero out
-		 * the unused bytes above EXT3_GOOD_OLD_INODE_SIZE,
-		 * so ignore those first few inodes.
-		 */
+	if (EXT3_INODE_SIZE(inode->i_sb) > EXT3_GOOD_OLD_INODE_SIZE) {
 		ei->i_extra_isize = le16_to_cpu(raw_inode->i_extra_isize);
 		if (EXT3_GOOD_OLD_INODE_SIZE + ei->i_extra_isize >
 		    EXT3_INODE_SIZE(inode->i_sb)) {
-			brelse (bh);
-			ret = -EIO;
-			goto bad_inode;
+			/*
+			 * Old mke2fs (<= 1.37) did not zero i_extra_size for
+			 * large reserved inodes. So just ignore bogus
+			 * i_extra_size for these inodes.
+			 */
+			if (inode->i_ino >= EXT3_FIRST_INO(inode->i_sb) + 1) {
+				brelse (bh);
+				ret = -EIO;
+				goto bad_inode;
+			}
+			ei->i_extra_isize = 0;
 		}
 		if (ei->i_extra_isize == 0) {
-			/* The extra space is currently unused. Use it. */
-			ei->i_extra_isize = sizeof(struct ext3_inode) -
-					    EXT3_GOOD_OLD_INODE_SIZE;
+			/*
+			 * We cannot use free space for reserved inodes because
+			 * old kernels (until 2.6.36) would just ignore xattrs
+			 * in that space. This workaround can be removed if we
+			 * ever deem that mounting a filesystem with an old
+			 * kernel is unlikely enough.
+			 */
+			if (inode->i_ino >= EXT3_FIRST_INO(inode->i_sb) + 1) {
+				/* The extra space is unused. Use it. */
+				ei->i_extra_isize = sizeof(struct ext3_inode) -
+						    EXT3_GOOD_OLD_INODE_SIZE;
+			}
 		} else {
 			__le32 *magic = (void *)raw_inode +
 					EXT3_GOOD_OLD_INODE_SIZE +
-- 
1.6.4.2

Re: [PATCH 1/2] ext3: Fix lost extented attributes for inode with ino == 11

[Andreas Dilger]

On 2010-09-14, at 06:32, Jan Kara wrote:
> If a filesystem has inode size > 128 and someone deletes lost+found and
> reuses inode 11 for some other file, extented attributes set for this
> inode before umount will get lost after remounting the filesystem. This
> is because extended attributes will get stored in an inode but ext3_iget
> will ignore them due to workaround of a bug in an old mkfs.
> 
> Fix the problem by initializing i_extra_isize to 0 for freshly allocated
> inodes where mkfs workaround in ext3_iget applies. This way these inodes
> will always store extended attributes in a special block and no problems
> occur.
> 
> The bug was spotted and a reproduction test provided by:
>  Masayoshi MIZUMA <m.mizuma@jp.fujitsu.com>
> 
> CC: Andreas Dilger <adilger.kernel@dilger.ca>

Both patches now look good to me.  You can add a:

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

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/ext3/ialloc.c |   11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext3/ialloc.c b/fs/ext3/ialloc.c
> index 4ab72db..9724aef 100644
> --- a/fs/ext3/ialloc.c
> +++ b/fs/ext3/ialloc.c
> @@ -570,9 +570,14 @@ got:
> 	ei->i_state_flags = 0;
> 	ext3_set_inode_state(inode, EXT3_STATE_NEW);
> 
> -	ei->i_extra_isize =
> -		(EXT3_INODE_SIZE(inode->i_sb) > EXT3_GOOD_OLD_INODE_SIZE) ?
> -		sizeof(struct ext3_inode) - EXT3_GOOD_OLD_INODE_SIZE : 0;
> +	/* See comment in ext3_iget for explanation */
> +	if (ino >= EXT3_FIRST_INO(sb) + 1 &&
> +	    EXT3_INODE_SIZE(sb) > EXT3_GOOD_OLD_INODE_SIZE) {
> +		ei->i_extra_isize =
> +			sizeof(struct ext3_inode) - EXT3_GOOD_OLD_INODE_SIZE;
> +	} else {
> +		ei->i_extra_isize = 0;
> +	}
> 
> 	ret = inode;
> 	dquot_initialize(inode);
> -- 
> 1.6.4.2
>