[dm-devel] [PATCH] dm-writeboost: Remove unsure BUG() from init_rambuf_pool

[Satoru Takeuchi]

Hi Joe and Akira,

At Sat, 19 Jul 2014 11:13:13 +0900,
Satoru Takeuchi wrote:
> 
> Hi Akira,
> 
> 2014-07-18 10:01 GMT+09:00 Akira Hayakawa <ruby.wktk at gmail.com>:
> > Hi, Satoru,
> >
> > I totally agree with removing the BUG() at (2).
> >
> > However, I don't agree with setting the upper limit of nr_rambuf_pool.
> > You are saying that any memory allocation failure on initialization should be avoided
> > but that sounds going too far. Removing that possibility from all kernel codes is impossible in practice.
> > Carefully terminating the initialization on allocation failure is sufficient.

Both Akira and me seem to have completely different policy about the upper
limit of nr_rambuf_pool argument. However both of us agree with removing
unsure BUG() in int init_rambuf_pool(). So I wrote a patch to do so as a
 first step. Please apply this patch.

In addition, I'd like to know your opinion about whether setting
the upper limit of nr_rambuf_pool argument is neccesary or not.



=======
From: Satoru Takeuchi <satoru.takeuchi at gmail.com>

If users set a very large value to nr_rambuf_pool argument, kernel would hit
BUG() in the error route of init_rambuf_pool().

drivers/md/dm-writeboost-metadata.c:
===============================================================================
...
static int init_rambuf_pool(struct wb_device *wb)
{
	int r = 0;
	size_t i;

	wb->rambuf_pool = kmalloc(sizeof(struct rambuffer) * wb->nr_rambuf_pool,
				  GFP_KERNEL);
	if (!wb->rambuf_pool)			# It is true here.
		return -ENOMEM;

	wb->rambuf_cachep = kmem_cache_create("dmwb_rambuf",
			1 << (wb->segment_size_order + SECTOR_SHIFT),
			1 << (wb->segment_size_order + SECTOR_SHIFT),
			SLAB_RED_ZONE, NULL);
	if (!wb->rambuf_cachep) {
		r = -ENOMEM;			# Enter this route or ....
		goto bad_cachep;
	}

	for (i = 0; i < wb->nr_rambuf_pool; i++) {
		size_t j;
		struct rambuffer *rambuf = wb->rambuf_pool + i;

		rambuf->data = kmem_cache_alloc(wb->rambuf_cachep, GFP_KERNEL);
		if (!rambuf->data) {
			...			# ... enter this route.
			goto bad_alloc_data;
		}
		check_buffer_alignment(rambuf->data);
	}

	return r;

bad_alloc_data:
	kmem_cache_destroy(wb->rambuf_cachep);
bad_cachep:
	kfree(wb->rambuf_pool);
	BUG();					# Kernel panic happens here.
	return r;
}
...
===============================================================================

Probably this BUG() was introduced erroneously and is safe to remove.

Signed-off-by: Satoru Takeuchi <satoru.takeuchi at gmail.com>
Cc: Joe Thornber <ejt at redhat.com>
Cc: Akira Hayakawa <ruby.wktk at gmail.com>
---
 drivers/md/dm-writeboost-metadata.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/md/dm-writeboost-metadata.c b/drivers/md/dm-writeboost-metadata.c
index b7b3eb7..ce6ea056 100644
--- a/drivers/md/dm-writeboost-metadata.c
+++ b/drivers/md/dm-writeboost-metadata.c
@@ -702,7 +702,6 @@ bad_alloc_data:
 	kmem_cache_destroy(wb->rambuf_cachep);
 bad_cachep:
 	kfree(wb->rambuf_pool);
-	BUG();
 	return r;
 }
 
-- 
2.0.1