[BUG] dm-writeboost: too big nr_rambuf_pool causes massive memory pressure or panic.

[BUG] dm-writeboost: too big nr_rambuf_pool causes massive memory pressure or panic.

[Satoru Takeuchi]

Hi,

Since the upper limit of nr_rambuf_pool argument, user can set too big value
to this argument.

drivers/md/dm-writeboost-target.c:
===============================================================================
static int consume_optional_argv(struct wb_device *wb, struct dm_arg_set *as)
{
        int r = 0;
        struct dm_target *ti = wb->ti;

        static struct dm_arg _args[] = {
                {0, 4, "Invalid optional argc"},
                {4, 10, "Invalid segment_size_order"},
                {1, UINT_MAX, "Invalid nr_rambuf_pool"},
        };
===============================================================================

It can cause the massive memory pressure to kernel by user's mistake
and results in

 - ENOMEM at the (1) or somewhere, and
 - panic at (2).

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)
                return -ENOMEM; # ................. (1)

        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;
                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) {
                        WBERR("Failed to allocate rambuf->data");
                        for (j = 0; j < i; j++) {
                                rambuf = wb->rambuf_pool + j;
                                kmem_cache_free(wb->rambuf_cachep, rambuf->data);
                        }
                        r = -ENOMEM;
                        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();                         # ........(2)
        return r;
}
...
===============================================================================

Please decide the appropriate upper limit of nr_rambuf_pool (unfortunately
I don't know about it), and remove BUG() at (2) if it's unnecessary.

Thanks,
Satoru

Re: [BUG] dm-writeboost: too big nr_rambuf_pool causes massive memory pressure or panic.

[Akira Hayakawa]

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.

For your guidance,
the amount of memory we need depends on how fast the SSD is.
In practice, few MB for RAM buffer is sufficient for
commodity SSD as caching device. However, it could be higher with much more faster SSD
or one may want to set it up for higher value in experiments.

- Akira

On Sat, 12 Jul 2014 18:06:58 +0900
Satoru Takeuchi <satoru.takeuchi@gmail.com> wrote:

> Hi,
> 
> Since the upper limit of nr_rambuf_pool argument, user can set too big value
> to this argument.
> 
> drivers/md/dm-writeboost-target.c:
> ===============================================================================
> static int consume_optional_argv(struct wb_device *wb, struct dm_arg_set *as)
> {
>         int r = 0;
>         struct dm_target *ti = wb->ti;
> 
>         static struct dm_arg _args[] = {
>                 {0, 4, "Invalid optional argc"},
>                 {4, 10, "Invalid segment_size_order"},
>                 {1, UINT_MAX, "Invalid nr_rambuf_pool"},
>         };
> ===============================================================================
> 
> It can cause the massive memory pressure to kernel by user's mistake
> and results in
> 
>  - ENOMEM at the (1) or somewhere, and
>  - panic at (2).
> 
> 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)
>                 return -ENOMEM; # ................. (1)
> 
>         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;
>                 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) {
>                         WBERR("Failed to allocate rambuf->data");
>                         for (j = 0; j < i; j++) {
>                                 rambuf = wb->rambuf_pool + j;
>                                 kmem_cache_free(wb->rambuf_cachep, rambuf->data);
>                         }
>                         r = -ENOMEM;
>                         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();                         # ........(2)
>         return r;
> }
> ...
> ===============================================================================
> 
> Please decide the appropriate upper limit of nr_rambuf_pool (unfortunately
> I don't know about it), and remove BUG() at (2) if it's unnecessary.
> 
> Thanks,
> Satoru
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [BUG] dm-writeboost: too big nr_rambuf_pool causes massive memory pressure or panic.

[Satoru Takeuchi]

Hi Akira,

2014-07-18 10:01 GMT+09:00 Akira Hayakawa <ruby.wktk@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.

My opinion is not avoiding any memory allocation failure, but avoiding
easy-to-remove case.
This kind of ENOMEM can easily be avoided if user can't set too big
value to this argument.
This is completely the different from ENOMEM happened in the
actually-memory-exhausting
system.

In addition, even if ENOMEM doesn't happen, users can consume massive amount of
"kernel" memory. It results in the unstable system behavior, for
example users can't create
processes, almost all anon pages and page caches are evicted and can't
get it again
since kernel memory can't be reclaimable. In worst case, it can be a
security hole.
So, I consider it's apparently a bug.

>
> For your guidance,
> the amount of memory we need depends on how fast the SSD is.
> In practice, few MB for RAM buffer is sufficient for
> commodity SSD as caching device. However, it could be higher with much more faster SSD
> or one may want to set it up for higher value in experiments.

I don't think so. If sufficient upper limit can't easily be estimated,
setting almost all of
memory as write cache is inappropriate since it get whole system useless as I
mentioned above. Even if  you can't determine the appropriate upper
limit at all,
I strongly recommend you to set the upper limit lower than reclaimable
memory anyway.
For example "(free memory)/<n>(*1)."

Even If there is no theoretical reason to set such upper limit, it's
OK. It's better than
no upper limit case. It can avoid inappropriate ENOMEM and kernel memory
exhausting anyway.

*1) <n> is integer, for example 2, 3, 4.

Thanks,
Satoru

>
> - Akira
>
> On Sat, 12 Jul 2014 18:06:58 +0900
> Satoru Takeuchi <satoru.takeuchi@gmail.com> wrote:
>
>> Hi,
>>
>> Since the upper limit of nr_rambuf_pool argument, user can set too big value
>> to this argument.
>>
>> drivers/md/dm-writeboost-target.c:
>> ===============================================================================
>> static int consume_optional_argv(struct wb_device *wb, struct dm_arg_set *as)
>> {
>>         int r = 0;
>>         struct dm_target *ti = wb->ti;
>>
>>         static struct dm_arg _args[] = {
>>                 {0, 4, "Invalid optional argc"},
>>                 {4, 10, "Invalid segment_size_order"},
>>                 {1, UINT_MAX, "Invalid nr_rambuf_pool"},
>>         };
>> ===============================================================================
>>
>> It can cause the massive memory pressure to kernel by user's mistake
>> and results in
>>
>>  - ENOMEM at the (1) or somewhere, and
>>  - panic at (2).
>>
>> 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)
>>                 return -ENOMEM; # ................. (1)
>>
>>         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;
>>                 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) {
>>                         WBERR("Failed to allocate rambuf->data");
>>                         for (j = 0; j < i; j++) {
>>                                 rambuf = wb->rambuf_pool + j;
>>                                 kmem_cache_free(wb->rambuf_cachep, rambuf->data);
>>                         }
>>                         r = -ENOMEM;
>>                         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();                         # ........(2)
>>         return r;
>> }
>> ...
>> ===============================================================================
>>
>> Please decide the appropriate upper limit of nr_rambuf_pool (unfortunately
>> I don't know about it), and remove BUG() at (2) if it's unnecessary.
>>
>> Thanks,
>> Satoru
>>
>> --
>> dm-devel mailing list
>> dm-devel@redhat.com
>> https://www.redhat.com/mailman/listinfo/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@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@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@gmail.com>
Cc: Joe Thornber <ejt@redhat.com>
Cc: Akira Hayakawa <ruby.wktk@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

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

[Alasdair G Kergon]

On Sat, Jul 19, 2014 at 08:40:52PM +0900, Satoru Takeuchi wrote:
> In addition, I'd like to know your opinion about whether setting
> the upper limit of nr_rambuf_pool argument is neccesary or not.
 
1) If it's a parameter passed from userspace under the control of the
sysadmin, then you should just trust the sysadmin to set a sensible
value, and only return an error if the value they ask for is unobtainable.

Example: dm-ioctl.c copy_params()

2) If certain large values would be useless, leaving no memory for
anything else, then you could apply a sensible limit based on the amount 
of memory available.

Example: dm_bufio.c 

 * Memory management policy:
 *      Limit the number of buffers to DM_BUFIO_MEMORY_PERCENT of main memory
 *      or DM_BUFIO_VMALLOC_PERCENT of vmalloc memory (whichever is lower).
 *      Always allocate at least DM_BUFIO_MIN_BUFFERS buffers.

Alasdair

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

[Satoru Takeuchi]

Hi Alasdaia,

2014-07-20 3:13 GMT+09:00 Alasdair G Kergon <agk@redhat.com>:
> On Sat, Jul 19, 2014 at 08:40:52PM +0900, Satoru Takeuchi wrote:
>> In addition, I'd like to know your opinion about whether setting
>> the upper limit of nr_rambuf_pool argument is neccesary or not.
>
> 1) If it's a parameter passed from userspace under the control of the
> sysadmin, then you should just trust the sysadmin to set a sensible
> value, and only return an error if the value they ask for is unobtainable.

Thank you for your comment.

I don't say anything if this function returns an ENOMEM without any effect to
whole system. In this case, it fails *after* evicting massive amount of memory.
Since linux is used by very large amount of system, I consider someone will
eventually make a mistake. So I consider setting upper limit is better way
from the perspective of fool proof.

>
> Example: dm-ioctl.c copy_params()
>
> 2) If certain large values would be useless, leaving no memory for
> anything else, then you could apply a sensible limit based on the amount
> of memory available.

I agree with you.

It's difficult to guess the appropriate upper limit from the perspective of
dm-writeboost at this point since it's depend on system and requires
a number of performance measurements. So, the second best way is limiting
the rambuf size based on the available memory as you say. It can avoid
the above-mentioned problem anyway and very easy to implement.

Thanks,
Satoru

>
> Example: dm_bufio.c
>
>  * Memory management policy:
>  *      Limit the number of buffers to DM_BUFIO_MEMORY_PERCENT of main memory
>  *      or DM_BUFIO_VMALLOC_PERCENT of vmalloc memory (whichever is lower).
>  *      Always allocate at least DM_BUFIO_MIN_BUFFERS buffers.
>
> Alasdair
>

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

[Akira Hayakawa]

Hi, Satoru,

As for removing BUG() at the line I will definitely ack but
please also send a pull request to Joe's tree where we
are raising up dm-writeboost. Sorry, I didn't tell you this.

In my opinion, I like to see the patch in both pull request and dm-devel.
This shares where dm-writeboost is going to with DM guys.
If you have other works to dm-writeboost, please do as this time.

Note that where you send the pull request is
thin-dev branch of jthornber/linux-2.6.
It's where dm-writeboost is in.

Anyway, I think you start to involve dm-writeboost.
It is really welcome.
And bringing us discussion from user/sysadmin perspective
is greatly welcome, too.

- Akira



On Sat, 19 Jul 2014 20:40:52 +0900
Satoru Takeuchi <satoru.takeuchi@gmail.com> wrote:

> 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@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@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@gmail.com>
> Cc: Joe Thornber <ejt@redhat.com>
> Cc: Akira Hayakawa <ruby.wktk@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
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel


-- 
Akira Hayakawa <ruby.wktk@gmail.com>