[PATCH 0/4] Fix corruption of snapshot store on invalidation

[PATCH 0/4] Fix corruption of snapshot store on invalidation

[Mikulas Patocka]

Hi

These four patches fix snapshot store corruption on snapshot invalidation.
(bug https://bugzilla.redhat.com/show_bug.cgi?id=461506)

The first two patches fix a race causing corruption of snapshot store 
header.

The second two patches fix a crash when the snapshot is activated with 
corrupted store. With the patches applied, error is reported instead of a 
crash and the volume is not activated.

Mikulas

[PATCH 1/4] Refactor chunk_io

[Mikulas Patocka]

Refactor chunk_io.

Pass an area pointer to chunk_io and simplify zero_disk_area to use chunk_io.
This patch is needed to simplify the following patch, otherwise excessive
code duplication would happen. This patch shouldn't change any functionality.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-snap-persistent.c |   25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

Index: linux-2.6.31-rc5-devel/drivers/md/dm-snap-persistent.c
===================================================================
--- linux-2.6.31-rc5-devel.orig/drivers/md/dm-snap-persistent.c	2009-08-03 17:32:06.000000000 +0200
+++ linux-2.6.31-rc5-devel/drivers/md/dm-snap-persistent.c	2009-08-03 17:40:29.000000000 +0200
@@ -188,7 +188,7 @@ static void do_metadata(struct work_stru
 /*
  * Read or write a chunk aligned and sized block of data from a device.
  */
-static int chunk_io(struct pstore *ps, chunk_t chunk, int rw, int metadata)
+static int chunk_io(struct pstore *ps, void *area, chunk_t chunk, int rw, int metadata)
 {
 	struct dm_io_region where = {
 		.bdev = ps->store->cow->bdev,
@@ -198,7 +198,7 @@ static int chunk_io(struct pstore *ps, c
 	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,
 	};
@@ -240,7 +240,7 @@ static int area_io(struct pstore *ps, in
 
 	chunk = area_location(ps, ps->current_area);
 
-	r = chunk_io(ps, chunk, rw, 0);
+	r = chunk_io(ps, ps->area, chunk, rw, 0);
 	if (r)
 		return r;
 
@@ -254,20 +254,7 @@ static void zero_memory_area(struct psto
 
 static int zero_disk_area(struct pstore *ps, chunk_t area)
 {
-	struct dm_io_region where = {
-		.bdev = ps->store->cow->bdev,
-		.sector = ps->store->chunk_size * area_location(ps, area),
-		.count = ps->store->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, 0);
 }
 
 static int read_header(struct pstore *ps, int *new_snapshot)
@@ -297,7 +284,7 @@ static int read_header(struct pstore *ps
 	if (r)
 		return r;
 
-	r = chunk_io(ps, 0, READ, 1);
+	r = chunk_io(ps, ps->area, 0, READ, 1);
 	if (r)
 		goto bad;
 
@@ -359,7 +346,7 @@ static int write_header(struct pstore *p
 	dh->version = cpu_to_le32(ps->version);
 	dh->chunk_size = cpu_to_le32(ps->store->chunk_size);
 
-	return chunk_io(ps, 0, WRITE, 1);
+	return chunk_io(ps, ps->area, 0, WRITE, 1);
 }
 
 /*

[PATCH 2/4] Use a separate memory area for the header.

[Mikulas Patocka]

Use a separate memory area for the header.

This patch fixes the racy corruption of on-disk header.
When the snapshot overflows, __invalidate_snapshot is called, which calls
snapshot store method drop_snapshot. It goes to persistent_drop_snapshot that
calls write_header. write_header constructs the new header in the "area"
location.

Concurrently with that, existing kcopyd job may finish, call copy_callback
and commit_exception method, that goes to persistent_commit_exception.
persistent_commit_exception doesn't do locking, relying on the fact that
callbacks are single-threaded, but it can race with snapshot invalidation and
overwrite the header that is just being written while the snapshot is being
invalidated.

The result of this race is a corrupted header being written that can lead to
a crash on further reactivation (if chunk_size is zero in the corrupted header).

See the bug: https://bugzilla.redhat.com/show_bug.cgi?id=461506

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-snap-persistent.c |   42 ++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

Index: linux-2.6.31-rc5-devel/drivers/md/dm-snap-persistent.c
===================================================================
--- linux-2.6.31-rc5-devel.orig/drivers/md/dm-snap-persistent.c	2009-08-03 17:40:29.000000000 +0200
+++ linux-2.6.31-rc5-devel/drivers/md/dm-snap-persistent.c	2009-08-03 17:43:28.000000000 +0200
@@ -106,6 +106,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.
 	 */
@@ -148,16 +155,25 @@ static int alloc_area(struct pstore *ps)
 	 */
 	ps->area = vmalloc(len);
 	if (!ps->area)
-		return r;
+		goto err0;
 
 	ps->zero_area = vmalloc(len);
-	if (!ps->zero_area) {
-		vfree(ps->area);
-		return r;
-	}
+	if (!ps->zero_area)
+		goto err1;
 	memset(ps->zero_area, 0, len);
 
+	ps->header_area = vmalloc(len);
+	if (!ps->header_area)
+		goto err2;
+
 	return 0;
+
+err2:
+	vfree(ps->zero_area);
+err1:
+	vfree(ps->area);
+err0:
+	return r;
 }
 
 static void free_area(struct pstore *ps)
@@ -169,6 +185,10 @@ static void free_area(struct pstore *ps)
 	if (ps->zero_area)
 		vfree(ps->zero_area);
 	ps->zero_area = NULL;
+
+	if (ps->header_area)
+		vfree(ps->header_area);
+	ps->header_area = NULL;
 }
 
 struct mdata_req {
@@ -284,11 +304,11 @@ static int read_header(struct pstore *ps
 	if (r)
 		return r;
 
-	r = chunk_io(ps, ps->area, 0, READ, 1);
+	r = chunk_io(ps, ps->header_area, 0, READ, 1);
 	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;
@@ -338,15 +358,15 @@ static int write_header(struct pstore *p
 {
 	struct disk_header *dh;
 
-	memset(ps->area, 0, ps->store->chunk_size << SECTOR_SHIFT);
+	memset(ps->header_area, 0, ps->store->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->store->chunk_size);
 
-	return chunk_io(ps, ps->area, 0, WRITE, 1);
+	return chunk_io(ps, ps->header_area, 0, WRITE, 1);
 }
 
 /*
@@ -666,6 +686,8 @@ static int persistent_ctr(struct dm_exce
 	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;
 

[PATCH 3/4] Refactor set_chunk_size.

[Mikulas Patocka]

Break the function set_chunk_size to two functions.

It doesn't change functionality, just makes the following patch simpler
and avoids code duplication.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-exception-store.c |    7 +++++++
 drivers/md/dm-exception-store.h |    4 ++++
 2 files changed, 11 insertions(+)

Index: linux-2.6.31-rc5-devel/drivers/md/dm-exception-store.c
===================================================================
--- linux-2.6.31-rc5-devel.orig/drivers/md/dm-exception-store.c	2009-08-04 03:00:55.000000000 +0200
+++ linux-2.6.31-rc5-devel/drivers/md/dm-exception-store.c	2009-08-04 03:01:32.000000000 +0200
@@ -171,6 +171,13 @@ static int set_chunk_size(struct dm_exce
 	 */
 	chunk_size_ulong = round_up(chunk_size_ulong, PAGE_SIZE >> 9);
 
+	return dm_exception_store_set_chunk_size(store, chunk_size_ulong, error);
+}
+
+int dm_exception_store_set_chunk_size(struct dm_exception_store *store,
+				      unsigned long chunk_size_ulong,
+				      char **error)
+{
 	/* Check chunk_size is a power of 2 */
 	if (!is_power_of_2(chunk_size_ulong)) {
 		*error = "Chunk size is not a power of 2";
Index: linux-2.6.31-rc5-devel/drivers/md/dm-exception-store.h
===================================================================
--- linux-2.6.31-rc5-devel.orig/drivers/md/dm-exception-store.h	2009-08-04 03:00:55.000000000 +0200
+++ linux-2.6.31-rc5-devel/drivers/md/dm-exception-store.h	2009-08-04 03:01:32.000000000 +0200
@@ -168,6 +168,10 @@ static inline chunk_t sector_to_chunk(st
 int dm_exception_store_type_register(struct dm_exception_store_type *type);
 int dm_exception_store_type_unregister(struct dm_exception_store_type *type);
 
+int dm_exception_store_set_chunk_size(struct dm_exception_store *store,
+				      unsigned long chunk_size_ulong,
+				      char **error);
+
 int dm_exception_store_create(struct dm_target *ti, int argc, char **argv,
 			      unsigned *args_used,
 			      struct dm_exception_store **store);

[PATCH 4/4] Check on-disk chunk size.

[Mikulas Patocka]

Check on-disk chunk size.

The previous code blindly used the chunk size stored on disk. This is wrong
because it can lead to a crash if the disk storage is corrupted. See

https://bugzilla.redhat.com/show_bug.cgi?id=461506

This code passes on-disk size through similar checks as size specified
on the table argument line, thus it won't allow to use bogus values (such
as zero, in case of that bug).

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-snap-persistent.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Index: linux-2.6.31-rc5-devel/drivers/md/dm-snap-persistent.c
===================================================================
--- linux-2.6.31-rc5-devel.orig/drivers/md/dm-snap-persistent.c	2009-08-04 03:20:11.000000000 +0200
+++ linux-2.6.31-rc5-devel/drivers/md/dm-snap-persistent.c	2009-08-04 03:24:16.000000000 +0200
@@ -283,6 +283,7 @@ static int read_header(struct pstore *ps
 	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
@@ -326,20 +327,23 @@ static int read_header(struct pstore *ps
 	ps->version = le32_to_cpu(dh->version);
 	chunk_size = le32_to_cpu(dh->chunk_size);
 
-	if (!chunk_size_supplied || ps->store->chunk_size == chunk_size)
+	if (ps->store->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->store->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->store->chunk_size);
 
 	/* We had a bogus chunk_size. Fix stuff up. */
 	free_area(ps);
 
-	ps->store->chunk_size = chunk_size;
-	ps->store->chunk_mask = chunk_size - 1;
-	ps->store->chunk_shift = ffs(chunk_size) - 1;
+	r = dm_exception_store_set_chunk_size(ps->store, 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->store->chunk_size),
 				ps->io_client);