Re: [patch 5/5] Disallow in-inode attributes for reserved inodes

[Andreas Dilger <adilger@clusterfs.com>]

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

On Jan 20, 2005  03:01 +0100, Andreas Gruenbacher wrote:
> When creating a filesystem with inodes bigger than 128 bytes, mke2fs
> fails to clear out bytes beyond EXT3_GOOD_OLD_INODE_SIZE in all inodes
> it creates (the journal, the filesystem root, and lost+found). We would
> require a zeroed-out i_extra_isize field but we don't get it, so
> disallow in-inode attributes for those inodes.
> 
> Add an i_extra_isize sanity check.

I'm not sure I agree with this patch.  It definitely resolves a problem
I had with the previous patch (4/5), namely that for inodes that were
created in a large-inode filesystem using a kernel w/o the large-inode
patch we didn't save EAs into the large-inode space at all.  With this
patch we will start using that space.

It is debatable whether we should mark inodes bad if the i_extra_isize
field is bad, or if we should just initialize i_extra_isize in that case.
On one hand there is very little else we can use to validate the on-disk
inodes, but on the other hand the EA data isn't critical to reading the
inode so this would seem to introduce additional failure cases.

I'm not sure whether an old e2fsck will also clear out the space in a
large inode that it writes or not (I know that the patched one we use
validates i_extra_isize).  If it doesn't it may be that there would be a
largish number of inodes that are marked bad because of this, so having
the kernel fix this would be good.  If the old e2fsck zeroes the large
inodes properly it would be prudent to have an ext3_error() here so that
the disk corruption triggers an e2fsck the next time the system starts.

For the root and lost+found inodes it looks like we can never store an
EA in the extra part of the inode regardless of whether i_extra_isize is
good or not.  If a bad value is found we could just initialize it and
start using that space (though not print an ext3_error() in that case,
an ext3_warning() if anything since this is probably the fault of mke2fs).

> Index: linux-2.6.11-latest/fs/ext3/inode.c
> ===================================================================
> --- linux-2.6.11-latest.orig/fs/ext3/inode.c
> +++ linux-2.6.11-latest/fs/ext3/inode.c
> @@ -2493,15 +2493,30 @@ void ext3_read_inode(struct inode * inod
>  		ei->i_data[block] = raw_inode->i_block[block];
>  	INIT_LIST_HEAD(&ei->i_orphan);
>  
> -	ei->i_extra_isize =
> -		(EXT3_INODE_SIZE(inode->i_sb) > EXT3_GOOD_OLD_INODE_SIZE) ?
> -		le16_to_cpu(raw_inode->i_extra_isize) : 0;
> -	if (ei->i_extra_isize) {
> -		__le32 *magic = (void *)raw_inode + EXT3_GOOD_OLD_INODE_SIZE +
> -				ei->i_extra_isize;
> -		if (le32_to_cpu(*magic))
> -			 ei->i_state |= EXT3_STATE_XATTR;
> -	}
> +	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.
> +		 */
> +		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))
> +			goto bad_inode;
> +		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;
> +		} else {
> +			__le32 *magic = (void *)raw_inode +
> +					EXT3_GOOD_OLD_INODE_SIZE +
> +					ei->i_extra_isize;
> +			if (*magic == cpu_to_le32(EXT3_XATTR_MAGIC))
> +				 ei->i_state |= EXT3_STATE_XATTR;
> +		}
> +	} else
> +		ei->i_extra_isize = 0;
>  
>  	if (S_ISREG(inode->i_mode)) {
>  		inode->i_op = &ext3_file_inode_operations;

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://members.shaw.ca/adilger/             http://members.shaw.ca/golinux/


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]