[PATCH 0/2] blkdev: Fix livelock when loop device updates capacity

[PATCH 0/2] blkdev: Fix livelock when loop device updates capacity

[Jan Kara]

Hello,

this series fixes a long standing issue with loop device which can change block
device size under a mounted filesystem which causes infinite loop inside buffer
head code. See patch 2/2 for details about the problem.

Note that generally it is dangerous to resize the loop device when filesystem
is mounted on top of it. However there are some valid use cases for this (such
as growing the loop device and then increasing the filesystem size) so we
cannot just restrict the functionality to exclusive owners of the device.

								Honza

[PATCH 1/2] nbd: Use set_blocksize() to set device blocksize

[Jan Kara]

NBD can update block device block size implicitely through
bd_set_size(). Make it explicitely set blocksize with set_blocksize() as
this behavior of bd_set_size() is going away.

CC: Josef Bacik <jbacik@fb.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 drivers/block/nbd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 08696f5f00bb..7c9a949e876b 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -288,9 +288,10 @@ static void nbd_size_update(struct nbd_device *nbd)
 	blk_queue_physical_block_size(nbd->disk->queue, config->blksize);
 	set_capacity(nbd->disk, config->bytesize >> 9);
 	if (bdev) {
-		if (bdev->bd_disk)
+		if (bdev->bd_disk) {
 			bd_set_size(bdev, config->bytesize);
-		else
+			set_blocksize(bdev, config->blksize);
+		} else
 			bdev->bd_invalidated = 1;
 		bdput(bdev);
 	}
-- 
2.16.4

[PATCH 2/2] blockdev: Fix livelocks on loop device

[Jan Kara]

bd_set_size() updates also block device's block size. This is somewhat
unexpected from its name and at this point, only blkdev_open() uses this
functionality. Furthermore, this can result in changing block size under
a filesystem mounted on a loop device which leads to livelocks inside
__getblk_gfp() like:

Sending NMI from CPU 0 to CPUs 1:
NMI backtrace for cpu 1
CPU: 1 PID: 10863 Comm: syz-executor0 Not tainted 4.18.0-rc5+ #151
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google
01/01/2011
RIP: 0010:__sanitizer_cov_trace_pc+0x3f/0x50 kernel/kcov.c:106
...
Call Trace:
 init_page_buffers+0x3e2/0x530 fs/buffer.c:904
 grow_dev_page fs/buffer.c:947 [inline]
 grow_buffers fs/buffer.c:1009 [inline]
 __getblk_slow fs/buffer.c:1036 [inline]
 __getblk_gfp+0x906/0xb10 fs/buffer.c:1313
 __bread_gfp+0x2d/0x310 fs/buffer.c:1347
 sb_bread include/linux/buffer_head.h:307 [inline]
 fat12_ent_bread+0x14e/0x3d0 fs/fat/fatent.c:75
 fat_ent_read_block fs/fat/fatent.c:441 [inline]
 fat_alloc_clusters+0x8ce/0x16e0 fs/fat/fatent.c:489
 fat_add_cluster+0x7a/0x150 fs/fat/inode.c:101
 __fat_get_block fs/fat/inode.c:148 [inline]
...

Trivial reproducer for the problem looks like:

truncate -s 1G /tmp/image
losetup /dev/loop0 /tmp/image
mkfs.ext4 -b 1024 /dev/loop0
mount -t ext4 /dev/loop0 /mnt
losetup -c /dev/loop0
l /mnt

Fix the problem by moving initialization of a block device block size
into a separate function and call it when needed.

Thanks to Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> for help with
debugging the problem.

Reported-by: syzbot+9933e4476f365f5d5a1b@syzkaller.appspotmail.com
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/block_dev.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index c546cdce77e6..58a4c1217fa8 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -104,6 +104,20 @@ void invalidate_bdev(struct block_device *bdev)
 }
 EXPORT_SYMBOL(invalidate_bdev);
 
+static void set_init_blocksize(struct block_device *bdev)
+{
+	unsigned bsize = bdev_logical_block_size(bdev);
+	loff_t size = i_size_read(bdev->bd_inode);
+
+	while (bsize < PAGE_SIZE) {
+		if (size & bsize)
+			break;
+		bsize <<= 1;
+	}
+	bdev->bd_block_size = bsize;
+	bdev->bd_inode->i_blkbits = blksize_bits(bsize);
+}
+
 int set_blocksize(struct block_device *bdev, int size)
 {
 	/* Size must be a power of two, and between 512 and PAGE_SIZE */
@@ -1431,18 +1445,9 @@ EXPORT_SYMBOL(check_disk_change);
 
 void bd_set_size(struct block_device *bdev, loff_t size)
 {
-	unsigned bsize = bdev_logical_block_size(bdev);
-
 	inode_lock(bdev->bd_inode);
 	i_size_write(bdev->bd_inode, size);
 	inode_unlock(bdev->bd_inode);
-	while (bsize < PAGE_SIZE) {
-		if (size & bsize)
-			break;
-		bsize <<= 1;
-	}
-	bdev->bd_block_size = bsize;
-	bdev->bd_inode->i_blkbits = blksize_bits(bsize);
 }
 EXPORT_SYMBOL(bd_set_size);
 
@@ -1519,8 +1524,10 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 				}
 			}
 
-			if (!ret)
+			if (!ret) {
 				bd_set_size(bdev,(loff_t)get_capacity(disk)<<9);
+				set_init_blocksize(bdev);
+			}
 
 			/*
 			 * If the device is invalidated, rescan partition
@@ -1555,6 +1562,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 				goto out_clear;
 			}
 			bd_set_size(bdev, (loff_t)bdev->bd_part->nr_sects << 9);
+			set_init_blocksize(bdev);
 		}
 
 		if (bdev->bd_bdi == &noop_backing_dev_info)
-- 
2.16.4

[PATCH] loop: Add test for changing capacity when filesystem is mounted

[Jan Kara]

Add test for changing capacity of a loop device when a filesystem with
non-default block size is mounted on it. This is a regression test for
"blockdev: Fix livelocks on loop device".

Signed-off-by: Jan Kara <jack@suse.cz>
---
 tests/loop/007     | 38 ++++++++++++++++++++++++++++++++++++++
 tests/loop/007.out |  2 ++
 2 files changed, 40 insertions(+)
 create mode 100755 tests/loop/007
 create mode 100644 tests/loop/007.out

diff --git a/tests/loop/007 b/tests/loop/007
new file mode 100755
index 000000000000..1e2bf5131c97
--- /dev/null
+++ b/tests/loop/007
@@ -0,0 +1,38 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2019 Jan Kara
+#
+# Test loop device capacity change handling with filesystem mounted on top.
+#
+# Regression test for patch "blockdev: Fix livelocks on loop device".
+#
+
+. tests/loop/rc
+
+DESCRIPTION="update loop device capacity with filesystem"
+
+QUICK=1
+
+requires() {
+	_have_program mkfs.ext4
+}
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	local mount_dir="/mnt/blktests/"
+
+	truncate -s 1G "$TMPDIR/img"
+	mkdir -p "$mount_dir"
+	local loop_device="$(losetup -P -f --show "$TMPDIR/img")"
+	mkfs.ext4 -b 1024 "$loop_device"
+	mount -t ext4 "$loop_device" /mnt
+	losetup -c "$loop_device"
+	# This hangs if rereading capacity changed block size
+	l /mnt
+	umount /mnt
+	losetup -d "$loop_device"
+	rm -fr "$mount_dir" "$TMPDIR/img"
+
+	echo "Test complete"
+}
diff --git a/tests/loop/007.out b/tests/loop/007.out
new file mode 100644
index 000000000000..32752934d48a
--- /dev/null
+++ b/tests/loop/007.out
@@ -0,0 +1,2 @@
+Running loop/007
+Test complete
-- 
2.16.4

Re: [PATCH] loop: Add test for changing capacity when filesystem is mounted

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

On Mon 14-01-19 11:41:19, Jan Kara wrote:
> On Mon 14-01-19 18:44:22, Tetsuo Handa wrote:
> > Thank you for the patch.
> > 
> > On 2019/01/14 17:48, Jan Kara wrote:
> > > +	# This hangs if rereading capacity changed block size
> > > +	l /mnt
> > 
> > Is 'l' defined as an alias to 'ls' ? I suggest not depending on such alias,
> > for at least 'l' is not defined in my environment.
> 
> Yeah, my bad. I have it everywhere and my finger memory just typed it into
> the test :). Of course 'ls -l' is better for the test so I'll fix it up in
> the next version. Thanks for review.

The test had a couple more bugs which should be fixed now so attached is a
new version which I've actually tested inside the blktests framework ;).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-loop-Add-test-for-changing-capacity-when-filesystem-.patch --]
[-- Type: text/x-patch, Size: 1904 bytes --]

From 45a003672775cb6dcebc99d7ddfe19a1c9507ac3 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Mon, 14 Jan 2019 09:23:42 +0100
Subject: [PATCH v2] loop: Add test for changing capacity when filesystem is
 mounted

Add test for changing capacity of a loop device when a filesystem with
non-default block size is mounted on it. This is a regression test for
"blockdev: Fix livelocks on loop device".

Signed-off-by: Jan Kara <jack@suse.cz>
---
 tests/loop/007     | 38 ++++++++++++++++++++++++++++++++++++++
 tests/loop/007.out |  2 ++
 2 files changed, 40 insertions(+)
 create mode 100755 tests/loop/007
 create mode 100644 tests/loop/007.out

diff --git a/tests/loop/007 b/tests/loop/007
new file mode 100755
index 000000000000..3598aeba4443
--- /dev/null
+++ b/tests/loop/007
@@ -0,0 +1,38 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2019 Jan Kara
+#
+# Test loop device capacity change handling with filesystem mounted on top.
+#
+# Regression test for patch "blockdev: Fix livelocks on loop device".
+#
+
+. tests/loop/rc
+
+DESCRIPTION="update loop device capacity with filesystem"
+
+QUICK=1
+
+requires() {
+	_have_program mkfs.ext4
+}
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	local mount_dir="/mnt/blktests/"
+
+	truncate -s 1G "$TMPDIR/img"
+	mkdir -p "$mount_dir"
+	local loop_device="$(losetup -P -f --show "$TMPDIR/img")"
+	mkfs.ext4 -b 1024 "$loop_device" &>/dev/null
+	mount -t ext4 "$loop_device" "$mount_dir"
+	losetup -c "$loop_device"
+	# This hangs if rereading capacity changed block size
+	ls -l "$mount_dir" >/dev/null
+	umount "$mount_dir"
+	losetup -d "$loop_device"
+	rm -fr "$mount_dir" "$TMPDIR/img"
+
+	echo "Test complete"
+}
diff --git a/tests/loop/007.out b/tests/loop/007.out
new file mode 100644
index 000000000000..32752934d48a
--- /dev/null
+++ b/tests/loop/007.out
@@ -0,0 +1,2 @@
+Running loop/007
+Test complete
-- 
2.16.4

Re: [PATCH] loop: Add test for changing capacity when filesystem is mounted

[Chaitanya Kulkarni]

Hi Jan,

Thanks for the test.

     1	From 45a003672775cb6dcebc99d7ddfe19a1c9507ac3 Mon Sep 17 00:00:00 2001
     2	From: Jan Kara <jack@suse.cz>
     3	Date: Mon, 14 Jan 2019 09:23:42 +0100
     4	Subject: [PATCH v2] loop: Add test for changing capacity when filesystem is
     5	 mounted
     6	
     7	Add test for changing capacity of a loop device when a filesystem with
     8	non-default block size is mounted on it. This is a regression test for
     9	"blockdev: Fix livelocks on loop device".
    10	
    11	Signed-off-by: Jan Kara <jack@suse.cz>
    12	---
    13	 tests/loop/007     | 38 ++++++++++++++++++++++++++++++++++++++
    14	 tests/loop/007.out |  2 ++
    15	 2 files changed, 40 insertions(+)
    16	 create mode 100755 tests/loop/007
    17	 create mode 100644 tests/loop/007.out
    18	
    19	diff --git a/tests/loop/007 b/tests/loop/007
    20	new file mode 100755
    21	index 000000000000..3598aeba4443
    22	--- /dev/null
    23	+++ b/tests/loop/007
    24	@@ -0,0 +1,38 @@
    25	+#!/bin/bash
    26	+# SPDX-License-Identifier: GPL-3.0+
    27	+# Copyright (C) 2019 Jan Kara
    28	+#
    29	+# Test loop device capacity change handling with filesystem mounted on top.
    30	+#
    31	+# Regression test for patch "blockdev: Fix livelocks on loop device".
    32	+#
    33	+
    34	+. tests/loop/rc
    35	+
    36	+DESCRIPTION="update loop device capacity with filesystem"
    37	+
    38	+QUICK=1
    39	+
    40	+requires() {
    41	+	_have_program mkfs.ext4
    42	+}
    43	+
    44	+test() {
    45	+	echo "Running ${TEST_NAME}"
    46	+
Can we add loop module load/unload at the start and end of the test case ? 
or is there any specific reason we are not loading and unloading the module in this test ?
    47	+	local mount_dir="/mnt/blktests/"
    48	+
    49	+	truncate -s 1G "$TMPDIR/img"
    50	+	mkdir -p "$mount_dir"
    51	+	local loop_device="$(losetup -P -f --show "$TMPDIR/img")"
    52	+	mkfs.ext4 -b 1024 "$loop_device" &>/dev/null
    53	+	mount -t ext4 "$loop_device" "$mount_dir"
    54	+	losetup -c "$loop_device"
    55	+	# This hangs if rereading capacity changed block size
    56	+	ls -l "$mount_dir" >/dev/null
    57	+	umount "$mount_dir"
    58	+	losetup -d "$loop_device"
    59	+	rm -fr "$mount_dir" "$TMPDIR/img"
    60	+
    61	+	echo "Test complete"
    62	+}
    63	diff --git a/tests/loop/007.out b/tests/loop/007.out
    64	new file mode 100644
    65	index 000000000000..32752934d48a
    66	--- /dev/null
    67	+++ b/tests/loop/007.out
    68	@@ -0,0 +1,2 @@
    69	+Running loop/007
    70	+Test complete
    71	-- 
    72	2.16.4
    73	


-ck




From: linux-block-owner@vger.kernel.org <linux-block-owner@vger.kernel.org> on behalf of Jan Kara <jack@suse.cz>
Sent: Monday, January 14, 2019 2:50 AM
To: Jens Axboe
Cc: Jan Kara; linux-block@vger.kernel.org; Josef Bacik; Tetsuo Handa
Subject: Re: [PATCH] loop: Add test for changing capacity when filesystem is mounted
  
 
On Mon 14-01-19 11:41:19, Jan Kara wrote:
> On Mon 14-01-19 18:44:22, Tetsuo Handa wrote:
> > Thank you for the patch.
> > 
> > On 2019/01/14 17:48, Jan Kara wrote:
> > > + # This hangs if rereading capacity changed block size
> > > + l /mnt
> > 
> > Is 'l' defined as an alias to 'ls' ? I suggest not depending on such alias,
> > for at least 'l' is not defined in my environment.
> 
> Yeah, my bad. I have it everywhere and my finger memory just typed it into
> the test :). Of course 'ls -l' is better for the test so I'll fix it up in
> the next version. Thanks for review.

The test had a couple more bugs which should be fixed now so attached is a
new version which I've actually tested inside the blktests framework ;).

                                                                Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR
    

Re: [PATCH] loop: Add test for changing capacity when filesystem is mounted

[Jan Kara]

On Mon 14-01-19 19:10:17, Chaitanya Kulkarni wrote:
> Hi Jan,
> 
> Thanks for the test.
> 
>      1	From 45a003672775cb6dcebc99d7ddfe19a1c9507ac3 Mon Sep 17 00:00:00 2001
>      2	From: Jan Kara <jack@suse.cz>
>      3	Date: Mon, 14 Jan 2019 09:23:42 +0100
>      4	Subject: [PATCH v2] loop: Add test for changing capacity when filesystem is
>      5	 mounted
>      6	
>      7	Add test for changing capacity of a loop device when a filesystem with
>      8	non-default block size is mounted on it. This is a regression test for
>      9	"blockdev: Fix livelocks on loop device".
>     10	
>     11	Signed-off-by: Jan Kara <jack@suse.cz>
>     12	---
>     13	 tests/loop/007     | 38 ++++++++++++++++++++++++++++++++++++++
>     14	 tests/loop/007.out |  2 ++
>     15	 2 files changed, 40 insertions(+)
>     16	 create mode 100755 tests/loop/007
>     17	 create mode 100644 tests/loop/007.out
>     18	
>     19	diff --git a/tests/loop/007 b/tests/loop/007
>     20	new file mode 100755
>     21	index 000000000000..3598aeba4443
>     22	--- /dev/null
>     23	+++ b/tests/loop/007
>     24	@@ -0,0 +1,38 @@
>     25	+#!/bin/bash
>     26	+# SPDX-License-Identifier: GPL-3.0+
>     27	+# Copyright (C) 2019 Jan Kara
>     28	+#
>     29	+# Test loop device capacity change handling with filesystem mounted on top.
>     30	+#
>     31	+# Regression test for patch "blockdev: Fix livelocks on loop device".
>     32	+#
>     33	+
>     34	+. tests/loop/rc
>     35	+
>     36	+DESCRIPTION="update loop device capacity with filesystem"
>     37	+
>     38	+QUICK=1
>     39	+
>     40	+requires() {
>     41	+	_have_program mkfs.ext4
>     42	+}
>     43	+
>     44	+test() {
>     45	+	echo "Running ${TEST_NAME}"
>     46	+
> Can we add loop module load/unload at the start and end of the test case
> ?  or is there any specific reason we are not loading and unloading the
> module in this test ?

Well, no other loop tests do this AFAICT. Normally, loop module gets
auto-loaded when first opening some loop device so manual loading usually
is not needed. Also unloading can easily fail if there are other users of
the loop module independent of the test suite.

So to sum it up, I can add loading / unloading to all loop tests but IMO
the benefit is doubtful.

								Honza

>     47	+	local mount_dir="/mnt/blktests/"
>     48	+
>     49	+	truncate -s 1G "$TMPDIR/img"
>     50	+	mkdir -p "$mount_dir"
>     51	+	local loop_device="$(losetup -P -f --show "$TMPDIR/img")"
>     52	+	mkfs.ext4 -b 1024 "$loop_device" &>/dev/null
>     53	+	mount -t ext4 "$loop_device" "$mount_dir"
>     54	+	losetup -c "$loop_device"
>     55	+	# This hangs if rereading capacity changed block size
>     56	+	ls -l "$mount_dir" >/dev/null
>     57	+	umount "$mount_dir"
>     58	+	losetup -d "$loop_device"
>     59	+	rm -fr "$mount_dir" "$TMPDIR/img"
>     60	+
>     61	+	echo "Test complete"
>     62	+}
>     63	diff --git a/tests/loop/007.out b/tests/loop/007.out
>     64	new file mode 100644
>     65	index 000000000000..32752934d48a
>     66	--- /dev/null
>     67	+++ b/tests/loop/007.out
>     68	@@ -0,0 +1,2 @@
>     69	+Running loop/007
>     70	+Test complete
>     71	-- 
>     72	2.16.4
>     73	
> 
> 
> -ck
> 
> 
> 
> 
> From: linux-block-owner@vger.kernel.org <linux-block-owner@vger.kernel.org> on behalf of Jan Kara <jack@suse.cz>
> Sent: Monday, January 14, 2019 2:50 AM
> To: Jens Axboe
> Cc: Jan Kara; linux-block@vger.kernel.org; Josef Bacik; Tetsuo Handa
> Subject: Re: [PATCH] loop: Add test for changing capacity when filesystem is mounted
>   
>  
> On Mon 14-01-19 11:41:19, Jan Kara wrote:
> > On Mon 14-01-19 18:44:22, Tetsuo Handa wrote:
> > > Thank you for the patch.
> > > 
> > > On 2019/01/14 17:48, Jan Kara wrote:
> > > > + # This hangs if rereading capacity changed block size
> > > > + l /mnt
> > > 
> > > Is 'l' defined as an alias to 'ls' ? I suggest not depending on such alias,
> > > for at least 'l' is not defined in my environment.
> > 
> > Yeah, my bad. I have it everywhere and my finger memory just typed it into
> > the test :). Of course 'ls -l' is better for the test so I'll fix it up in
> > the next version. Thanks for review.
> 
> The test had a couple more bugs which should be fixed now so attached is a
> new version which I've actually tested inside the blktests framework ;).
> 
>                                                                 Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>     
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/2] blkdev: Fix livelock when loop device updates capacity

[Jens Axboe]

On 1/14/19 1:48 AM, Jan Kara wrote:
> Hello,
> 
> this series fixes a long standing issue with loop device which can change block
> device size under a mounted filesystem which causes infinite loop inside buffer
> head code. See patch 2/2 for details about the problem.
> 
> Note that generally it is dangerous to resize the loop device when filesystem
> is mounted on top of it. However there are some valid use cases for this (such
> as growing the loop device and then increasing the filesystem size) so we
> cannot just restrict the functionality to exclusive owners of the device.

That's really a shame...

Anyway, applied for 5.0, thanks Jan.

-- 
Jens Axboe