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

BUG In review Satoru Takeuchi 3 participants 7 messages Jul 12, 2014 – Jul 22, 2014 Device mapper

lore

Share & tools

lore

Satoru Takeuchi

author

2014-07-12 09:06 UTC #0 lore raw

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

Akira Hayakawa

2014-07-18 01:01 UTC #1 lore raw

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

Quoted 86 lines · On Sat, 12 Jul 2014 18:06:58 +0900 Satoru Takeuchi <[email protected]>…

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
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

Satoru Takeuchi

author

2014-07-19 02:13 UTC #2 lore raw

Hi Akira,

2014-07-18 10:01 GMT+09:00 Akira Hayakawa <[email protected]>:

Quoted 8 lines

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.

Quoted 6 lines

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

Quoted 92 lines

- Akira
Quoted 86 lines · On Sat, 12 Jul 2014 18:06:58 +0900 Satoru Takeuchi <[email protected]>…
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
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

Satoru Takeuchi

author

2014-07-19 11:40 UTC #3 lore raw

PATCH 1/1 dm-writeboost: Remove unsure BUG() from init_rambuf_pool

Hi Joe and Akira,

At Sat, 19 Jul 2014 11:13:13 +0900,

Quoted 12 lines · Satoru Takeuchi

Hi Akira,
2014-07-18 10:01 GMT+09:00 Akira Hayakawa <[email protected]>:
Quoted 8 lines
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 <[email protected]>

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 <[email protected]>

Cc Joe Thornber <[email protected]>

Cc Akira Hayakawa <[email protected]>

---
 drivers/md/dm-writeboost-metadata.c | 1 -
 1 file changed, 1 deletion(-)

drivers/md/dm-writeboost-metadata.c +0 -1

index b7b3eb7..ce6ea056 100644
@@ -702,7 +702,6 @@ bad_alloc_data:
 	kmem_cache_destroy(wb->rambuf_cachep);
 bad_cachep:
 	kfree(wb->rambuf_pool);
-	BUG();
 	return r;
 }

Signature · 1 line

2.0.1

Alasdair G Kergon

maintainer

2014-07-19 18:13 UTC #4 lore raw

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

Quoted 2 lines · Satoru Takeuchi

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

Satoru Takeuchi

author

2014-07-21 13:52 UTC #5 lore raw

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

Hi Alasdaia,

2014-07-20 3:13 GMT+09:00 Alasdair G Kergon <[email protected]>:

Quoted 7 lines · Satoru Takeuchi

Quoted 2 lines
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.

Quoted 6 lines

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

Quoted 10 lines

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

Akira Hayakawa

2014-07-22 07:14 UTC #6 lore raw

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

In reply to Satoru Takeuchi

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

Quoted 107 lines · On Sat, 19 Jul 2014 20:40:52 +0900 Satoru Takeuchi <[email protected]>…

Hi Joe and Akira,
At Sat, 19 Jul 2014 11:13:13 +0900,
Quoted 12 lines · Satoru Takeuchi
Hi Akira,
2014-07-18 10:01 GMT+09:00 Akira Hayakawa <[email protected]>:
Quoted 8 lines
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 <[email protected]>
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 <[email protected]>
Cc Joe Thornber <[email protected]>
Cc Akira Hayakawa <[email protected]>
---
 drivers/md/dm-writeboost-metadata.c | 1 -
 1 file changed, 1 deletion(-)
drivers/md/dm-writeboost-metadata.c +0 -1
index b7b3eb7..ce6ea056 100644
@@ -702,7 +702,6 @@ bad_alloc_data:
 	kmem_cache_destroy(wb->rambuf_cachep);
 bad_cachep:
 	kfree(wb->rambuf_pool);
-	BUG();
 	return r;
 }
 
Signature · 6 lines
2.0.1

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

Signature · 1 line

Akira Hayakawa <[email protected]>