Re: [parted-devel] [parted 1.8.8] msdos label and file system corruption issues with > 2TB disk

[Michael Reed]

Michael Reed wrote:
> Comments below.

More comments below....

> 
> Jim Meyering wrote:
>> Michael Reed <address@hidden> wrote:
>>> (I'm copying parted-devel as this ties into GPT-MBR hybrid discussion in 
>>> that
>>> there is a need to be able to boot BIOS based systems from disks larger than
>>> 2TB and still be able to address all the storage.)
>>>
>>> Using an on board hardware raid5 "disk" of 5,851,561,984 512 byte sectors,
>>> or roughly 2.9TB, we installed name_brand_Enterprise_distro.  We created an
>>> 800 MB swap partition (1), a 25GB root partition (2), and the rest of the 
>>> space
>>> to a large scratch xfs file system.
>>>
>>> The OS installation went fine and upon reboot the system complained that the
>>> large scratch file system was, essentially, corrupt.  Here's my analysis of
>>> what happened.
>>>
>>> Via analysis of the MSDOS partition table, I can state that it can address
>>> a maximum of 4TB of space.  Each partition entry consists of a starting 
>>> block
>>> and a length.  Both of these fields are 32 bits in length.  No individual
>>> partition can begin beyond 2TB.  No individual partition can be longer than
>>> 2TB.  In order to address 4TB, at least two partition table entries must be
>>> used, for example:
>>>
>>>     start           length
>>>     0x00000000      0xfffffffe
>>>     0xffffffff      0xffffffff
>>>
>>> (It's interesting to note that Linux will handle 4TB of space defined via
>>> the msdos label, and Windows 2003/2008 appears to stop at 2TB.)
>>>
>>> Working with parted after the system was up I can draw these conclusions:
>>>
>>>     1) parted doesn't complain that a partition length is too long
>>>        for the msdos label in which it will be stored.
>>>
>>>     2) parted "truncates" the partition
>>>
>>>     3) parted "lies" to the OS about the size of the partition.
>> Thank you for the detailed bug report!
>> With the patch below, I've fixed Parted so it won't do that any more.
> 
> I will try to find real hardware with which to test this today.  I'll
> let you know how it goes.  Please hold off on posting the patch until
> I can play with it.  You'll hear back from me Monday (CA business
> hours) at the latest.
> 
> Does this effect the ability to create > 2TB with GPT labels?

If I had READ THE PATCH more closely, I would have noticed the conditioning
on the msdos label:

        if (!(part->type & PED_PARTITION_METADATA)
            && strcmp (disk->type->name, "msdos") == 0) {

This clearly doesn't effect GPT labels.

Mike

> 
> Thank you for taking the time to do this work.
> 
> Comments within the patch....
> 
>> The patch to parted is deceptively simple looking.
>> Just finding the right place in the call tree to insert the
>> tests was not easy.  Name/semantic mismatches (and lack of comments)
>> led me to experiment with two others first.
>>
>> An early version of the patch did not have this guard condition:
>>
>>   if (!(part->type & PED_PARTITION_METADATA)
>>
>> Everything appeared to work until I tried to *print*
>> a partition table containing the largest (2TB-1-sector) partition.
>> Without the guard, it'd complain about the starting sector being too large.
>> The actual sector number may have been 2^32+2k.
>>
>> Designing the tests was a challenge, too.
>> Just creating a file that parted can use as a "device" in which to
>> create such a large partition (>= 2TB) is tricky, since many common
>> file system types have a maximum file size of just under 2TB.
>> But thanks to sparse files, the actual space required is just a few MB.
>>
>> Since I ended up choosing to create and mount an XFS partition, the test
>> does useful work only when run by root.  You may also need to install
>> the xfsprogs package to get mkfs.xfs.  You can run the test manually
>> like this:
>>
>>   (cd tests && sudo ./t4100-msdos-partition-limits.sh --verbose)
>>
>> Here's the change-set I expect to push today or tomorrow:
>> -----------------
>> >From 1bf00ea84e80d9f04aed3c4a15ddb307b1807f5e Mon Sep 17 00:00:00 2001
>> From: Jim Meyering <address@hidden>
>> Date: Thu, 10 Jan 2008 14:51:56 +0100
>> Subject: [PATCH] Enforce inherent limitations of the dos partition table 
>> format.
>>
>> * libparted/disk.c (_check_partition): Enforce the 32-bit limitation
>> on a partition's starting sector number and length (in sectors).
>> With the usual 512-byte sector size, this limits the maximum
>> partition size to just under 2TB.
>> * tests/t4100-msdos-partition-limits.sh: New file.  Test the above.
>> * tests/Makefile.am (TESTS): Add t4100-msdos-partition-limits.sh.
>>
>> Signed-off-by: Jim Meyering <address@hidden>
>> ---
>>  libparted/disk.c                      |   41 ++++++++-
>>  tests/Makefile.am                     |    3 +-
>>  tests/t4100-msdos-partition-limits.sh |  169 
>> +++++++++++++++++++++++++++++++++
>>  3 files changed, 211 insertions(+), 2 deletions(-)
>>  create mode 100755 tests/t4100-msdos-partition-limits.sh
>>
>> diff --git a/libparted/disk.c b/libparted/disk.c
>> index 087fbbf..135e230 100644
>> --- a/libparted/disk.c
>> +++ b/libparted/disk.c
>> @@ -1,6 +1,6 @@
>>   /*
>>      libparted - a library for manipulating disk partitions
>> -    Copyright (C) 1999, 2000, 2001, 2002, 2003, 2005, 2007
>> +    Copyright (C) 1999, 2000, 2001, 2002, 2003, 2005, 2007, 2008
>>                    Free Software Foundation, Inc.
>>
>>      This program is free software; you can redistribute it and/or modify
>> @@ -1735,6 +1735,45 @@ _check_partition (PedDisk* disk, PedPartition* part)
>>              return 0;
>>      }
>>
>> +    if (!(part->type & PED_PARTITION_METADATA)
>> +            && strcmp (disk->type->name, "msdos") == 0) {
>> +            /* Enforce some restrictions inherent in the DOS
>> +               partition table format.  Without these, one would be able
>> +               to create a 2TB partition (or larger), and it would work,
>> +               but only until the next reboot.  This was insidious: the
>> +               too-large partition would work initially, because with
>> +               Linux-2.4.x and newer we set the partition start sector
>> +               and length (in sectors) accurately and directly via the
>> +               BLKPG ioctl.  However, only the last 32 bits of each
>> +               number would be written to the partition table, and the
>> +               next time the system would read/use those corrupted numbers
>> +               it would usually complain about an invalid partition.
>> +                   The same applies to the starting sector number.  */
>> +
>> +            /* The partition length, in sectors, must fit in 32 bytes.  */
> 
> You probably want this to be "32 bits".
> 
>> +            if (part->geom.length > UINT32_MAX) {
>> +                    ped_exception_throw (
>> +                            PED_EXCEPTION_ERROR,
>> +                            PED_EXCEPTION_CANCEL,
>> +                            _("partition length of %jd sectors exceeds"
>> +                                  " the DOS-partition-table-imposed maximum"
>> +                                  " of 2^32-1"),
>> +                            part->geom.length);
>> +                    return 0;
>> +            }
>> +
>> +            /* The starting sector number must fit in 32 bytes.  */
>> +            if (part->geom.start > UINT32_MAX) {
>> +                    ped_exception_throw (
>> +                            PED_EXCEPTION_ERROR,
>> +                            PED_EXCEPTION_CANCEL,
>> +                            _("starting sector number, %jd exceeds"
>> +                                  " the DOS-partition-table-imposed maximum"
>> +                                  " of 2^32-1"), part->geom.start);
>> +                    return 0;
>> +            }
>> +    }
>> +
>>      return 1;
>>  }
>>
>> diff --git a/tests/Makefile.am b/tests/Makefile.am
>> index 3a3020e..b9cd205 100644
>> --- a/tests/Makefile.am
>> +++ b/tests/Makefile.am
>> @@ -7,7 +7,8 @@ TESTS = \
>>    t2000-mkfs.sh \
>>    t2100-mkswap.sh \
>>    t3000-constraints.sh \
>> -  t3100-resize-ext2-partion.sh
>> +  t3100-resize-ext2-partion.sh \
>> +  t4100-msdos-partition-limits.sh
>>
>>  EXTRA_DIST = \
>>    $(TESTS) test-lib.sh mkdtemp
>> diff --git a/tests/t4100-msdos-partition-limits.sh 
>> b/tests/t4100-msdos-partition-limits.sh
>> new file mode 100755
>> index 0000000..13e32af
>> --- /dev/null
>> +++ b/tests/t4100-msdos-partition-limits.sh
>> @@ -0,0 +1,169 @@
>> +#!/bin/sh
>> +
>> +# Copyright (C) 2008 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +test_description='msdos: enforce limits on partition start sector and 
>> length'
>> +
>> +# Need root privileges to use mount.
>> +privileges_required_=1
>> +
>> +. ./init.sh
>> +
>> +####################################################
>> +# Create and mount a file system capable of dealing with >=2TB files.
>> +# We must be able to create a file with an apparent length of 2TB or larger.
>> +# It needn't be a large file system.
>> +fs=fs_file
>> +mp=`pwd`/mount-point
>> +n=128
>> +
>> +test_expect_success \
>> +    'create an XFS file system' \
>> +    '
>> +    dd if=/dev/zero of=$fs bs=1MB count=2 seek=20 &&
>> +    mkfs.xfs -q $fs &&
>> +    mkdir "$mp"
>> +
>> +    '
>> +
>> +# Unmount upon interrupt, failure, etc., as well as upon normal completion.
>> +cleanup_() { cd "$test_dir_" && umount "$mp" > /dev/null 2>&1; }
>> +
>> +test_expect_success \
>> +    'mount it' \
>> +    '
>> +    mount -o loop $fs "$mp" &&
>> +    cd "$mp"
>> +
>> +    '
>> +dev=loop-file
>> +
>> +do_mkpart()
>> +{
>> +  start_sector=$1
>> +  end_sector=$2
>> +  # echo '********' $(echo $end_sector - $start_sector + 1 |bc)
>> +  dd if=/dev/zero of=$dev bs=1b count=2k seek=$end_sector 2> /dev/null &&
>> +  parted -s $dev mklabel msdos &&
>> +  parted -s $dev mkpart p xfs ${start_sector}s ${end_sector}s
>> +}
>> +
>> +# Specify the starting sector number and length in sectors,
>> +# rather than start and end.
>> +do_mkpart_start_and_len()
>> +{
>> +  start_sector=$1
>> +  len=$2
>> +  end_sector=$(echo $start_sector + $len - 1|bc)
>> +  do_mkpart $start_sector $end_sector
>> +}
>> +
>> +test_expect_success \
>> +    'a partition length of 2^32-1 works.' \
>> +    '
>> +    end=$(echo $n+2^32-2|bc) &&
>> +    do_mkpart $n $end
>> +    '
>> +
>> +cat > exp <<EOF
>> +Model:  (file)
>> +Disk: 4294969470s
>> +Sector size (logical/physical): 512B/512B
>> +Partition Table: msdos
>> +
>> +Number  Start  End          Size         Type     File system  Flags
>> + 1      ${n}s   ${end}s  4294967295s  primary
>> +
>> +EOF
>> +
>> +test_expect_success \
>> +    'print the result' \
>> +    'parted -s $dev unit s p > out 2>&1 &&
>> +     sed "s/Disk .*:/Disk:/;s/ *$//" out > k && mv k out &&
>> +     diff -u out exp
>> +    '
>> +
>> +test_expect_failure \
>> +    'a partition length of exactly 2^32 sectors provokes failure.' \
>> +    'do_mkpart $n $(echo $n+2^32-1|bc) > err 2>&1'
>> +
>> +msg='Error: partition length of 4294967296 sectors exceeds the '\
>> +'DOS-partition-table-imposed maximum of 2^32-1'
>> +test_expect_success \
>> +    'check for new diagnostic' \
>> +    'echo "$msg" > exp && diff -u err exp'
>> +
>> +# FIXME: investigate this.
>> +# Unexpectedly to me, both of these failed with this same diagnostic:
>> +#
>> +#   Error: partition length of 4294967296 sectors exceeds the \
>> +#   DOS-partition-table-imposed maximum of 2^32-1" > exp &&
>> +#
>> +# I expected the one below to fail with a length of _4294967297_.
>> +# Debugging, I see that _check_partition *does* detect this,
>> +# but the diagnostic doesn't get displayed because of the wonders
>> +# of parted's exception mechanism.
>> +
>> +test_expect_failure \
>> +    'a partition length of 2^32+1 sectors provokes failure.' \
>> +    'do_mkpart $n $(echo $n+2^32|bc) > err 2>&1'
>> +
>> +test_expect_success \
>> +    'check for new diagnostic' \
>> +    'echo "$msg" > exp && diff -u err exp'
>> +
>> +# =========================================================
>> +# Now consider partition starting sector numbers.
>> +msg='Error: starting sector number, 4294967296 exceeds the '\
>> +'DOS-partition-table-imposed maximum of 2^32-1'
>> +
>> +test_expect_success \
>> +    'a partition start sector number of 2^32-1 works.' \
>> +    'do_mkpart_start_and_len $(echo 2^32-1|bc) 1000'
>> +
>> +cat > exp <<EOF
>> +Model:  (file)
>> +Disk: 4294970342s
>> +Sector size (logical/physical): 512B/512B
>> +Partition Table: msdos
>> +
>> +Number  Start        End          Size   Type     File system  Flags
>> + 1      4294967295s  4294968294s  1000s  primary
>> +
>> +EOF
>> +
>> +test_expect_success \
>> +    'print the result' \
>> +    'parted -s $dev unit s p > out 2>&1 &&
>> +     sed "s/Disk .*:/Disk:/;s/ *$//" out > k && mv k out &&
>> +     diff -u out exp
>> +    '
>> +
>> +test_expect_failure \
>> +    'a partition start sector number of 2^32 must fail.' \
>> +    'do_mkpart_start_and_len $(echo 2^32|bc) 1000 > err 2>&1'
>> +test_expect_success \
>> +    'check for new diagnostic' \
>> +    'echo "$msg" > exp && diff -u err exp'
>> +
>> +test_expect_failure \
>> +    'a partition start sector number of 2^32+1 must fail, too.' \
>> +    'do_mkpart_start_and_len $(echo 2^32+1|bc) 1000 > err 2>&1'
>> +test_expect_success \
>> +    'check for new diagnostic' \
>> +    'echo "$msg" > exp && diff -u err exp'
>> +
>> +test_done
>> --
>> 1.5.4.rc2.85.g71fd
>>
>> _______________________________________________
>> parted-devel mailing list
>> address@hidden
>> http://lists.alioth.debian.org/mailman/listinfo/parted-devel
>>
>>
> 
> 
> _______________________________________________
> parted-devel mailing list
> address@hidden
> http://lists.alioth.debian.org/mailman/listinfo/parted-devel
> 
>