[PATCH] dm transaction manager: handle space map checker failure

[PATCH] dm transaction manager: handle space map checker failure

[Mike Snitzer]

If CONFIG_DM_DEBUG_SPACE_MAPS is enabled and dm_sm_checker_create()
fails, dm_tm_create_internal() would still return success even though it
cleaned up all resources it was supposed to have created.

Fix the space map checker code to return an appropriate ERR_PTR and have
dm_tm_create_internal() check for it with IS_ERR.

Reported-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/md/persistent-data/dm-space-map-checker.c  |   24 ++++++++++----------
 .../md/persistent-data/dm-transaction-manager.c    |    8 +++++-
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/md/persistent-data/dm-space-map-checker.c b/drivers/md/persistent-data/dm-space-map-checker.c
index 50ed53b..6d7c832 100644
--- a/drivers/md/persistent-data/dm-space-map-checker.c
+++ b/drivers/md/persistent-data/dm-space-map-checker.c
@@ -343,25 +343,25 @@ struct dm_space_map *dm_sm_checker_create(struct dm_space_map *sm)
 	int r;
 	struct sm_checker *smc;
 
-	if (!sm)
-		return NULL;
+	if (IS_ERR_OR_NULL(sm))
+		return ERR_PTR(-EINVAL);
 
 	smc = kmalloc(sizeof(*smc), GFP_KERNEL);
 	if (!smc)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	memcpy(&smc->sm, &ops_, sizeof(smc->sm));
 	r = ca_create(&smc->old_counts, sm);
 	if (r) {
 		kfree(smc);
-		return NULL;
+		return ERR_PTR(r);
 	}
 
 	r = ca_create(&smc->counts, sm);
 	if (r) {
 		ca_destroy(&smc->old_counts);
 		kfree(smc);
-		return NULL;
+		return ERR_PTR(r);
 	}
 
 	smc->real_sm = sm;
@@ -371,7 +371,7 @@ struct dm_space_map *dm_sm_checker_create(struct dm_space_map *sm)
 		ca_destroy(&smc->counts);
 		ca_destroy(&smc->old_counts);
 		kfree(smc);
-		return NULL;
+		return ERR_PTR(r);
 	}
 
 	r = ca_commit(&smc->old_counts, &smc->counts);
@@ -379,7 +379,7 @@ struct dm_space_map *dm_sm_checker_create(struct dm_space_map *sm)
 		ca_destroy(&smc->counts);
 		ca_destroy(&smc->old_counts);
 		kfree(smc);
-		return NULL;
+		return ERR_PTR(r);
 	}
 
 	return &smc->sm;
@@ -391,25 +391,25 @@ struct dm_space_map *dm_sm_checker_create_fresh(struct dm_space_map *sm)
 	int r;
 	struct sm_checker *smc;
 
-	if (!sm)
-		return NULL;
+	if (IS_ERR_OR_NULL(sm))
+		return ERR_PTR(-EINVAL);
 
 	smc = kmalloc(sizeof(*smc), GFP_KERNEL);
 	if (!smc)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	memcpy(&smc->sm, &ops_, sizeof(smc->sm));
 	r = ca_create(&smc->old_counts, sm);
 	if (r) {
 		kfree(smc);
-		return NULL;
+		return ERR_PTR(r);
 	}
 
 	r = ca_create(&smc->counts, sm);
 	if (r) {
 		ca_destroy(&smc->old_counts);
 		kfree(smc);
-		return NULL;
+		return ERR_PTR(r);
 	}
 
 	smc->real_sm = sm;
diff --git a/drivers/md/persistent-data/dm-transaction-manager.c b/drivers/md/persistent-data/dm-transaction-manager.c
index 02bf78e..e5604b3 100644
--- a/drivers/md/persistent-data/dm-transaction-manager.c
+++ b/drivers/md/persistent-data/dm-transaction-manager.c
@@ -347,8 +347,10 @@ static int dm_tm_create_internal(struct dm_block_manager *bm,
 		}
 
 		*sm = dm_sm_checker_create(inner);
-		if (!*sm)
+		if (IS_ERR(*sm)) {
+			r = PTR_ERR(*sm);
 			goto bad2;
+		}
 
 	} else {
 		r = dm_bm_write_lock(dm_tm_get_bm(*tm), sb_location,
@@ -367,8 +369,10 @@ static int dm_tm_create_internal(struct dm_block_manager *bm,
 		}
 
 		*sm = dm_sm_checker_create(inner);
-		if (!*sm)
+		if (IS_ERR(*sm)) {
+			r = PTR_ERR(*sm);
 			goto bad2;
+		}
 	}
 
 	return 0;
-- 
1.7.4.4

Re: [PATCH] dm transaction manager: handle space map checker failure

[Vivek Goyal]

On Wed, Jun 20, 2012 at 03:24:59PM -0400, Mike Snitzer wrote:
> If CONFIG_DM_DEBUG_SPACE_MAPS is enabled and dm_sm_checker_create()
> fails, dm_tm_create_internal() would still return success even though it
> cleaned up all resources it was supposed to have created.
> 
> Fix the space map checker code to return an appropriate ERR_PTR and have
> dm_tm_create_internal() check for it with IS_ERR.
> 

I tested the patch and it works. It fails gracefully instead of segfaulting.

device-mapper: reload ioctl failed: Cannot allocate memory

I still do get waring though about large memory allocation. That's a
separate issue though.

Thanks
Vivek

> Reported-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/md/persistent-data/dm-space-map-checker.c  |   24 ++++++++++----------
>  .../md/persistent-data/dm-transaction-manager.c    |    8 +++++-
>  2 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/md/persistent-data/dm-space-map-checker.c b/drivers/md/persistent-data/dm-space-map-checker.c
> index 50ed53b..6d7c832 100644
> --- a/drivers/md/persistent-data/dm-space-map-checker.c
> +++ b/drivers/md/persistent-data/dm-space-map-checker.c
> @@ -343,25 +343,25 @@ struct dm_space_map *dm_sm_checker_create(struct dm_space_map *sm)
>  	int r;
>  	struct sm_checker *smc;
>  
> -	if (!sm)
> -		return NULL;
> +	if (IS_ERR_OR_NULL(sm))
> +		return ERR_PTR(-EINVAL);
>  
>  	smc = kmalloc(sizeof(*smc), GFP_KERNEL);
>  	if (!smc)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	memcpy(&smc->sm, &ops_, sizeof(smc->sm));
>  	r = ca_create(&smc->old_counts, sm);
>  	if (r) {
>  		kfree(smc);
> -		return NULL;
> +		return ERR_PTR(r);
>  	}
>  
>  	r = ca_create(&smc->counts, sm);
>  	if (r) {
>  		ca_destroy(&smc->old_counts);
>  		kfree(smc);
> -		return NULL;
> +		return ERR_PTR(r);
>  	}
>  
>  	smc->real_sm = sm;
> @@ -371,7 +371,7 @@ struct dm_space_map *dm_sm_checker_create(struct dm_space_map *sm)
>  		ca_destroy(&smc->counts);
>  		ca_destroy(&smc->old_counts);
>  		kfree(smc);
> -		return NULL;
> +		return ERR_PTR(r);
>  	}
>  
>  	r = ca_commit(&smc->old_counts, &smc->counts);
> @@ -379,7 +379,7 @@ struct dm_space_map *dm_sm_checker_create(struct dm_space_map *sm)
>  		ca_destroy(&smc->counts);
>  		ca_destroy(&smc->old_counts);
>  		kfree(smc);
> -		return NULL;
> +		return ERR_PTR(r);
>  	}
>  
>  	return &smc->sm;
> @@ -391,25 +391,25 @@ struct dm_space_map *dm_sm_checker_create_fresh(struct dm_space_map *sm)
>  	int r;
>  	struct sm_checker *smc;
>  
> -	if (!sm)
> -		return NULL;
> +	if (IS_ERR_OR_NULL(sm))
> +		return ERR_PTR(-EINVAL);
>  
>  	smc = kmalloc(sizeof(*smc), GFP_KERNEL);
>  	if (!smc)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	memcpy(&smc->sm, &ops_, sizeof(smc->sm));
>  	r = ca_create(&smc->old_counts, sm);
>  	if (r) {
>  		kfree(smc);
> -		return NULL;
> +		return ERR_PTR(r);
>  	}
>  
>  	r = ca_create(&smc->counts, sm);
>  	if (r) {
>  		ca_destroy(&smc->old_counts);
>  		kfree(smc);
> -		return NULL;
> +		return ERR_PTR(r);
>  	}
>  
>  	smc->real_sm = sm;
> diff --git a/drivers/md/persistent-data/dm-transaction-manager.c b/drivers/md/persistent-data/dm-transaction-manager.c
> index 02bf78e..e5604b3 100644
> --- a/drivers/md/persistent-data/dm-transaction-manager.c
> +++ b/drivers/md/persistent-data/dm-transaction-manager.c
> @@ -347,8 +347,10 @@ static int dm_tm_create_internal(struct dm_block_manager *bm,
>  		}
>  
>  		*sm = dm_sm_checker_create(inner);
> -		if (!*sm)
> +		if (IS_ERR(*sm)) {
> +			r = PTR_ERR(*sm);
>  			goto bad2;
> +		}
>  
>  	} else {
>  		r = dm_bm_write_lock(dm_tm_get_bm(*tm), sb_location,
> @@ -367,8 +369,10 @@ static int dm_tm_create_internal(struct dm_block_manager *bm,
>  		}
>  
>  		*sm = dm_sm_checker_create(inner);
> -		if (!*sm)
> +		if (IS_ERR(*sm)) {
> +			r = PTR_ERR(*sm);
>  			goto bad2;
> +		}
>  	}
>  
>  	return 0;
> -- 
> 1.7.4.4

Re: [PATCH] dm transaction manager: handle space map checker failure

[Vivek Goyal]

On Wed, Jun 20, 2012 at 05:20:10PM -0400, Vivek Goyal wrote:
> On Wed, Jun 20, 2012 at 03:24:59PM -0400, Mike Snitzer wrote:
> > If CONFIG_DM_DEBUG_SPACE_MAPS is enabled and dm_sm_checker_create()
> > fails, dm_tm_create_internal() would still return success even though it
> > cleaned up all resources it was supposed to have created.
> > 
> > Fix the space map checker code to return an appropriate ERR_PTR and have
> > dm_tm_create_internal() check for it with IS_ERR.
> > 
> 
> I tested the patch and it works. It fails gracefully instead of segfaulting.
> 
> device-mapper: reload ioctl failed: Cannot allocate memory
> 
> I still do get waring though about large memory allocation. That's a
> separate issue though.

I put a trace_printk() in ca_create() to see how much memory we are trying
to allocated using kzalloc. And answer is 10485760. Number of blocks
obtained from space map is 2621440. I think this might be happening because 
my metadata device size is 10G.

Thanks
Vivek

Re: dm transaction manager: handle space map checker failure

[Mike Snitzer]

On Wed, Jun 20 2012 at  5:37pm -0400,
Vivek Goyal <vgoyal@redhat.com> wrote:

> On Wed, Jun 20, 2012 at 05:20:10PM -0400, Vivek Goyal wrote:
> > On Wed, Jun 20, 2012 at 03:24:59PM -0400, Mike Snitzer wrote:
> > > If CONFIG_DM_DEBUG_SPACE_MAPS is enabled and dm_sm_checker_create()
> > > fails, dm_tm_create_internal() would still return success even though it
> > > cleaned up all resources it was supposed to have created.
> > > 
> > > Fix the space map checker code to return an appropriate ERR_PTR and have
> > > dm_tm_create_internal() check for it with IS_ERR.
> > > 
> > 
> > I tested the patch and it works. It fails gracefully instead of segfaulting.
> > 
> > device-mapper: reload ioctl failed: Cannot allocate memory
> > 
> > I still do get waring though about large memory allocation. That's a
> > separate issue though.
> 
> I put a trace_printk() in ca_create() to see how much memory we are trying
> to allocated using kzalloc. And answer is 10485760. Number of blocks
> obtained from space map is 2621440. I think this might be happening because 
> my metadata device size is 10G.

It is.  My metadata device is 1G and I'm seeing nr_blocks=262144

So kzalloc on your system cannot find 10M of contiguous memory.

How does this patch work for you?

diff --git a/drivers/md/persistent-data/dm-space-map-checker.c b/drivers/md/persistent-data/dm-space-map-checker.c
index 6d7c832..75ab11f 100644
--- a/drivers/md/persistent-data/dm-space-map-checker.c
+++ b/drivers/md/persistent-data/dm-space-map-checker.c
@@ -89,7 +89,7 @@ static int ca_create(struct count_array *ca, struct dm_space_map *sm)
 
 	ca->nr = nr_blocks;
 	ca->nr_free = nr_blocks;
-	ca->counts = kzalloc(sizeof(*ca->counts) * nr_blocks, GFP_KERNEL);
+	ca->counts = vzalloc(sizeof(*ca->counts) * nr_blocks);
 	if (!ca->counts)
 		return -ENOMEM;
 

Re: dm transaction manager: handle space map checker failure

[Mike Snitzer]

On Wed, Jun 20 2012 at  6:15pm -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Wed, Jun 20 2012 at  5:37pm -0400,
> Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > On Wed, Jun 20, 2012 at 05:20:10PM -0400, Vivek Goyal wrote:
> > > On Wed, Jun 20, 2012 at 03:24:59PM -0400, Mike Snitzer wrote:
> > > > If CONFIG_DM_DEBUG_SPACE_MAPS is enabled and dm_sm_checker_create()
> > > > fails, dm_tm_create_internal() would still return success even though it
> > > > cleaned up all resources it was supposed to have created.
> > > > 
> > > > Fix the space map checker code to return an appropriate ERR_PTR and have
> > > > dm_tm_create_internal() check for it with IS_ERR.
> > > > 
> > > 
> > > I tested the patch and it works. It fails gracefully instead of segfaulting.
> > > 
> > > device-mapper: reload ioctl failed: Cannot allocate memory
> > > 
> > > I still do get waring though about large memory allocation. That's a
> > > separate issue though.
> > 
> > I put a trace_printk() in ca_create() to see how much memory we are trying
> > to allocated using kzalloc. And answer is 10485760. Number of blocks
> > obtained from space map is 2621440. I think this might be happening because 
> > my metadata device size is 10G.
> 
> It is.  My metadata device is 1G and I'm seeing nr_blocks=262144
> 
> So kzalloc on your system cannot find 10M of contiguous memory.
> 
> How does this patch work for you?

Hey Vivek,

Here is a more comprehensive patch (seems vmalloc doesn't support
passing a @size of 0, whereas kmalloc does).

But I know that this patch causes sm_checker_destroy() to crash in the
kfree() from ca_destroy(&smc->old_counts);

If I simply switch back from vzalloc to using kzalloc all works fine!?

Seems very odd, will dig deeper when I get a chance.

commit 65f1ea9401aeec7618626c3c32defbee5db30deb
Author: Mike Snitzer <snitzer@redhat.com>
Date:   Thu Jun 21 00:05:18 2012 -0400

    dm space map: fix error path of space map checker creation

diff --git a/drivers/md/persistent-data/dm-space-map-checker.c b/drivers/md/persistent-data/dm-space-map-checker.c
index 6d7c832..d4463dc 100644
--- a/drivers/md/persistent-data/dm-space-map-checker.c
+++ b/drivers/md/persistent-data/dm-space-map-checker.c
@@ -89,9 +89,13 @@ static int ca_create(struct count_array *ca, struct dm_space_map *sm)
 
 	ca->nr = nr_blocks;
 	ca->nr_free = nr_blocks;
-	ca->counts = kzalloc(sizeof(*ca->counts) * nr_blocks, GFP_KERNEL);
-	if (!ca->counts)
-		return -ENOMEM;
+
+	if (nr_blocks) {
+		ca->counts = vzalloc(sizeof(*ca->counts) * nr_blocks);
+		if (!ca->counts)
+			return -ENOMEM;
+	} else
+		ca->counts = NULL;
 
 	return 0;
 }
@@ -126,12 +130,14 @@ static int ca_load(struct count_array *ca, struct dm_space_map *sm)
 static int ca_extend(struct count_array *ca, dm_block_t extra_blocks)
 {
 	dm_block_t nr_blocks = ca->nr + extra_blocks;
-	uint32_t *counts = kzalloc(sizeof(*counts) * nr_blocks, GFP_KERNEL);
+	uint32_t *counts = vzalloc(sizeof(*counts) * nr_blocks);
 	if (!counts)
 		return -ENOMEM;
 
-	memcpy(counts, ca->counts, sizeof(*counts) * ca->nr);
-	kfree(ca->counts);
+	if (ca->counts) {
+		memcpy(counts, ca->counts, sizeof(*counts) * ca->nr);
+		kfree(ca->counts);
+	}
 	ca->nr = nr_blocks;
 	ca->nr_free += extra_blocks;
 	ca->counts = counts;
diff --git a/drivers/md/persistent-data/dm-space-map-disk.c b/drivers/md/persistent-data/dm-space-map-disk.c
index 9535234..124828b 100644
--- a/drivers/md/persistent-data/dm-space-map-disk.c
+++ b/drivers/md/persistent-data/dm-space-map-disk.c
@@ -315,7 +315,13 @@ struct dm_space_map *dm_sm_disk_create(struct dm_transaction_manager *tm,
 				       dm_block_t nr_blocks)
 {
 	struct dm_space_map *sm = dm_sm_disk_create_real(tm, nr_blocks);
-	return dm_sm_checker_create_fresh(sm);
+	struct dm_space_map *smc = dm_sm_checker_create_fresh(sm);
+
+	if (IS_ERR(smc) && !IS_ERR_OR_NULL(sm))
+		dm_sm_destroy(sm);
+	sm = smc;
+
+	return sm;
 }
 EXPORT_SYMBOL_GPL(dm_sm_disk_create);
 

Re: dm transaction manager: handle space map checker failure

[Mike Snitzer]

On Thu, Jun 21 2012 at  1:10am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> Hey Vivek,
> 
> Here is a more comprehensive patch (seems vmalloc doesn't support
> passing a @size of 0, whereas kmalloc does).
> 
> But I know that this patch causes sm_checker_destroy() to crash in the
> kfree() from ca_destroy(&smc->old_counts);
> 
> If I simply switch back from vzalloc to using kzalloc all works fine!?
> 
> Seems very odd, will dig deeper when I get a chance.

Not odd, just a stupid oversight on my part.  Needed to switch to using
vfree too ;)

Here is an updated patch.

(Joe and Alasdair, I'll post 2 new cleaned up patches for 3.5; the
dm-space-map-disk.c change in this patch needs to be folded into v2 of
the "dm transaction manager: handle space map checker failure" patch I
posted yesterday)

 drivers/md/persistent-data/dm-space-map-checker.c |   28 ++++++++++++--------
 drivers/md/persistent-data/dm-space-map-disk.c    |    8 +++++-
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/md/persistent-data/dm-space-map-checker.c b/drivers/md/persistent-data/dm-space-map-checker.c
index 6d7c832..f0eded3 100644
--- a/drivers/md/persistent-data/dm-space-map-checker.c
+++ b/drivers/md/persistent-data/dm-space-map-checker.c
@@ -89,13 +89,22 @@ static int ca_create(struct count_array *ca, struct dm_space_map *sm)
 
 	ca->nr = nr_blocks;
 	ca->nr_free = nr_blocks;
-	ca->counts = kzalloc(sizeof(*ca->counts) * nr_blocks, GFP_KERNEL);
-	if (!ca->counts)
-		return -ENOMEM;
+
+	if (nr_blocks) {
+		ca->counts = vzalloc(sizeof(*ca->counts) * nr_blocks);
+		if (!ca->counts)
+			return -ENOMEM;
+	} else
+		ca->counts = NULL;
 
 	return 0;
 }
 
+static void ca_destroy(struct count_array *ca)
+{
+	vfree(ca->counts);
+}
+
 static int ca_load(struct count_array *ca, struct dm_space_map *sm)
 {
 	int r;
@@ -126,12 +135,14 @@ static int ca_load(struct count_array *ca, struct dm_space_map *sm)
 static int ca_extend(struct count_array *ca, dm_block_t extra_blocks)
 {
 	dm_block_t nr_blocks = ca->nr + extra_blocks;
-	uint32_t *counts = kzalloc(sizeof(*counts) * nr_blocks, GFP_KERNEL);
+	uint32_t *counts = vzalloc(sizeof(*counts) * nr_blocks);
 	if (!counts)
 		return -ENOMEM;
 
-	memcpy(counts, ca->counts, sizeof(*counts) * ca->nr);
-	kfree(ca->counts);
+	if (ca->counts) {
+		memcpy(counts, ca->counts, sizeof(*counts) * ca->nr);
+		ca_destroy(ca);
+	}
 	ca->nr = nr_blocks;
 	ca->nr_free += extra_blocks;
 	ca->counts = counts;
@@ -151,11 +162,6 @@ static int ca_commit(struct count_array *old, struct count_array *new)
 	return 0;
 }
 
-static void ca_destroy(struct count_array *ca)
-{
-	kfree(ca->counts);
-}
-
 /*----------------------------------------------------------------*/
 
 struct sm_checker {
diff --git a/drivers/md/persistent-data/dm-space-map-disk.c b/drivers/md/persistent-data/dm-space-map-disk.c
index 9535234..124828b 100644
--- a/drivers/md/persistent-data/dm-space-map-disk.c
+++ b/drivers/md/persistent-data/dm-space-map-disk.c
@@ -315,7 +315,13 @@ struct dm_space_map *dm_sm_disk_create(struct dm_transaction_manager *tm,
 				       dm_block_t nr_blocks)
 {
 	struct dm_space_map *sm = dm_sm_disk_create_real(tm, nr_blocks);
-	return dm_sm_checker_create_fresh(sm);
+	struct dm_space_map *smc = dm_sm_checker_create_fresh(sm);
+
+	if (IS_ERR(smc) && !IS_ERR_OR_NULL(sm))
+		dm_sm_destroy(sm);
+	sm = smc;
+
+	return sm;
 }
 EXPORT_SYMBOL_GPL(dm_sm_disk_create);
 

Re: dm transaction manager: handle space map checker failure

[Joe Thornber]

On Thu, Jun 21, 2012 at 02:10:09AM -0400, Mike Snitzer wrote:
> (Joe and Alasdair, I'll post 2 new cleaned up patches for 3.5; the
> dm-space-map-disk.c change in this patch needs to be folded into v2 of
> the "dm transaction manager: handle space map checker failure" patch I
> posted yesterday)

I think it's time to pull that checker completely.  Confidence in the
space maps is higher these days, and this checker is very expensive in
memory for realistic sized pools.

- Joe