Date:   Wed, 18 Jan 2023 00:05:11 -0500
From:   "Theodore Ts'o" <tytso@....edu>
To:     zhanchengbin <zhanchengbin1@...wei.com>
Cc:     linux-ext4@...r.kernel.org, liuzhiqiang26@...wei.com
Subject: Re: [PATCH v2] resize2fs: resize2fs disk hardlinks will be error

On Tue, Jan 04, 2022 at 10:23:52PM +0800, zhanchengbin wrote:
> Resize2fs disk hardlinks which mounting after the same name as tmpfs
>   filesystem it will be error. The items in /proc/mounts are traversed,
> when you get to tmpfs,file!=mnt->mnt_fsname, therefore, the
> stat(mnt->mnt_fsname, &st_buf) branch is used, however, the values of
>   file_rdev and st_buf.st_rdev are the same. As a result, the system
> mistakenly considers that disk is mounted to /root/tmp. As a result
> , resize2fs fails.

Apologies for the delay in getting to this patch.  The original
patch[1] was corrupted (looks like you used Mozilla Thunderbird as
your Mail User Agent, which line-wrapped the patch and thus confused
patchwork[2] as well making it impossible for b4 and git am to handle
the patch).  

[1] https://lore.kernel.org/all/9dcadf7a-b12a-c977-2de2-222e20f0cebe@huawei.com/
[2] http://patchwork.ozlabs.org/project/linux-ext4/patch/9dcadf7a-b12a-c977-2de2-222e20f0cebe@huawei.com/

I also rewrite the commit description to make it more clear:

    libext2fs: add extra checks to ext2fs_check_mount_point()
    
    A pseudo-filesystem, such as tmpfs, can have anything at all in its
    mnt_fsname entry.  Normally, it is just "tmpfs", like this:
    
    tmpfs /tmp tmpfs rw,relatime,inode64 0 0
    ^^^^^
    
    but in a pathological or malicious case, a system administrator can
    specify a block device as its mnt_fsname which is the same as some
    other block device.  For example:
    
    /dev/loop0 /tmp/test-tmpfs tmpfs rw,relatime,inode64 0 0
    ^^^^^^^^^^
    /dev/loop0 /tmp/test-mnt ext4 rw,relatime 0 0
    
    In this case, ext2fs_check_mount_point() may erroneously return
    that the mountpoint for the file system on /dev/loop0 is mounted on
    /tmp/test-tmpfs, instead of the correct /tmp/test-mnt.  This causes
    problems for resize2fs, since in order to do an online resize, it
    needs to open the directory where the file system is mounted, and
    trigger the online resize ioctl.  If it opens the incorrect directory,
    then resize2fs will fail.
    
    So we need to add some additional checking to make sure that
    directory's st_dev matches the block device's st_rdev field.
    
    An example shell script which reproduces the problem fixed by this
    commit is as follows:
    
       loop_file=/tmp/foo.img
       tmpfs_dir=/tmp/test-tmpfs
       mnt_dir=/tmp/test-mnt
    
       mkdir -p $tmpfs_dir $mnt_dir
       dd if=/dev/zero of=$loop_file bs=1k count=65536
       test_dev=$(losetup --show -f $loop_file)
       mke2fs -t ext4 -F -b 1024 $test_dev 32768
       mount -t tmpfs $test_dev $tmpfs_dir  # create the evil /proc/mounts entry
       mount -t ext4 $test_dev $mnt_dir
       ln -f ${test_dev} ${test_dev}-ln
       resize2fs ${test_dev}-ln
    
    [ Fixed up the corrupted patch and rewrote the commit description to
      be more clear -- tytso ]
    
    Signed-off-by: zhanchengbin <zhanchengbin1@...wei.com>
    Signed-off-by: Zhiqiang Liu <liuzhiqiang26@...wei.com>
    Signed-off-by: Theodore Ts'o <tytso@....edu>

As you can see, the best commit description describes *why* a
particular change is needed, and gives the background so the reader
can understand what problem is being fixed.  The one-line change makes
it clear that the change is to libext2fs's ismounted.c, and *not* to
resize2fs, although you were making this bug to fix resize2fs after a
system administrator did something non-standard and/or malicious.

Also note how I rewrote the reproducer to be simpler and more
portable.

Cheers,

						- Ted

Powered by blists - more mailing lists