Date: Wed, 3 Dec 2008 17:53:43 -0500 From: "Josh Carroll" <josh.carroll@gmail.com> To: "Kostik Belousov" <kostikbel@gmail.com> Cc: freebsd-fs@freebsd.org, FreeBSD Stable <freebsd-stable@freebsd.org> Subject: Re: ext2 inode size patch - RE: PR kern/124621 Message-ID: <8cb6106e0812031453j6dc2f2f4i374145823c084eaa@mail.gmail.com> In-Reply-To: <20081125150342.GL2042@deviant.kiev.zoral.com.ua> References: <8cb6106e0811241129o642dcf28re4ae177c8ccbaa25@mail.gmail.com> <20081125140601.GH2042@deviant.kiev.zoral.com.ua> <8cb6106e0811250617q5fffb41exe20dfb8314fc4a9d@mail.gmail.com> <20081125142827.GI2042@deviant.kiev.zoral.com.ua> <8cb6106e0811250657q6fdf08b0x1e94f35fd0a7ed4f@mail.gmail.com> <20081125150342.GL2042@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
> Ok, I describe my concern once more. I do not object against the checking
> of the inode size. But, if inode size is changed, then some data is added
> to the inode, that could (and usually does, otherwise why extend it ?)
> change intrerpetation of the inode. Thus, we need a verification of the
> fact that simply ignoring added fields does not damage filesystem or
> cause user data corruption. Verification != testing.
Let me take another crack at explaining why I think this patch is not dangerous.
The inode size is determined by reading the following member:
__u16 s_inode_size;
of the ext2_super_block structure.
I took a look at the Linux 2.6.27.7 kernel source, and indeed they do
something very similar if not the same:
#define EXT2_INODE_SIZE(s) (EXT2_SB(s)->s_inode_size)
If you compare to what I did:
#define EXT2_INODE_SIZE(s) ((s)->u.ext2_sb.s_inode_size)
This is really the same thing, since EXT2_SB is defined (in the Linux
kernel src as):
#ifdef __KERNEL__
#include <linux/ext2_fs_sb.h>
static inline struct ext2_sb_info *EXT2_SB(struct super_block *sb)
{
return sb->s_fs_info;
}
And struct ext2_sb_info has the following member:
int s_inode_size;
Essentially, the changes I made simply query the existing information
from the filesystem, which is what the Linux kernel does as well.
The inode size, blocks per group, etc are all defined at filesystem
creation time by mke2fs and it ensures the sanity of the relationship
between the inodes/blocks/block groups.
The first diagram here:
http://sunsite.nus.sg/LDP/LDP/tlk/node95.html
Makes it clear that as long as the number of inodes per block and the
blocks per group is is sane during fs creation, looking up the inode
size as my patch does is fine, since the creation of the filesystem is
ensures a correct by construction situation. We're simply reading the
size of the inode based on the filesystem.
I hope this is sufficient to convince some further thought about
committing this.
For those interested in the relevant Linux kernel source, you can have
a look at line 105 of include/linux/ext2_fs.h from the linux-2.6.27.7
kernel source.
Thanks,
Josh
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8cb6106e0812031453j6dc2f2f4i374145823c084eaa>