fs/inode.c: make inode_init_always() initialize i_ino to 0

From: Eric Biggers <[email protected]>

Currently inode_init_always() doesn't initialize i_ino to 0. This is unexpected because unlike the other inode fields that aren't initialized by inode_init_always(), i_ino isn't guaranteed to end up back at its initial value after the inode is freed. Only one filesystem (XFS) actually sets set i_ino back to 0 when freeing its inodes.

So, callers of new_inode() see some random previous i_ino. Normally that's fine, since normally i_ino isn't accessed before being set. There can be edge cases where that isn't necessarily true, though.

The one I've run into is that on ext4, when creating an encrypted file, the new file's encryption key has to be set up prior to the jbd2 transaction, and thus prior to i_ino being set. If something goes wrong, fs/crypto/ may log warning or error messages, which normally include i_ino. So it needs to know whether it is valid to include i_ino yet or not. Also, on some files i_ino needs to be hashed for use in the crypto, so fs/crypto/ needs to know whether that can be done yet or not.

There are ways this could be worked around, either in fs/crypto/ or in fs/ext4/. But, it seems there's no reason not to just fix inode_init_always() to do the expected thing and initialize i_ino to 0.

So, do that, and also remove the initialization in jfs_fill_super() that becomes redundant.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/inode.c     | 1 +
 fs/jfs/super.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

fs/inode.c +1 -0

index 9d78c37b00b81..eb001129f157c 100644
@@ -142,6 +142,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	atomic_set(&inode->i_count, 1);
 	inode->i_op = &empty_iops;
 	inode->i_fop = &no_open_fops;
+	inode->i_ino = 0;
 	inode->__i_nlink = 1;
 	inode->i_opflags = 0;
 	if (sb->s_xattr)

fs/jfs/super.c +0 -0

index b2dc4d1f9dcc5..1f0ffabbde566 100644

Quoted 52 lines · Eric Biggers

From: Eric Biggers <[email protected]>

Currently inode_init_always() doesn't initialize i_ino to 0. This is unexpected because unlike the other inode fields that aren't initialized by inode_init_always(), i_ino isn't guaranteed to end up back at its initial value after the inode is freed. Only one filesystem (XFS) actually sets set i_ino back to 0 when freeing its inodes.

So, callers of new_inode() see some random previous i_ino. Normally that's fine, since normally i_ino isn't accessed before being set. There can be edge cases where that isn't necessarily true, though.

The one I've run into is that on ext4, when creating an encrypted file, the new file's encryption key has to be set up prior to the jbd2 transaction, and thus prior to i_ino being set. If something goes wrong, fs/crypto/ may log warning or error messages, which normally include i_ino. So it needs to know whether it is valid to include i_ino yet or not. Also, on some files i_ino needs to be hashed for use in the crypto, so fs/crypto/ needs to know whether that can be done yet or not.

There are ways this could be worked around, either in fs/crypto/ or in fs/ext4/. But, it seems there's no reason not to just fix inode_init_always() to do the expected thing and initialize i_ino to 0.

So, do that, and also remove the initialization in jfs_fill_super() that becomes redundant.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/inode.c     | 1 +
 fs/jfs/super.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

fs/inode.c +1 -0

index 9d78c37b00b81..eb001129f157c 100644
@@ -142,6 +142,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	atomic_set(&inode->i_count, 1);
 	inode->i_op = &empty_iops;
 	inode->i_fop = &no_open_fops;
+	inode->i_ino = 0;
 	inode->__i_nlink = 1;
 	inode->i_opflags = 0;
 	if (sb->s_xattr)

fs/jfs/super.c +0 -0

index b2dc4d1f9dcc5..1f0ffabbde566 100644
@@ -551,7 +551,6 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent)
 		ret = -ENOMEM;
 		goto out_unload;

fs/jfs/super.c static int jfs_fill_super(struct super_block *sb, void *data, int silent) · line 556

Quoted 6 lines

fs/jfs/super.c +0 -1

 	}
-	inode->i_ino = 0;
 	inode->i_size = i_size_read(sb->s_bdev->bd_inode);
 	inode->i_mapping->a_ops = &jfs_metapage_aops;
 	inode_fake_hash(inode);

Al, any thoughts on this?

Quoted 53 lines · Eric Biggers

Quoted 52 lines · Eric Biggers

From: Eric Biggers <[email protected]>

Currently inode_init_always() doesn't initialize i_ino to 0. This is unexpected because unlike the other inode fields that aren't initialized by inode_init_always(), i_ino isn't guaranteed to end up back at its initial value after the inode is freed. Only one filesystem (XFS) actually sets set i_ino back to 0 when freeing its inodes.

So, callers of new_inode() see some random previous i_ino. Normally that's fine, since normally i_ino isn't accessed before being set. There can be edge cases where that isn't necessarily true, though.

The one I've run into is that on ext4, when creating an encrypted file, the new file's encryption key has to be set up prior to the jbd2 transaction, and thus prior to i_ino being set. If something goes wrong, fs/crypto/ may log warning or error messages, which normally include i_ino. So it needs to know whether it is valid to include i_ino yet or not. Also, on some files i_ino needs to be hashed for use in the crypto, so fs/crypto/ needs to know whether that can be done yet or not.

There are ways this could be worked around, either in fs/crypto/ or in fs/ext4/. But, it seems there's no reason not to just fix inode_init_always() to do the expected thing and initialize i_ino to 0.

So, do that, and also remove the initialization in jfs_fill_super() that becomes redundant.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/inode.c     | 1 +
 fs/jfs/super.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

fs/inode.c +1 -0

index 9d78c37b00b81..eb001129f157c 100644
@@ -142,6 +142,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	atomic_set(&inode->i_count, 1);
 	inode->i_op = &empty_iops;
 	inode->i_fop = &no_open_fops;
+	inode->i_ino = 0;
 	inode->__i_nlink = 1;
 	inode->i_opflags = 0;
 	if (sb->s_xattr)

fs/jfs/super.c +0 -0

index b2dc4d1f9dcc5..1f0ffabbde566 100644
@@ -551,7 +551,6 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent)
 		ret = -ENOMEM;
 		goto out_unload;

Quoted 8 lines

fs/jfs/super.c static int jfs_fill_super(struct super_block *sb, void *data, int silent) · line 556

Quoted 6 lines

fs/jfs/super.c +0 -1

 	}
-	inode->i_ino = 0;
 	inode->i_size = i_size_read(sb->s_bdev->bd_inode);
 	inode->i_mapping->a_ops = &jfs_metapage_aops;
 	inode_fake_hash(inode);

Al, any thoughts on this?

Quoted 54 lines · Eric Biggers

Quoted 53 lines · Eric Biggers

Quoted 52 lines · Eric Biggers

From: Eric Biggers <[email protected]>

Currently inode_init_always() doesn't initialize i_ino to 0. This is unexpected because unlike the other inode fields that aren't initialized by inode_init_always(), i_ino isn't guaranteed to end up back at its initial value after the inode is freed. Only one filesystem (XFS) actually sets set i_ino back to 0 when freeing its inodes.

So, callers of new_inode() see some random previous i_ino. Normally that's fine, since normally i_ino isn't accessed before being set. There can be edge cases where that isn't necessarily true, though.

The one I've run into is that on ext4, when creating an encrypted file, the new file's encryption key has to be set up prior to the jbd2 transaction, and thus prior to i_ino being set. If something goes wrong, fs/crypto/ may log warning or error messages, which normally include i_ino. So it needs to know whether it is valid to include i_ino yet or not. Also, on some files i_ino needs to be hashed for use in the crypto, so fs/crypto/ needs to know whether that can be done yet or not.

There are ways this could be worked around, either in fs/crypto/ or in fs/ext4/. But, it seems there's no reason not to just fix inode_init_always() to do the expected thing and initialize i_ino to 0.

So, do that, and also remove the initialization in jfs_fill_super() that becomes redundant.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/inode.c     | 1 +
 fs/jfs/super.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

fs/inode.c +1 -0

index 9d78c37b00b81..eb001129f157c 100644
@@ -142,6 +142,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	atomic_set(&inode->i_count, 1);
 	inode->i_op = &empty_iops;
 	inode->i_fop = &no_open_fops;
+	inode->i_ino = 0;
 	inode->__i_nlink = 1;
 	inode->i_opflags = 0;
 	if (sb->s_xattr)

fs/jfs/super.c +0 -0

index b2dc4d1f9dcc5..1f0ffabbde566 100644
@@ -551,7 +551,6 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent)
 		ret = -ENOMEM;
 		goto out_unload;

Quoted 8 lines

Quoted 8 lines

fs/jfs/super.c static int jfs_fill_super(struct super_block *sb, void *data, int silent) · line 556

Quoted 6 lines

fs/jfs/super.c +0 -1

 	}
-	inode->i_ino = 0;
 	inode->i_size = i_size_read(sb->s_bdev->bd_inode);
 	inode->i_mapping->a_ops = &jfs_metapage_aops;
 	inode_fake_hash(inode);

Al, any thoughts on this?

Ping.

Quoted 64 lines · Eric Biggers

Quoted 62 lines · Eric Biggers

Quoted 61 lines · Eric Biggers

Quoted 51 lines · Eric Biggers

From: Eric Biggers <[email protected]>

Currently inode_init_always() doesn't initialize i_ino to 0. This is unexpected because unlike the other inode fields that aren't initialized by inode_init_always(), i_ino isn't guaranteed to end up back at its initial value after the inode is freed. Only one filesystem (XFS) actually sets set i_ino back to 0 when freeing its inodes.

So, callers of new_inode() see some random previous i_ino. Normally that's fine, since normally i_ino isn't accessed before being set. There can be edge cases where that isn't necessarily true, though.

The one I've run into is that on ext4, when creating an encrypted file, the new file's encryption key has to be set up prior to the jbd2 transaction, and thus prior to i_ino being set. If something goes wrong, fs/crypto/ may log warning or error messages, which normally include i_ino. So it needs to know whether it is valid to include i_ino yet or not. Also, on some files i_ino needs to be hashed for use in the crypto, so fs/crypto/ needs to know whether that can be done yet or not.

There are ways this could be worked around, either in fs/crypto/ or in fs/ext4/. But, it seems there's no reason not to just fix inode_init_always() to do the expected thing and initialize i_ino to 0.

So, do that, and also remove the initialization in jfs_fill_super() that becomes redundant.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/inode.c     | 1 +
 fs/jfs/super.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

fs/inode.c +1 -0

index 9d78c37b00b81..eb001129f157c 100644
@@ -142,6 +142,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	atomic_set(&inode->i_count, 1);
 	inode->i_op = &empty_iops;
 	inode->i_fop = &no_open_fops;
+	inode->i_ino = 0;
 	inode->__i_nlink = 1;
 	inode->i_opflags = 0;
 	if (sb->s_xattr)

fs/jfs/super.c +0 -0

index b2dc4d1f9dcc5..1f0ffabbde566 100644

fs/jfs/super.c static int jfs_fill_super(struct super_block *sb, void *data, int silent) · line 556

Quoted 7 lines

fs/jfs/super.c +0 -1

@@ -551,7 +551,6 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent)
 		ret = -ENOMEM;
 		goto out_unload;
 	}
-	inode->i_ino = 0;
 	inode->i_size = i_size_read(sb->s_bdev->bd_inode);
 	inode->i_mapping->a_ops = &jfs_metapage_aops;
 	inode_fake_hash(inode);

Al, any thoughts on this?

Quoted 1 line

Ping.

Ping.

From: Eric Biggers <[email protected]>

Currently inode_init_always() doesn't initialize i_ino to 0. This is unexpected because unlike the other inode fields that aren't initialized by inode_init_always(), i_ino isn't guaranteed to end up back at its initial value after the inode is freed. Only one filesystem (XFS) actually sets set i_ino back to 0 when freeing its inodes.

So, callers of new_inode() see some random previous i_ino. Normally that's fine, since normally i_ino isn't accessed before being set. There can be edge cases where that isn't necessarily true, though.

The one I've run into is that on ext4, when creating an encrypted file, the new file's encryption key has to be set up prior to the jbd2 transaction, and thus prior to i_ino being set. If something goes wrong, fs/crypto/ may log warning or error messages, which normally include i_ino. So it needs to know whether it is valid to include i_ino yet or not. Also, on some files i_ino needs to be hashed for use in the crypto, so fs/crypto/ needs to know whether that can be done yet or not.

There are ways this could be worked around, either in fs/crypto/ or in fs/ext4/. But, it seems there's no reason not to just fix inode_init_always() to do the expected thing and initialize i_ino to 0.

So, do that, and also remove the initialization in jfs_fill_super() that becomes redundant.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/inode.c | 1 +
 fs/jfs/super.c | 1 -

 2 files changed, 1 insertion(+), 1 deletion(-)

fs/inode.c +1 -0

index 9d78c37b00b81..eb001129f157c 100644
@@ -142,6 +142,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
  	atomic_set(&inode->i_count, 1);
  	inode->i_op = &empty_iops;
  	inode->i_fop = &no_open_fops;
+	inode->i_ino = 0;
  	inode->__i_nlink = 1;
  	inode->i_opflags = 0;
  	if (sb->s_xattr)

fs/jfs/super.c +0 -1

index b2dc4d1f9dcc5..1f0ffabbde566 100644
@@ -551,7 +551,6 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent)
  		ret = -ENOMEM;
  		goto out_unload;
  	}
-	inode->i_ino = 0;
  	inode->i_size = i_size_read(sb->s_bdev->bd_inode);
  	inode->i_mapping->a_ops = &jfs_metapage_aops;
  	inode_fake_hash(inode);

base-commit: 5fc6b075e165f641fbc366b58b578055762d5f8c