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

[Michael Reed]

Applying the patch to parted 1.8.8 (not a perfect fit, bit close) appears
to correct the problem.

duck /tmp/parted-1.8.8# patch -p1  < /tmp/parted.msdos.patch.eml
patching file libparted/disk.c
Hunk #2 succeeded at 1734 (offset -1 lines).
patching file tests/Makefile.am
Hunk #1 succeeded at 6 with fuzz 2 (offset -1 lines).
patching file tests/t4100-msdos-partition-limits.sh

Testing shows that I sometimes get this instead of the exception:

(parted) mkpart p ext2 2s 4294967297s
mkpart p ext2 2s 4294967297s
Warning: You requested a partition from 2s to 4294967297s.
The closest location we can manage is 63s to 4294961684s.  Is this still
acceptable to you?
Yes/No? no
no
(parted) 

If the range of the error is greater, I'll get the exception:

(parted) mkpart p ext2 33GB 2700GB
mkpart p ext2 33GB 2700GB
Error: partition length of 5208979860 sectors exceeds the
DOS-partition-table-imposed maximum of 2^32-1

No matter, as either case keeps me from creating an invalid label.

I also noticed that this problem also exists on the SGI label type, dvh.

Perhaps an examination of the various supported partition types is in order
to impose this limitation on more than just msdos labels?  A quick glance
(from linux/fs/partitions/*c) I see the following partition types which may
have a problem.  I didn't check them all....

        sgi (confirmed to have a problem)
        sun
        mac


I tested this if clause against dvh labels and it appears to not break anything.

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

Change the exception messages and comment to not be DOS centric and you're
good to go.

Thanks again for taking the time to do this work.

And yes, SGI is interested in having the dvh fix...

Mike


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.
> 
> 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.  */
> +             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