Patchwork [12/28] ext4: Use i_generation in inode-related metadata checksums

login
register
mail settings
Submitter Darrick J. Wong
Date Oct. 8, 2011, 7:55 a.m.
Message ID <20111008075502.20506.15728.stgit@elm3c44.beaverton.ibm.com>
Download mbox | patch
Permalink /patch/118493/
State Superseded
Headers show

Comments

Darrick J. Wong - Oct. 8, 2011, 7:55 a.m.
Whenever we are calculating a checksum for a piece of metadata that is
associated with an inode, incorporate i_generation into that calculation so
that old metadata blocks cannot be re-associated after a delete/create cycle.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---
 fs/ext4/ialloc.c |    9 ++++++---
 fs/ext4/inode.c  |    9 ++++++---
 fs/ext4/ioctl.c  |    4 ++++
 3 files changed, 16 insertions(+), 6 deletions(-)



--
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
Andreas Dilger - Oct. 12, 2011, 7:52 p.m.
On 2011-10-08, at 12:55 AM, Darrick J. Wong wrote:
> Whenever we are calculating a checksum for a piece of metadata that is
> associated with an inode, incorporate i_generation into that calculation
> so that old metadata blocks cannot be re-associated after a delete/create cycle.

It would be better to fold this into the previous patch, since it will
otherwise cause the inode checksum algorithm to change in the middle of the patch series.

On a related note, in ext4_new_inode() it would be useful to change the
setting of i_generation so that it skips i_generation == 0, which doesn't
contribute to the checksum:

        spin_lock(&sbi->s_next_gen_lock);
        inode->i_generation = sbi->s_next_generation++;
+	if (unlikely(inode->i_generation == 0))
+	        inode->i_generation = sbi->s_next_generation++;
        spin_unlock(&sbi->s_next_gen_lock);

> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index 6e5876a..d4b59e9 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -1031,11 +1031,14 @@ got:
> 	/* Precompute second piece of crc */
> 	if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
> 			EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) {
> +		__u32 crc;
> 		struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> 		__le32 inum = cpu_to_le32(inode->i_ino);
> -		ei->i_uuid_inum_crc = ext4_chksum(sbi, sbi->s_uuid_crc,
> -						  (__u8 *)&inum,
> -						  sizeof(inum));
> +		__le32 gen = cpu_to_le32(inode->i_generation);
> +		crc = ext4_chksum(sbi, sbi->s_uuid_crc, (__u8 *)&inum,
> +				  sizeof(inum));
> +		ei->i_uuid_inum_crc = ext4_chksum(sbi, crc, (__u8 *)&gen,
> +						  sizeof(gen));
> 	}
> 
> 	ext4_clear_state_flags(ei); /* Only relevant on 32-bit archs */
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index f18bfe3..fdf0b1e 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -149,6 +149,10 @@ flags_out:
> 		if (!inode_owner_or_capable(inode))
> 			return -EPERM;
> 
> +		if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> +				EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> +			return -ENOTTY;

This should get an ext4_warning() in the non-checksum case to warn
users that this ioctl is deprecated and will be removed in the
future unless there is a good reason to keep it.

Cheers, Andreas





--
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
Darrick J. Wong - Oct. 12, 2011, 9:28 p.m.
On Wed, Oct 12, 2011 at 12:52:30PM -0700, Andreas Dilger wrote:
> On 2011-10-08, at 12:55 AM, Darrick J. Wong wrote:
> > Whenever we are calculating a checksum for a piece of metadata that is
> > associated with an inode, incorporate i_generation into that calculation
> > so that old metadata blocks cannot be re-associated after a delete/create cycle.
> 
> It would be better to fold this into the previous patch, since it will
> otherwise cause the inode checksum algorithm to change in the middle of the patch series.

Sure.  I guess I can go do that on the e2fsprogs side too.

> On a related note, in ext4_new_inode() it would be useful to change the
> setting of i_generation so that it skips i_generation == 0, which doesn't
> contribute to the checksum:
> 
>         spin_lock(&sbi->s_next_gen_lock);
>         inode->i_generation = sbi->s_next_generation++;
> +	if (unlikely(inode->i_generation == 0))
> +	        inode->i_generation = sbi->s_next_generation++;
>         spin_unlock(&sbi->s_next_gen_lock);

For that to happen the UUID+inode would have to checksum to zero, which seems
fairly unlikely since I set the "initial" value in ext4_chksum to ~0 to avoid
the case where UUID = 0, and there's no such thing as inode 0, which means thee
likelihood of the checksum being 0 at this point ought to be 1:2^32.  Even
then, the checksum will be different if the value of i_generation changes.

That said, if we /do/ implement this, then perhaps e2fsprogs should be taught
to set i_generation, as it does not do that currently.

> > diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> > index 6e5876a..d4b59e9 100644
> > --- a/fs/ext4/ialloc.c
> > +++ b/fs/ext4/ialloc.c
> > @@ -1031,11 +1031,14 @@ got:
> > 	/* Precompute second piece of crc */
> > 	if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
> > 			EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) {
> > +		__u32 crc;
> > 		struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> > 		__le32 inum = cpu_to_le32(inode->i_ino);
> > -		ei->i_uuid_inum_crc = ext4_chksum(sbi, sbi->s_uuid_crc,
> > -						  (__u8 *)&inum,
> > -						  sizeof(inum));
> > +		__le32 gen = cpu_to_le32(inode->i_generation);
> > +		crc = ext4_chksum(sbi, sbi->s_uuid_crc, (__u8 *)&inum,
> > +				  sizeof(inum));
> > +		ei->i_uuid_inum_crc = ext4_chksum(sbi, crc, (__u8 *)&gen,
> > +						  sizeof(gen));
> > 	}
> > 
> > 	ext4_clear_state_flags(ei); /* Only relevant on 32-bit archs */
> > 
> > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> > index f18bfe3..fdf0b1e 100644
> > --- a/fs/ext4/ioctl.c
> > +++ b/fs/ext4/ioctl.c
> > @@ -149,6 +149,10 @@ flags_out:
> > 		if (!inode_owner_or_capable(inode))
> > 			return -EPERM;
> > 
> > +		if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> > +				EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> > +			return -ENOTTY;
> 
> This should get an ext4_warning() in the non-checksum case to warn
> users that this ioctl is deprecated and will be removed in the
> future unless there is a good reason to keep it.

Ok.  Maybe put it in feature_removal_schedule.txt too?  Maybe not; the ioctl is
not being totally removed, it's merely unsupported for the metadata_csum case.

--D

--
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
Andreas Dilger - Oct. 13, 2011, 12:06 a.m.
On 2011-10-12, at 3:28 PM, Darrick J. Wong wrote:
> On Wed, Oct 12, 2011 at 12:52:30PM -0700, Andreas Dilger wrote:
>> On 2011-10-08, at 12:55 AM, Darrick J. Wong wrote:
>>> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
>>> index f18bfe3..fdf0b1e 100644
>>> --- a/fs/ext4/ioctl.c
>>> +++ b/fs/ext4/ioctl.c
>>> @@ -149,6 +149,10 @@ flags_out:
>>> 		if (!inode_owner_or_capable(inode))
>>> 			return -EPERM;
>>> 
>>> +		if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
>>> +				EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
>>> +			return -ENOTTY;
>> 
>> This should get an ext4_warning() in the non-checksum case to warn
>> users that this ioctl is deprecated and will be removed in the
>> future unless there is a good reason to keep it.
> 
> Ok.  Maybe put it in feature_removal_schedule.txt too?  Maybe not; the
> ioctl is not being totally removed, it's merely unsupported for the
> metadata_csum case.

I understand it is not being removed entirely, but since it doesn't
work at all with the checksum-enabled case, we may as well mark it
deprecated for the non-checksum case to allow us to catch any users
(however unlikely I think it is) earlier rather than later.

Cheers, Andreas





--
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

Patch

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 6e5876a..d4b59e9 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -1031,11 +1031,14 @@  got:
 	/* Precompute second piece of crc */
 	if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
 			EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) {
+		__u32 crc;
 		struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 		__le32 inum = cpu_to_le32(inode->i_ino);
-		ei->i_uuid_inum_crc = ext4_chksum(sbi, sbi->s_uuid_crc,
-						  (__u8 *)&inum,
-						  sizeof(inum));
+		__le32 gen = cpu_to_le32(inode->i_generation);
+		crc = ext4_chksum(sbi, sbi->s_uuid_crc, (__u8 *)&inum,
+				  sizeof(inum));
+		ei->i_uuid_inum_crc = ext4_chksum(sbi, crc, (__u8 *)&gen,
+						  sizeof(gen));
 	}
 
 	ext4_clear_state_flags(ei); /* Only relevant on 32-bit archs */
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index b00315d..593f3bf 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3492,10 +3492,13 @@  struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 	if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
 			EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) {
 		struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+		__u32 crc;
 		__le32 inum = cpu_to_le32(inode->i_ino);
-		ei->i_uuid_inum_crc = ext4_chksum(sbi, sbi->s_uuid_crc,
-						  (__u8 *)&inum,
-						  sizeof(inum));
+		__le32 gen = raw_inode->i_generation;
+		crc = ext4_chksum(sbi, sbi->s_uuid_crc, (__u8 *)&inum,
+				  sizeof(inum));
+		ei->i_uuid_inum_crc = ext4_chksum(sbi, crc, (__u8 *)&gen,
+						  sizeof(gen));
 	}
 
 	if (!ext4_inode_csum_verify(inode, raw_inode, ei)) {
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index f18bfe3..fdf0b1e 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -149,6 +149,10 @@  flags_out:
 		if (!inode_owner_or_capable(inode))
 			return -EPERM;
 
+		if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
+				EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
+			return -ENOTTY;
+
 		err = mnt_want_write(filp->f_path.mnt);
 		if (err)
 			return err;