Fix SMP races in dm-crypt

PATCH In review Olaf Kirch 5 participants 10 messages Mar 21, 2007 – Mar 22, 2007 Device mapper

Share & tools

Olaf Kirch

author

2007-03-21 07:06 UTC #0 lore raw

cover Fix SMP races in dm-crypt

(I'm resending this, because it seems mailman ate this posting the first time around - apologies if you're seeing this twice).

I investigated kernel Bug 5948 (Oops when accessing SATA with device mapper on AMD 64 X2) to help cut down on the number of open bugs.

The problem reported in that bug seems to be caused by memory pressure when writing - crypt_alloc_buffer can return less than the full number of pages required. In that case, the bio is split, but we're keeping a pointer to the first chunk of the request around in first_clone, and copy it. If the first clone completes I/O while we're cloning it, we oops on garbage in the bvec.

I was able to reproduce this on 2.6.20 - and the following patches seem to fix it for me.

dm-crypt-clone_init-early
We need to call clone_init early on, before we can conceivably ever call bio_put on the clone

dm-crypt-clone-race
Do no access bios after generic_make_request

dm-crypt-drop-first_clone
Drop the first_clone business, and always allocate a fresh bio.

Olaf

Signature · 2 lines

Olaf Kirch  |  --- o --- Nous sommes du soleil we love when we play
[email protected] |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax

Alasdair G Kergon

maintainer

2007-03-21 10:03 UTC #1 lore raw

Quoted 2 lines · Olaf Kirch

(I'm resending this, because it seems mailman ate this posting the first time around - apologies if you're seeing this twice).

The first emails got trapped for moderation.

The 3rd patch was hit by line-wrap and needed fixing up.

Quoted 2 lines

I investigated kernel Bug 5948 (Oops when accessing SATA with device mapper on AMD 64 X2) to help cut down on the number of open bugs.

Thanks for your detective work on this! The patches look good and I've added them to
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/
(with prefix dm-crypt-fix) and will review them in full in the next couple of days.

Alasdair

Signature · 1 line

[email protected]

Olaf Kirch

author

2007-03-21 10:47 UTC #2 lore raw

[PATCH 4/3] Allocate smaller clones

Hi Alasdair,

FWIW, here's a fourth patch that tidies up crypt_allocate_buffer a little. It's not strictly required to fix the bug, but the code looks simpler this way, I think.

Olaf
------------------------
Allocate smaller clones

With the previous dm-crypt fixes, there is no need for the clone bios to have the same bvec size as the original - we just need to make them big enough for the remaining number of pages. The only requirement is that we clear the "out" index in convert_context, so that crypt_convert starts storing data at the right position within the clone bio.

Signed-off-by [email protected]

---
 drivers/md/dm-crypt.c |   28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

drivers/md/dm-crypt.c +8 ~~-20~~

Index: linux-2.6.20/drivers/md/dm-crypt.c
===================================================================
@@ -380,7 +380,7 @@ static int crypt_convert(struct crypt_co
  * May return a smaller bio when running out of pages
  */
 static struct bio *
-crypt_alloc_buffer(struct crypt_io *io, unsigned int size, unsigned int *bio_vec_idx)
+crypt_alloc_buffer(struct crypt_io *io, unsigned int size)
 {
 	struct crypt_config *cc = io->target->private;
 	struct bio *clone;

@@ -394,16 +394,7 @@ crypt_alloc_buffer(struct crypt_io *io, 
 
 	clone_init(io, clone);
 
-	/* if the last bio was not complete, continue where that one ended */
-	clone->bi_idx = *bio_vec_idx;
-	clone->bi_vcnt = *bio_vec_idx;
-	clone->bi_size = 0;
-	clone->bi_flags &= ~(1 << BIO_SEG_VALID);
-
-	/* clone->bi_idx pages have already been allocated */
-	size -= clone->bi_idx * PAGE_SIZE;
-
-	for (i = clone->bi_idx; i < nr_iovecs; i++) {
+	for (i = 0; i < nr_iovecs; i++) {
 		struct bio_vec *bv = bio_iovec_idx(clone, i);
 
 		bv->bv_page = mempool_alloc(cc->page_pool, gfp_mask);

@@ -415,7 +406,7 @@ crypt_alloc_buffer(struct crypt_io *io, 
 		 * return a partially allocated bio, the caller will then try
 		 * to allocate additional bios while submitting this partial bio
 		 */
-		if ((i - clone->bi_idx) == (MIN_BIO_PAGES - 1))
+		if (i == (MIN_BIO_PAGES - 1))
 			gfp_mask = (gfp_mask | __GFP_NOWARN) & ~__GFP_WAIT;
 
 		bv->bv_offset = 0;

@@ -434,12 +425,6 @@ crypt_alloc_buffer(struct crypt_io *io, 
 		return NULL;
 	}
 
-	/*
-	 * Remember the last bio_vec allocated to be able
-	 * to correctly continue after the splitting.
-	 */
-	*bio_vec_idx = clone->bi_vcnt;
-
 	return clone;
 }

@@ -597,7 +582,6 @@ static void process_write(struct crypt_i
 	struct convert_context ctx;
 	unsigned remaining = base_bio->bi_size;
 	sector_t sector = base_bio->bi_sector - io->target->begin;
-	unsigned bvec_idx = 0;
 
 	atomic_inc(&io->pending);

@@ -608,13 +592,14 @@ static void process_write(struct crypt_i
 	 * so repeat the whole process until all the data can be handled.
 	 */
 	while (remaining) {
-		clone = crypt_alloc_buffer(io, base_bio->bi_size, &bvec_idx);
+		clone = crypt_alloc_buffer(io, remaining);
 		if (unlikely(!clone)) {
 			dec_pending(io, -ENOMEM);
 			return;
 		}
 
 		ctx.bio_out = clone;
+		ctx.idx_out = 0;
 
 		if (unlikely(crypt_convert(cc, &ctx) < 0)) {
 			crypt_free_buffer_pages(cc, clone, clone->bi_size);

@@ -623,6 +608,9 @@ static void process_write(struct crypt_i
 			return;
 		}
 
+		/* crypt_convert should have filled the clone bio */
+		BUG_ON(ctx.idx_out < clone->bi_vcnt);
+
 		clone->bi_sector = cc->start + sector;
 		remaining -= clone->bi_size;
 		sector += bio_sectors(clone);

Signature · 2 lines

Olaf Kirch  |  --- o --- Nous sommes du soleil we love when we play
[email protected] |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax

Jim Meyering

2007-03-21 13:32 UTC #3 lore raw

In reply to Alasdair G Kergon

Quoted 2 lines · Alasdair G Kergon

Quoted 1 line · Olaf Kirch
(I'm resending this, because it seems mailman ate this posting

On a similar note, I sent two patches on Mar 12.
While they did reach the archive, perhaps you haven't seen them?

    https://www.redhat.com/archives/dm-devel/2007-March/msg00010.html
    https://www.redhat.com/archives/dm-devel/2007-March/msg00011.html

2007-03-12 Jim Meyering <[email protected]>

* lib/fs/libdevmapper.c (do_suspend): Detect write failure reliably. (do_load): Likewise.
(find_mount_point): Avoid buffer overrun for unusually long inputs.

2007-03-11 Jim Meyering <[email protected]>

* dmsetup/dmsetup.c (_loop_table): Detect close failure.

Alasdair G Kergon

maintainer

2007-03-21 15:44 UTC #4 lore raw

Quoted 5 lines · Jim Meyering

2007-03-12 Jim Meyering <[email protected]>
* lib/fs/libdevmapper.c (do_suspend): Detect write failure reliably. (do_load): Likewise.
(find_mount_point): Avoid buffer overrun for unusually long inputs.

That userspace code is neither used nor maintained any more. It's probably time to drop it from CVS so people don't waste time looking at it:-)

Quoted 3 lines

2007-03-11 Jim Meyering <[email protected]>
* dmsetup/dmsetup.c (_loop_table): Detect close failure.

I'll apply that, thanks, but I'm in no hurry because that section of code is unfinished and only partially reviewed.

Alasdair

Signature · 1 line

[email protected]

Christophe Saout

Acked-by on the v1 series

2007-03-22 20:28 UTC #5 lore raw

In reply to Olaf Kirch

Hi Olaf,

Quoted 20 lines

(I'm resending this, because it seems mailman ate this posting the first time around - apologies if you're seeing this twice).
I investigated kernel Bug 5948 (Oops when accessing SATA with device mapper on AMD 64 X2) to help cut down on the number of open bugs.
The problem reported in that bug seems to be caused by memory pressure when writing - crypt_alloc_buffer can return less than the full number of pages required. In that case, the bio is split, but we're keeping a pointer to the first chunk of the request around in first_clone, and copy it. If the first clone completes I/O while we're cloning it, we oops on garbage in the bvec.
I was able to reproduce this on 2.6.20 - and the following patches seem to fix it for me.
dm-crypt-clone_init-early
We need to call clone_init early on, before we can conceivably ever call bio_put on the clone

Quoted 5 lines

dm-crypt-clone-race
Do no access bios after generic_make_request
dm-crypt-drop-first_clone
Drop the first_clone business, and always allocate a fresh bio.

I fully agree. I didn't notice when the sharing of the bvec went away, but it's no longer needed and the code is simpler. As for the other two
- ouch, you're right.

Thanks for investigating fixing these things.

All four patches are

Acked-by Christophe Saout <[email protected]>

Andrew Morton

2007-03-22 20:41 UTC #6 lore raw

Quoted 1 line · On Thu, 22 Mar 2007 21:28:16 +0100 Christophe Saout <[email protected]> wrote:

Hi Olaf,

err, which Olaf are you addressing? He is not cc'ed.

Quoted 30 lines

Quoted 20 lines
(I'm resending this, because it seems mailman ate this posting the first time around - apologies if you're seeing this twice).
I investigated kernel Bug 5948 (Oops when accessing SATA with device mapper on AMD 64 X2) to help cut down on the number of open bugs.
The problem reported in that bug seems to be caused by memory pressure when writing - crypt_alloc_buffer can return less than the full number of pages required. In that case, the bio is split, but we're keeping a pointer to the first chunk of the request around in first_clone, and copy it. If the first clone completes I/O while we're cloning it, we oops on garbage in the bvec.
I was able to reproduce this on 2.6.20 - and the following patches seem to fix it for me.
dm-crypt-clone_init-early
We need to call clone_init early on, before we can conceivably ever call bio_put on the clone
Quoted 5 lines
dm-crypt-clone-race
Do no access bios after generic_make_request
dm-crypt-drop-first_clone
Drop the first_clone business, and always allocate a fresh bio.
I fully agree. I didn't notice when the sharing of the bvec went away, but it's no longer needed and the code is simpler. As for the other two
- ouch, you're right.

Quoted 5 lines

Thanks for investigating fixing these things.
All four patches are
Acked-by Christophe Saout <[email protected]>

I'm hunting around, but cannot find the patches to which you refer. Maybe they're on dm-devel only?

Christophe Saout

2007-03-22 20:51 UTC #7 lore raw

Am Donnerstag, den 22.03.2007, 13:41 -0700 schrieb Andrew Morton:

Quoted 3 lines

Quoted 1 line
Hi Olaf,
err, which Olaf are you addressing? He is not cc'ed.

Stupid client... didn't notice. Thanks .

Quoted 30 lines

Quoted 30 lines
Quoted 20 lines
(I'm resending this, because it seems mailman ate this posting the first time around - apologies if you're seeing this twice).
I investigated kernel Bug 5948 (Oops when accessing SATA with device mapper on AMD 64 X2) to help cut down on the number of open bugs.
The problem reported in that bug seems to be caused by memory pressure when writing - crypt_alloc_buffer can return less than the full number of pages required. In that case, the bio is split, but we're keeping a pointer to the first chunk of the request around in first_clone, and copy it. If the first clone completes I/O while we're cloning it, we oops on garbage in the bvec.
I was able to reproduce this on 2.6.20 - and the following patches seem to fix it for me.
dm-crypt-clone_init-early
We need to call clone_init early on, before we can conceivably ever call bio_put on the clone
Quoted 5 lines
dm-crypt-clone-race
Do no access bios after generic_make_request
dm-crypt-drop-first_clone
Drop the first_clone business, and always allocate a fresh bio.
I fully agree. I didn't notice when the sharing of the bvec went away, but it's no longer needed and the code is simpler. As for the other two
- ouch, you're right.

Quoted 8 lines

Quoted 5 lines
Thanks for investigating fixing these things.
All four patches are
Acked-by Christophe Saout <[email protected]>
I'm hunting around, but cannot find the patches to which you refer. Maybe they're on dm-devel only?

Yes, but Olaf also Cc'ed you. Didn't you get them?

Alasdair G Kergon

maintainer

2007-03-22 21:53 UTC #8 lore raw

In reply to Andrew Morton

Quoted 4 lines · Andrew Morton

On Thu, 22 Mar 2007 21:28:16 +0100
Christophe Saout <[email protected]> wrote:
I'm hunting around, but cannot find the patches to which you refer. Maybe they're on dm-devel only?

These four:
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-crypt-fix-call-to-clone_init.patch
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-crypt-fix-avoid-cloned-bio-ref-after-free.patch
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-crypt-fix-remove-first_clone.patch
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-crypt-use-smaller-bvecs-in-clones.patch

Grab them from there, or I'll send you them properly at the start of next week as part of the next dm patch set.

Alasdair

Signature · 1 line

[email protected]

Andrew Morton

2007-03-22 22:44 UTC #9 lore raw

Quoted 6 lines · On Thu, 22 Mar 2007 21:53:53 +0000 Alasdair G Kergon <[email protected]> wrote:

Quoted 4 lines · Andrew Morton
On Thu, 22 Mar 2007 21:28:16 +0100
Christophe Saout <[email protected]> wrote:
I'm hunting around, but cannot find the patches to which you refer. Maybe they're on dm-devel only?

Quoted 9 lines

These four:
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-crypt-fix-call-to-clone_init.patch
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-crypt-fix-avoid-cloned-bio-ref-after-free.patch
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-crypt-fix-remove-first_clone.patch
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-crypt-use-smaller-bvecs-in-clones.patch
Grab them from there, or I'll send you them properly at the start of next week as part of the next dm patch set.

yup, thanks - Christophe forwarded them.

(And I merged them with the wrong From: attribution (fixes that))

btw, should I include
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/* in the -mm lineup? It's easy to do from this end.