[PATCH] dm-log-writes: fix bug with too large bios

[PATCH] dm-log-writes: fix bug with too large bios

[Mikulas Patocka]

bio_alloc can allocate a bio with at most BIO_MAX_PAGES (256) vector
entries. However, the incoming bio may have more vector entries if it was
allocated by other means. For example, bcache submits bios with more than
BIO_MAX_PAGES entries. This results in bio_alloc failure.

To avoid the failure, change the code so that it allocates bio with at
most BIO_MAX_PAGES entries. If the incoming bio has more entries,
bio_add_page will fail and a new bio will be allocated - the code that
handles bio_add_page failure already exists in the dm-log-writes target.

Also, move atomic_inc(&lc->io_blocks) before bio_alloc to fix a bug that
the target hangs if bio_alloc fails. The error path does put_io_block(lc),
so we must do atomic_inc(&lc->io_blocks) before invoking the error path to
avoid underflow of lc->io_blocks.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org	# v4.1+

Index: linux-4.6/drivers/md/dm-log-writes.c
===================================================================
--- linux-4.6.orig/drivers/md/dm-log-writes.c
+++ linux-4.6/drivers/md/dm-log-writes.c
@@ -258,12 +258,12 @@ static int log_one_block(struct log_writ
 		goto out;
 	sector++;
 
-	bio = bio_alloc(GFP_KERNEL, block->vec_cnt);
+	atomic_inc(&lc->io_blocks);
+	bio = bio_alloc(GFP_KERNEL, min(block->vec_cnt, BIO_MAX_PAGES));
 	if (!bio) {
 		DMERR("Couldn't alloc log bio");
 		goto error;
 	}
-	atomic_inc(&lc->io_blocks);
 	bio->bi_iter.bi_size = 0;
 	bio->bi_iter.bi_sector = sector;
 	bio->bi_bdev = lc->logdev->bdev;
@@ -280,7 +280,7 @@ static int log_one_block(struct log_writ
 		if (ret != block->vecs[i].bv_len) {
 			atomic_inc(&lc->io_blocks);
 			submit_bio(WRITE, bio);
-			bio = bio_alloc(GFP_KERNEL, block->vec_cnt - i);
+			bio = bio_alloc(GFP_KERNEL, min(block->vec_cnt - i, BIO_MAX_PAGES));
 			if (!bio) {
 				DMERR("Couldn't alloc log bio");
 				goto error;

Re: [PATCH] dm-log-writes: fix bug with too large bios

[Josef Bacik]

On 05/27/2016 10:51 AM, Mikulas Patocka wrote:
> bio_alloc can allocate a bio with at most BIO_MAX_PAGES (256) vector
> entries. However, the incoming bio may have more vector entries if it was
> allocated by other means. For example, bcache submits bios with more than
> BIO_MAX_PAGES entries. This results in bio_alloc failure.
>
> To avoid the failure, change the code so that it allocates bio with at
> most BIO_MAX_PAGES entries. If the incoming bio has more entries,
> bio_add_page will fail and a new bio will be allocated - the code that
> handles bio_add_page failure already exists in the dm-log-writes target.
>
> Also, move atomic_inc(&lc->io_blocks) before bio_alloc to fix a bug that
> the target hangs if bio_alloc fails. The error path does put_io_block(lc),
> so we must do atomic_inc(&lc->io_blocks) before invoking the error path to
> avoid underflow of lc->io_blocks.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org	# v4.1+

Reviewed-by: Josef Bacik <jbacik@fb.com>

Thanks,

Josef

RE: [PATCH] dm-log-writes: fix bug with too large bios

[James Johnston]

Hi Mikulas,

> bio_alloc can allocate a bio with at most BIO_MAX_PAGES (256) vector
> entries. However, the incoming bio may have more vector entries if it was
> allocated by other means. For example, bcache submits bios with more than
> BIO_MAX_PAGES entries. This results in bio_alloc failure.
> 
> To avoid the failure, change the code so that it allocates bio with at
> most BIO_MAX_PAGES entries. If the incoming bio has more entries,
> bio_add_page will fail and a new bio will be allocated - the code that
> handles bio_add_page failure already exists in the dm-log-writes target.
> 
> Also, move atomic_inc(&lc->io_blocks) before bio_alloc to fix a bug that
> the target hangs if bio_alloc fails. The error path does put_io_block(lc),
> so we must do atomic_inc(&lc->io_blocks) before invoking the error path to
> avoid underflow of lc->io_blocks.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org	# v4.1+

How does this relate to the previous patch you made to dm-crypt?  How best
should I test this?  It looks like the dm-crypt patch fixed the problem.

Should I test by applying this patch ONLY and reverting the dm-crypt patch?
(i.e. does this patch also fix the problem.)  Or should I just test with
both patches applied simultaneously?

James

Re: dm-log-writes: fix bug with too large bios

[Mike Snitzer]

On Wed, Jun 01 2016 at 12:23am -0400,
James Johnston <johnstonj.public@codenest.com> wrote:

> Hi Mikulas,
> 
> > bio_alloc can allocate a bio with at most BIO_MAX_PAGES (256) vector
> > entries. However, the incoming bio may have more vector entries if it was
> > allocated by other means. For example, bcache submits bios with more than
> > BIO_MAX_PAGES entries. This results in bio_alloc failure.
> > 
> > To avoid the failure, change the code so that it allocates bio with at
> > most BIO_MAX_PAGES entries. If the incoming bio has more entries,
> > bio_add_page will fail and a new bio will be allocated - the code that
> > handles bio_add_page failure already exists in the dm-log-writes target.
> > 
> > Also, move atomic_inc(&lc->io_blocks) before bio_alloc to fix a bug that
> > the target hangs if bio_alloc fails. The error path does put_io_block(lc),
> > so we must do atomic_inc(&lc->io_blocks) before invoking the error path to
> > avoid underflow of lc->io_blocks.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Cc: stable@vger.kernel.org	# v4.1+
> 
> How does this relate to the previous patch you made to dm-crypt?  How best
> should I test this?  It looks like the dm-crypt patch fixed the problem.
> 
> Should I test by applying this patch ONLY and reverting the dm-crypt patch?
> (i.e. does this patch also fix the problem.)  Or should I just test with
> both patches applied simultaneously?

The dm-log-writes patch has nothing to do with the dm-crypt patch.  It
is just that both targets have comparable issues with bcache issuing
really large bios.

RE: [PATCH] dm-log-writes: fix bug with too large bios

[Mikulas Patocka]

On Wed, 1 Jun 2016, James Johnston wrote:

> Hi Mikulas,
> 
> > bio_alloc can allocate a bio with at most BIO_MAX_PAGES (256) vector
> > entries. However, the incoming bio may have more vector entries if it was
> > allocated by other means. For example, bcache submits bios with more than
> > BIO_MAX_PAGES entries. This results in bio_alloc failure.
> > 
> > To avoid the failure, change the code so that it allocates bio with at
> > most BIO_MAX_PAGES entries. If the incoming bio has more entries,
> > bio_add_page will fail and a new bio will be allocated - the code that
> > handles bio_add_page failure already exists in the dm-log-writes target.
> > 
> > Also, move atomic_inc(&lc->io_blocks) before bio_alloc to fix a bug that
> > the target hangs if bio_alloc fails. The error path does put_io_block(lc),
> > so we must do atomic_inc(&lc->io_blocks) before invoking the error path to
> > avoid underflow of lc->io_blocks.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Cc: stable@vger.kernel.org	# v4.1+
> 
> How does this relate to the previous patch you made to dm-crypt?  How best
> should I test this?  It looks like the dm-crypt patch fixed the problem.
> 
> Should I test by applying this patch ONLY and reverting the dm-crypt patch?
> (i.e. does this patch also fix the problem.)  Or should I just test with
> both patches applied simultaneously?
> 
> James

When I found a bug in dm-crypt, I searched the other targets for the same 
problem and found out that the same bug is also present in dm-log-writes.

The bug in dm-log-writes can be reproduced if you make a bcache device 
with 2MiB bucket size and place in on the dm-log-writes target.

When the bug is present, the dm-log-writes target reports "device-mapper: 
log-writes: Couldn't alloc log bio".

This is a script that triggers the bug for dm-log-writes. Before running 
the script, create devices /dev/mapper/loop-test1, loop-test2 and 
loop-test3.

Mikulas


wipefs -a /dev/mapper/loop-test1
./make-bcache --wipe-bcache --bucket 2M -C /dev/mapper/loop-test1
dmsetup create backCrypt --table "0 `blockdev --getsize /dev/mapper/loop-test2` log-writes /dev/mapper/loop-test2 /dev/mapper/loop-test3"
wipefs -a /dev/mapper/backCrypt
./make-bcache --wipe-bcache -B /dev/mapper/backCrypt
modprobe bcache
echo /dev/mapper/loop-test1 > /sys/fs/bcache/register
echo /dev/mapper/backCrypt > /sys/fs/bcache/register
./bcache-super-show /dev/mapper/loop-test1 | grep cset.uuid | cut -f 3 >/sys/block/bcache0/bcache/attach
echo writeback > /sys/block/bcache0/bcache/cache_mode

cd /sys/block/bcache0/bcache
echo 0 > sequential_cutoff
# Verify that the cache is attached (i.e. does not say "no cache")
cat state
dd if=/dev/urandom of=/dev/bcache0 bs=1M count=250
cat dirty_data
cat state
echo 1 > detach
cat dirty_data
cat state