Hello,
> 04:23 < bwh> Send mail to [email protected] and cc [email protected]
Forwarding debian bug https://bugs.debian.org/738758
On Wed, Feb 12, 2014 at 07:55:34PM +0100, Gabriele Giacone wrote:
> Package: src:linux
> Version: 3.12.9-1
> Severity: normal
>
> Since CONFIG_EXT4_USE_FOR_EXT23 has been enabled, ext4 module can't properly
> handle ext2 created for Hurd causing data corruption.
> ext2 module would do its job.
>
> Reproducible with:
>
> dd if=/dev/zero of=test.fs count=10000
> echo y|mkfs.ext2 -o hurd test.fs
> mount -t ext2 test.fs /mnt
> touch /mnt/bug0000
> umount /mnt
> fsck.ext2 -fp test.fs
>
> Attached output with kernel 3.10-3 and 3.12-1.
>
> + dd if=/dev/zero of=test.fs count=10000
> 10000+0 records in
> 10000+0 records out
> 5120000 bytes (5.1 MB) copied, 0.0459037 s, 112 MB/s
> + echo y
> + mkfs.ext2 -o hurd test.fs
> mke2fs 1.42.9 (28-Dec-2013)
> test.fs is not a block special device.
> Proceed anyway? (y,n) Discarding device blocks: done
> Filesystem label=
> OS type: Hurd
> Block size=4096 (log=2)
> Fragment size=4096 (log=2)
> Stride=0 blocks, Stripe width=0 blocks
> 1280 inodes, 1250 blocks
> 62 blocks (4.96%) reserved for the super user
> First data block=0
> 1 block group
> 32768 blocks per group, 32768 fragments per group
> 1280 inodes per group
>
> Allocating group tables: 0/1 done
> Writing inode tables: 0/1 done
> Writing superblocks and filesystem accounting information: 0/1 done
>
> + mount -t ext2 test.fs /mnt
> + touch /mnt/bug0000
> + umount /mnt
> + fsck.ext2 -fp test.fs
> test.fs: 12/1280 files (0.0% non-contiguous), 50/1250 blocks
> + dd if=/dev/zero of=test.fs count=10000
> 10000+0 records in
> 10000+0 records out
> 5120000 bytes (5.1 MB) copied, 0.0227442 s, 225 MB/s
> + echo y
> s.ext2 -o hurd test.fs
> mke2fs 1.42.9 (28-Dec-2013)
> test.fs is not a block special device.
> Proceed anyway? (y,n) Discarding device blocks: done
> Filesystem label=
> OS type: Hurd
> Block size=4096 (log=2)
> Fragment size=4096 (log=2)
> Stride=0 blocks, Stripe width=0 blocks
> 1280 inodes, 1250 blocks
> 62 blocks (4.96%) reserved for the super user
> First data block=0
> 1 block group
> 32768 blocks per group, 32768 fragments per group
> 1280 inodes per group
>
> Allocating group tables: 0/1 done
> Writing inode tables: 0/1 done
> Writing superblocks and filesystem accounting information: 0/1 done
>
> + mount -t ext2 test.fs /mnt
> + touch /mnt/bug0000
> + umount /mnt
> + fsck.ext2 -fp test.fs
> test.fs: Inode 2, i_blocks is 8, should be 16. FIXED.
> test.fs: Inode 12, i_blocks is 0, should be 8. FIXED.
> test.fs: Duplicate or bad block in use!
> test.fs: Multiply-claimed block(s) in inode 2: 1
> test.fs: Multiply-claimed block(s) in inode 12: 1
> test.fs: (There are 2 inodes containing multiply-claimed blocks.)
>
> test.fs: File / (inode #2, mod time Wed Feb 12 19:02:45 2014)
> has 1 multiply-claimed block(s), shared with 2 file(s):
> test.fs: <filesystem metadata>
> test.fs: /bug0000 (inode #12, mod time Wed Feb 12 19:02:45 2014)
> test.fs:
>
> test.fs: UNEXPECTED INCONSISTENCY; RUN fsck MANUALLY.
> (i.e., without -a or -p options)
--
G..e
The Hurd file system uses uses the inode field which is now used for
i_version for its translator block. This means that ext2 file systems
that are formatted for GNU Hurd can't be used to support NFSv4. Given
that Hurd file systems don't support extents, and a huge number of
modern file system features, this is no great loss.
If we don't do this, the attempt to update the i_version field will
stomp over the translator block field, which will cause file system
corruption for Hurd file systems. This can be replicated via:
mke2fs -t ext2 -o hurd /dev/vdc
mount -t ext4 /dev/vdc /vdc
touch /vdc/bug0000
umount /dev/vdc
e2fsck -f /dev/vdc
Addresses-Debian-Bug: #738758
Reported-By: Gabriele Giacone <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
fs/ext4/inode.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 7cc2455..ed2c13a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4168,11 +4168,14 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
EXT4_INODE_GET_XTIME(i_atime, inode, raw_inode);
EXT4_EINODE_GET_XTIME(i_crtime, ei, raw_inode);
- inode->i_version = le32_to_cpu(raw_inode->i_disk_version);
- if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
- if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
- inode->i_version |=
- (__u64)(le32_to_cpu(raw_inode->i_version_hi)) << 32;
+ if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
+ cpu_to_le32(EXT4_OS_HURD)) {
+ inode->i_version = le32_to_cpu(raw_inode->i_disk_version);
+ if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
+ if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
+ inode->i_version |=
+ (__u64)(le32_to_cpu(raw_inode->i_version_hi)) << 32;
+ }
}
ret = 0;
@@ -4388,12 +4391,16 @@ static int ext4_do_update_inode(handle_t *handle,
raw_inode->i_block[block] = ei->i_data[block];
}
- raw_inode->i_disk_version = cpu_to_le32(inode->i_version);
- if (ei->i_extra_isize) {
- if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
- raw_inode->i_version_hi =
- cpu_to_le32(inode->i_version >> 32);
- raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
+ if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
+ cpu_to_le32(EXT4_OS_HURD)) {
+ raw_inode->i_disk_version = cpu_to_le32(inode->i_version);
+ if (ei->i_extra_isize) {
+ if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
+ raw_inode->i_version_hi =
+ cpu_to_le32(inode->i_version >> 32);
+ raw_inode->i_extra_isize =
+ cpu_to_le16(ei->i_extra_isize);
+ }
}
ext4_inode_csum_set(inode, raw_inode, ei);
--
1.9.0
Probably worthwhile to make those !EXT4_OS_HURD checks likely()?
Cheers, Andreas
> On Mar 19, 2014, at 22:34, Theodore Ts'o <[email protected]> wrote:
>
> The Hurd file system uses uses the inode field which is now used for
> i_version for its translator block. This means that ext2 file systems
> that are formatted for GNU Hurd can't be used to support NFSv4. Given
> that Hurd file systems don't support extents, and a huge number of
> modern file system features, this is no great loss.
>
> If we don't do this, the attempt to update the i_version field will
> stomp over the translator block field, which will cause file system
> corruption for Hurd file systems. This can be replicated via:
>
> mke2fs -t ext2 -o hurd /dev/vdc
> mount -t ext4 /dev/vdc /vdc
> touch /vdc/bug0000
> umount /dev/vdc
> e2fsck -f /dev/vdc
>
> Addresses-Debian-Bug: #738758
>
> Reported-By: Gabriele Giacone <[email protected]>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> ---
> fs/ext4/inode.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 7cc2455..ed2c13a 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4168,11 +4168,14 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
> EXT4_INODE_GET_XTIME(i_atime, inode, raw_inode);
> EXT4_EINODE_GET_XTIME(i_crtime, ei, raw_inode);
>
> - inode->i_version = le32_to_cpu(raw_inode->i_disk_version);
> - if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
> - if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
> - inode->i_version |=
> - (__u64)(le32_to_cpu(raw_inode->i_version_hi)) << 32;
> + if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
> + cpu_to_le32(EXT4_OS_HURD)) {
> + inode->i_version = le32_to_cpu(raw_inode->i_disk_version);
> + if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
> + if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
> + inode->i_version |=
> + (__u64)(le32_to_cpu(raw_inode->i_version_hi)) << 32;
> + }
> }
>
> ret = 0;
> @@ -4388,12 +4391,16 @@ static int ext4_do_update_inode(handle_t *handle,
> raw_inode->i_block[block] = ei->i_data[block];
> }
>
> - raw_inode->i_disk_version = cpu_to_le32(inode->i_version);
> - if (ei->i_extra_isize) {
> - if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
> - raw_inode->i_version_hi =
> - cpu_to_le32(inode->i_version >> 32);
> - raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
> + if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
> + cpu_to_le32(EXT4_OS_HURD)) {
> + raw_inode->i_disk_version = cpu_to_le32(inode->i_version);
> + if (ei->i_extra_isize) {
> + if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
> + raw_inode->i_version_hi =
> + cpu_to_le32(inode->i_version >> 32);
> + raw_inode->i_extra_isize =
> + cpu_to_le16(ei->i_extra_isize);
> + }
> }
>
> ext4_inode_csum_set(inode, raw_inode, ei);
> --
> 1.9.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 19, 2014 at 11:27:57PM -0600, Andreas Dilger wrote:
> Probably worthwhile to make those !EXT4_OS_HURD checks likely()?
Does it make sense to support the format at all given that it's unlikely
to get any testing?
On Thu, Mar 20, 2014 at 01:10:48AM -0700, Christoph Hellwig wrote:
> On Wed, Mar 19, 2014 at 11:27:57PM -0600, Andreas Dilger wrote:
> > Probably worthwhile to make those !EXT4_OS_HURD checks likely()?
Yes, and I was planning on optimizing the checks a bit more, but it
makes sense to do that in a separate patch, since this is not the only
place where we are making EXT4_OS_HURD checks.
>
> Does it make sense to support the format at all given that it's unlikely
> to get any testing?
There are some crazy people still trying to make the Hurd a viable
file system. There's even a Debian port for it. :-) The problem is
that some of the folks who are still trying to make the Hurd real want
to use ext2 as an interchange format between Linux and Hurd, and
presumably that's how they ran across this particular bug.
While I think Hurd is a joke, and it's laughable that the primary file
system for the Hurd doesn't support journalling, or extents, delayed
allocation, or extended attributes[1], and it's hard for them get feature
parity because of the GPL v3 vs v2 compatibility problem, I don't mind
making minimal efforts to provide legacy support to the GNU Hurd.
[1] http://savannah.gnu.org/patch/?5126 (Patches available for the last
seven years, not yet integrated)
In the long run, the right answer is that the Hurd should use an
extended attribute to store the translator information. Indeed, this
has been discussed seven years ago[2]. But unfortunately, the
development temp for Hurd is a wee bit slower than for the Linux
kernel. :-)
[2] http://savannah.gnu.org/task/?5503
- Ted
On Thu, 2014-03-20 at 10:44 -0400, [email protected] wrote:
> On Thu, Mar 20, 2014 at 01:10:48AM -0700, Christoph Hellwig wrote:
> > On Wed, Mar 19, 2014 at 11:27:57PM -0600, Andreas Dilger wrote:
> > > Probably worthwhile to make those !EXT4_OS_HURD checks likely()?
>
> Yes, and I was planning on optimizing the checks a bit more, but it
> makes sense to do that in a separate patch, since this is not the only
> place where we are making EXT4_OS_HURD checks.
>
> >
> > Does it make sense to support the format at all given that it's unlikely
> > to get any testing?
>
> There are some crazy people still trying to make the Hurd a viable
> file system. There's even a Debian port for it. :-) The problem is
> that some of the folks who are still trying to make the Hurd real want
> to use ext2 as an interchange format between Linux and Hurd, and
> presumably that's how they ran across this particular bug.
[...]
That, plus we turned on CONFIG_EXT4_USE_FOR_EXT23 for Debian kernel
packages starting with Linux 3.11.
It looks like ext2 and ext3 would always initialise i_version to 1 in
memory; does it matter that you're changing that to 0 for Hurd
filesystems?
Ben.
--
Ben Hutchings
One of the nice things about standards is that there are so many of them.
On Thu, Mar 20, 2014 at 05:30:32PM +0000, Ben Hutchings wrote:
>
> It looks like ext2 and ext3 would always initialise i_version to 1 in
> memory; does it matter that you're changing that to 0 for Hurd
> filesystems?
No, NFS only cares that the i_version number has changed, and it's
mainly important if you have two clients trying to simultaneously
access the same file, so they get a signal that they need to
invalidate their locally cached metadata (or data, in the case of
NFSv4). But Hurd only supports NFSv2, and the performance is such
that I doubt anyone would be all that interested in using a Hurd
server as a NFS server for even a small workgroup, let alone a
department, so this is unlikely to be a big deal.
Cheers,
- Ted
Set a in-memory superblock flag to indicate whether the file system is
designed to support the Hurd.
Also, add a sanity check to make sure the 64-bit feature is not set
for Hurd file systems, since i_file_acl_high conflicts with a
Hurd-specific field. This is not a big deal, since Hurd doesn't
support file systems larger than 1GB[1] anyway.
[1] http://walfield.org/pub/people/neal/papers/hurd-faq/FAQ.en.html#q2-6
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
fs/ext4/ext4.h | 2 ++
fs/ext4/inode.c | 9 +++------
fs/ext4/super.c | 10 ++++++++++
3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index f4f889e..e01135d 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1001,6 +1001,8 @@ struct ext4_inode_info {
#define EXT4_MOUNT2_STD_GROUP_SIZE 0x00000002 /* We have standard group
size of blocksize * 8
blocks */
+#define EXT4_MOUNT2_HURD_COMPAT 0x00000004 /* Support HURD-castrated
+ file systems */
#define clear_opt(sb, opt) EXT4_SB(sb)->s_mount_opt &= \
~EXT4_MOUNT_##opt
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index ed2c13a..a3a5eb3 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4168,8 +4168,7 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
EXT4_INODE_GET_XTIME(i_atime, inode, raw_inode);
EXT4_EINODE_GET_XTIME(i_crtime, ei, raw_inode);
- if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
- cpu_to_le32(EXT4_OS_HURD)) {
+ if (!test_opt2(inode->i_sb, HURD_COMPAT)) {
inode->i_version = le32_to_cpu(raw_inode->i_disk_version);
if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
@@ -4345,8 +4344,7 @@ static int ext4_do_update_inode(handle_t *handle,
goto out_brelse;
raw_inode->i_dtime = cpu_to_le32(ei->i_dtime);
raw_inode->i_flags = cpu_to_le32(ei->i_flags & 0xFFFFFFFF);
- if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
- cpu_to_le32(EXT4_OS_HURD))
+ if (!test_opt2(inode->i_sb, HURD_COMPAT))
raw_inode->i_file_acl_high =
cpu_to_le16(ei->i_file_acl >> 32);
raw_inode->i_file_acl_lo = cpu_to_le32(ei->i_file_acl);
@@ -4391,8 +4389,7 @@ static int ext4_do_update_inode(handle_t *handle,
raw_inode->i_block[block] = ei->i_data[block];
}
- if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
- cpu_to_le32(EXT4_OS_HURD)) {
+ if (!test_opt2(inode->i_sb, HURD_COMPAT)) {
raw_inode->i_disk_version = cpu_to_le32(inode->i_version);
if (ei->i_extra_isize) {
if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 3c9eadf..6adee9a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3580,6 +3580,16 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
"feature flags set on rev 0 fs, "
"running e2fsck is recommended");
+ if (es->s_creator_os == cpu_to_le32(EXT4_OS_HURD)) {
+ set_opt2(sb, HURD_COMPAT);
+ if (EXT4_HAS_INCOMPAT_FEATURE(sb,
+ EXT4_FEATURE_INCOMPAT_64BIT)) {
+ ext4_msg(sb, KERN_ERR,
+ "The Hurd can't support 64-bit file systems");
+ goto failed_mount;
+ }
+ }
+
if (IS_EXT2_SB(sb)) {
if (ext2_feature_set_ok(sb))
ext4_msg(sb, KERN_INFO, "mounting ext2 file system "
--
1.9.0
Theodore Ts'o, le Fri 21 Mar 2014 12:48:53 -0400, a ?crit :
> Also, add a sanity check to make sure the 64-bit feature is not set
> for Hurd file systems, since i_file_acl_high conflicts with a
> Hurd-specific field. This is not a big deal, since Hurd doesn't
> support file systems larger than 1GB[1] anyway.
Err, it does. We regularly use volumes with dozens of GiB.
> [1] http://walfield.org/pub/people/neal/papers/hurd-faq/FAQ.en.html#q2-6
That faq is very largely outdated. See the official faq on
http://www.gnu.org/software/hurd/faq.html
Samuel
--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]