From: Mikulas Patocka <mpatocka@redhat.com>
Date: Wed, 2 Dec 2009 04:29:11 -0500
Subject: [md] fix snapshot crash on invalidation
Message-id: <Pine.LNX.4.64.0912012322000.19328@hs20-bc2-1.build.redhat.com>
Patchwork-id: 21640
O-Subject: [RHEL5.5 PATCH] bz461506: fix snapshot crash on invalidation
Bugzilla: 461506
RH-Acked-by: Jonathan E Brassow <jbrassow@redhat.com>

Hi

This patch fixes a problem in snapshots: the same memory area is used for
metadata chunk i/o and for header i/o. When the snapshot overflows and is
invalidated, a header is updated. A concurrent metadata chunk i/o may
corrupt the header at the same time and write "0" to the chunk size field.
When attempting to activate a snapshot with "0" chunk size, the computer
crashes.

The patch fixes both problems, it uses separate areas for the header and
for metadata. And it validates chunk size read from disk, so that
snapshots with invalid header are rejected and the computer doesn't crash.

Upstream status: committed in 2.6.31, this patch is a combination of 4
upstream patches with commit ids 02d2fd31defce6ff77146ad0fef4f19006055d86,
61578dcd3fafe6babd72e8db32110cc0b630a432,
2defcc3fb4661e7351cb2ac48d843efc4c64db13,
ae0b7448e91353ea5f821601a055aca6b58042cd

Testing: the race condition is unreproducible (it only happened few times
during RH testing). I manually wrote "0" as a chunk size to the header and
verified that the computer no longer crashes when attempting to activate
this snapshot. I also ran a simple snapshot test to make sure that the
patch doesn't break general functionality of snapshots.

Mikulas


diff --git a/drivers/md/dm-exception-store.c b/drivers/md/dm-exception-store.c
index 330f1e8..a16a20b 100644
--- a/drivers/md/dm-exception-store.c
+++ b/drivers/md/dm-exception-store.c
@@ -109,6 +109,13 @@ struct pstore {
 	void *zero_area;
 
 	/*
+	 * An area used for header. The header can be written
+	 * concurrently with metadata (when invalidating the snapshot),
+	 * so it needs a separate buffer.
+	 */
+	void *header_area;
+
+	/*
 	 * Used to keep track of which metadata area the data in
 	 * 'chunk' refers to.
 	 */
@@ -149,16 +156,25 @@ static int alloc_area(struct pstore *ps)
 	 */
 	ps->area = vmalloc(len);
 	if (!ps->area)
-		return r;
+		goto err_area;
 
 	ps->zero_area = vmalloc(len);
-	if (!ps->zero_area) {
-		vfree(ps->area);
-		return r;
-	}
+	if (!ps->zero_area)
+		goto err_zero_area;
 	memset(ps->zero_area, 0, len);
 
+	ps->header_area = vmalloc(len);
+	if (!ps->header_area)
+		goto err_header_area;
+
 	return 0;
+
+err_header_area:
+	vfree(ps->zero_area);
+err_zero_area:
+	vfree(ps->area);
+err_area:
+	return r;
 }
 
 static void free_area(struct pstore *ps)
@@ -167,12 +183,14 @@ static void free_area(struct pstore *ps)
 	ps->area = NULL;
 	vfree(ps->zero_area);
 	ps->zero_area = NULL;
+	vfree(ps->header_area);
+	ps->header_area = NULL;
 }
 
 /*
  * Read or write a chunk aligned and sized block of data from a device.
  */
-static int chunk_io(struct pstore *ps, uint32_t chunk, int rw)
+static int chunk_io(struct pstore *ps, void *area, chunk_t chunk, int rw)
 {
 	struct io_region where = {
 		.bdev = ps->snap->cow->bdev,
@@ -182,7 +200,7 @@ static int chunk_io(struct pstore *ps, uint32_t chunk, int rw)
 	struct dm_io_request io_req = {
 		.bi_rw = rw,
 		.mem.type = DM_IO_VMA,
-		.mem.ptr.vma = ps->area,
+		.mem.ptr.vma = area,
 		.client = ps->io_client,
 		.notify.fn = NULL,
 	};
@@ -209,7 +227,7 @@ static int area_io(struct pstore *ps, int rw)
 
 	chunk = area_location(ps, ps->current_area);
 
-	r = chunk_io(ps, chunk, rw);
+	r = chunk_io(ps, ps->area, chunk, rw);
 	if (r)
 		return r;
 
@@ -223,20 +241,7 @@ static void zero_memory_area(struct pstore *ps)
 
 static int zero_disk_area(struct pstore *ps, chunk_t area)
 {
-	struct io_region where = {
-		.bdev = ps->snap->cow->bdev,
-		.sector = ps->snap->chunk_size * area_location(ps, area),
-		.count = ps->snap->chunk_size,
-	};
-	struct dm_io_request io_req = {
-		.bi_rw = WRITE,
-		.mem.type = DM_IO_VMA,
-		.mem.ptr.vma = ps->zero_area,
-		.client = ps->io_client,
-		.notify.fn = NULL,
-	};
-
-	return dm_io(&io_req, 1, &where, NULL);
+	return chunk_io(ps, ps->zero_area, area_location(ps, area), WRITE);
 }
 
 static int read_header(struct pstore *ps, int *new_snapshot)
@@ -245,6 +250,7 @@ static int read_header(struct pstore *ps, int *new_snapshot)
 	struct disk_header *dh;
 	chunk_t chunk_size;
 	int chunk_size_supplied = 1;
+	char *chunk_err;
 
 	/*
 	 * Use default chunk size (or hardsect_size, if larger) if none supplied
@@ -266,11 +272,11 @@ static int read_header(struct pstore *ps, int *new_snapshot)
 	if (r)
 		return r;
 
-	r = chunk_io(ps, 0, READ);
+	r = chunk_io(ps, ps->header_area, 0, READ);
 	if (r)
 		goto bad;
 
-	dh = (struct disk_header *) ps->area;
+	dh = (struct disk_header *) ps->header_area;
 
 	if (le32_to_cpu(dh->magic) == 0) {
 		*new_snapshot = 1;
@@ -288,20 +294,25 @@ static int read_header(struct pstore *ps, int *new_snapshot)
 	ps->version = le32_to_cpu(dh->version);
 	chunk_size = le32_to_cpu(dh->chunk_size);
 
-	if (!chunk_size_supplied || ps->snap->chunk_size == chunk_size)
+	if (ps->snap->chunk_size == chunk_size)
 		return 0;
 
-	DMWARN("chunk size %llu in device metadata overrides "
-	       "table chunk size of %llu.",
-	       (unsigned long long)chunk_size,
-	       (unsigned long long)ps->snap->chunk_size);
+	if (chunk_size_supplied)
+		DMWARN("chunk size %llu in device metadata overrides "
+		       "table chunk size of %llu.",
+		       (unsigned long long)chunk_size,
+		       (unsigned long long)ps->snap->chunk_size);
 
 	/* We had a bogus chunk_size. Fix stuff up. */
 	free_area(ps);
 
-	ps->snap->chunk_size = chunk_size;
-	ps->snap->chunk_mask = chunk_size - 1;
-	ps->snap->chunk_shift = ffs(chunk_size) - 1;
+	r = dm_exception_store_set_chunk_size(ps->snap, chunk_size,
+					      &chunk_err);
+	if (r) {
+		DMERR("invalid on-disk chunk size %llu: %s.",
+		      (unsigned long long)chunk_size, chunk_err);
+		return r;
+	}
 
 	r = dm_io_client_resize(sectors_to_pages(ps->snap->chunk_size),
 				ps->io_client);
@@ -320,15 +331,15 @@ static int write_header(struct pstore *ps)
 {
 	struct disk_header *dh;
 
-	memset(ps->area, 0, ps->snap->chunk_size << SECTOR_SHIFT);
+	memset(ps->header_area, 0, ps->snap->chunk_size << SECTOR_SHIFT);
 
-	dh = (struct disk_header *) ps->area;
+	dh = (struct disk_header *) ps->header_area;
 	dh->magic = cpu_to_le32(SNAP_MAGIC);
 	dh->valid = cpu_to_le32(ps->valid);
 	dh->version = cpu_to_le32(ps->version);
 	dh->chunk_size = cpu_to_le32(ps->snap->chunk_size);
 
-	return chunk_io(ps, 0, WRITE);
+	return chunk_io(ps, ps->header_area, 0, WRITE);
 }
 
 /*
@@ -627,6 +638,8 @@ int dm_create_persistent(struct exception_store *store)
 	ps->valid = 1;
 	ps->version = SNAPSHOT_DISK_VERSION;
 	ps->area = NULL;
+	ps->zero_area = NULL;
+	ps->header_area = NULL;
 	ps->next_free = 2;	/* skipping the header and first area */
 	ps->current_committed = 0;
 
diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index 8fba1f6..1d240dd 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -535,9 +535,14 @@ static int set_chunk_size(struct dm_snapshot *s, const char *chunk_size_arg,
 	 * round up if it's not.
 	 */
 	chunk_size = round_up(chunk_size, PAGE_SIZE >> 9);
+	return dm_exception_store_set_chunk_size(s, chunk_size, error);
+}
 
+int dm_exception_store_set_chunk_size(struct dm_snapshot *s,
+				      unsigned long chunk_size, char **error)
+{
 	/* Check chunk_size is a power of 2 */
-	if (chunk_size & (chunk_size - 1)) {
+	if (!is_power_of_2(chunk_size)) {
 		*error = "Chunk size is not a power of 2";
 		return -EINVAL;
 	}
@@ -548,6 +553,11 @@ static int set_chunk_size(struct dm_snapshot *s, const char *chunk_size_arg,
 		return -EINVAL;
 	}
 
+	if (chunk_size > INT_MAX >> SECTOR_SHIFT) {
+		*error = "Chunk size is too high";
+		return -EINVAL;
+	}
+
 	s->chunk_size = chunk_size;
 	s->chunk_mask = chunk_size - 1;
 	s->chunk_shift = ffs(chunk_size) - 1;
diff --git a/drivers/md/dm-snap.h b/drivers/md/dm-snap.h
index 910f9e9..aec48ed 100644
--- a/drivers/md/dm-snap.h
+++ b/drivers/md/dm-snap.h
@@ -233,4 +233,7 @@ static inline int bdev_equal(struct block_device *lhs, struct block_device *rhs)
 	return lhs == rhs;
 }
 
+int dm_exception_store_set_chunk_size(struct dm_snapshot *s,
+				      unsigned long chunk_size, char **error);
+
 #endif