Add ZPOOL_MAX_TYPE_NAME define, and change zpool_driver *type field to
type[ZPOOL_MAX_TYPE_NAME]. Remove redundant type field from struct zpool
and use zpool->driver->type instead.
The define will be used by zswap for its zpool param type name length.
Signed-off-by: Dan Streetman <[email protected]>
---
include/linux/zpool.h | 5 +++--
mm/zpool.c | 11 ++++-------
2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/include/linux/zpool.h b/include/linux/zpool.h
index 42f8ec9..cf70312 100644
--- a/include/linux/zpool.h
+++ b/include/linux/zpool.h
@@ -41,7 +41,7 @@ bool zpool_has_pool(char *type);
struct zpool *zpool_create_pool(char *type, char *name,
gfp_t gfp, const struct zpool_ops *ops);
-char *zpool_get_type(struct zpool *pool);
+const char *zpool_get_type(struct zpool *pool);
void zpool_destroy_pool(struct zpool *pool);
@@ -60,6 +60,7 @@ void zpool_unmap_handle(struct zpool *pool, unsigned long handle);
u64 zpool_get_total_size(struct zpool *pool);
+#define ZPOOL_MAX_TYPE_NAME 64
/**
* struct zpool_driver - driver implementation for zpool
@@ -78,7 +79,7 @@ u64 zpool_get_total_size(struct zpool *pool);
* with zpool.
*/
struct zpool_driver {
- char *type;
+ char type[ZPOOL_MAX_TYPE_NAME];
struct module *owner;
atomic_t refcount;
struct list_head list;
diff --git a/mm/zpool.c b/mm/zpool.c
index 8f670d3..8a0ef86 100644
--- a/mm/zpool.c
+++ b/mm/zpool.c
@@ -18,8 +18,6 @@
#include <linux/zpool.h>
struct zpool {
- char *type;
-
struct zpool_driver *driver;
void *pool;
const struct zpool_ops *ops;
@@ -79,7 +77,7 @@ static struct zpool_driver *zpool_get_driver(char *type)
spin_lock(&drivers_lock);
list_for_each_entry(driver, &drivers_head, list) {
- if (!strcmp(driver->type, type)) {
+ if (!strncmp(driver->type, type, ZPOOL_MAX_TYPE_NAME)) {
bool got = try_module_get(driver->owner);
if (got)
@@ -174,7 +172,6 @@ struct zpool *zpool_create_pool(char *type, char *name, gfp_t gfp,
return NULL;
}
- zpool->type = driver->type;
zpool->driver = driver;
zpool->pool = driver->create(name, gfp, ops, zpool);
zpool->ops = ops;
@@ -208,7 +205,7 @@ struct zpool *zpool_create_pool(char *type, char *name, gfp_t gfp,
*/
void zpool_destroy_pool(struct zpool *zpool)
{
- pr_debug("destroying pool type %s\n", zpool->type);
+ pr_debug("destroying pool type %s\n", zpool->driver->type);
spin_lock(&pools_lock);
list_del(&zpool->list);
@@ -228,9 +225,9 @@ void zpool_destroy_pool(struct zpool *zpool)
*
* Returns: The type of zpool.
*/
-char *zpool_get_type(struct zpool *zpool)
+const char *zpool_get_type(struct zpool *zpool)
{
- return zpool->type;
+ return zpool->driver->type;
}
/**
--
2.1.0
Add ZSWAP_MAX_KPARAM_NAME define and change the "zpool" and "compressor"
kparams maxlen to use the define. Update the param set function to
use a char[ZSWAP_MAX_KPARAM_NAME] instead of a variable-sized char[].
The kbuild test robot reported:
>> mm/zswap.c:759:1: warning: '__zswap_param_set' uses dynamic stack
>> allocation
which was a variable-sized char[] allocation on the stack:
char str[kp->str->maxlen], *s;
This technically was ok, as there are only 2 possible kparams sent to
this function, and both of them have their maxlen set low (to 32 or 64),
but this patch simplifies and clarifies things by creating a single
define to use for the kparam maxlen and the size of the stack char[].
Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Dan Streetman <[email protected]>
---
mm/zswap.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/mm/zswap.c b/mm/zswap.c
index 4043df7..d74872e 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -80,12 +80,15 @@ static u64 zswap_duplicate_entry;
static bool zswap_enabled;
module_param_named(enabled, zswap_enabled, bool, 0644);
+/* This should be >= CRYPTO_MAX_ALG_NAME and ZPOOL_MAX_TYPE_NAME */
+#define ZSWAP_MAX_KPARAM_NAME 64
+
/* Crypto compressor to use */
#define ZSWAP_COMPRESSOR_DEFAULT "lzo"
-static char zswap_compressor[CRYPTO_MAX_ALG_NAME] = ZSWAP_COMPRESSOR_DEFAULT;
+static char zswap_compressor[ZSWAP_MAX_KPARAM_NAME] = ZSWAP_COMPRESSOR_DEFAULT;
static struct kparam_string zswap_compressor_kparam = {
.string = zswap_compressor,
- .maxlen = sizeof(zswap_compressor),
+ .maxlen = ZSWAP_MAX_KPARAM_NAME,
};
static int zswap_compressor_param_set(const char *,
const struct kernel_param *);
@@ -98,10 +101,10 @@ module_param_cb(compressor, &zswap_compressor_param_ops,
/* Compressed storage zpool to use */
#define ZSWAP_ZPOOL_DEFAULT "zbud"
-static char zswap_zpool_type[32 /* arbitrary */] = ZSWAP_ZPOOL_DEFAULT;
+static char zswap_zpool_type[ZSWAP_MAX_KPARAM_NAME] = ZSWAP_ZPOOL_DEFAULT;
static struct kparam_string zswap_zpool_kparam = {
.string = zswap_zpool_type,
- .maxlen = sizeof(zswap_zpool_type),
+ .maxlen = ZSWAP_MAX_KPARAM_NAME,
};
static int zswap_zpool_param_set(const char *, const struct kernel_param *);
static struct kernel_param_ops zswap_zpool_param_ops = {
@@ -688,14 +691,12 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
char *type, char *compressor)
{
struct zswap_pool *pool, *put_pool = NULL;
- char str[kp->str->maxlen], *s;
+ char str[ZSWAP_MAX_KPARAM_NAME], *s;
int ret;
- /*
- * kp is either zswap_zpool_kparam or zswap_compressor_kparam, defined
- * at the top of this file, so maxlen is CRYPTO_MAX_ALG_NAME (64) or
- * 32 (arbitrary).
- */
+ if (WARN_ON(kp->str->maxlen > ZSWAP_MAX_KPARAM_NAME))
+ return -EINVAL;
+
strlcpy(str, val, kp->str->maxlen);
s = strim(str);
@@ -1228,6 +1229,9 @@ static int __init init_zswap(void)
{
struct zswap_pool *pool;
+ BUILD_BUG_ON(ZSWAP_MAX_KPARAM_NAME < CRYPTO_MAX_ALG_NAME);
+ BUILD_BUG_ON(ZSWAP_MAX_KPARAM_NAME < ZPOOL_MAX_TYPE_NAME);
+
zswap_init_started = true;
if (zswap_entry_cache_create()) {
--
2.1.0
On Tue, 18 Aug 2015 16:06:00 -0400 Dan Streetman <[email protected]> wrote:
> Add ZPOOL_MAX_TYPE_NAME define, and change zpool_driver *type field to
> type[ZPOOL_MAX_TYPE_NAME]. Remove redundant type field from struct zpool
> and use zpool->driver->type instead.
>
> The define will be used by zswap for its zpool param type name length.
>
Patchset is fugly. All this putzing around with fixed-length strings,
worrying about overflow and is-it-null-terminated-or-isnt-it. Shudder.
It's much better to use variable-length strings everywhere. We're not
operating in contexts which can't use kmalloc, we're not
performance-intensive and these strings aren't being written to
fixed-size fields on disk or anything. Why do we need any fixed-length
strings?
IOW, why not just replace that alloca with a kstrdup()?
> --- a/include/linux/zpool.h
> +++ b/include/linux/zpool.h
>
> ...
>
> @@ -79,7 +77,7 @@ static struct zpool_driver *zpool_get_driver(char *type)
>
> spin_lock(&drivers_lock);
> list_for_each_entry(driver, &drivers_head, list) {
> - if (!strcmp(driver->type, type)) {
> + if (!strncmp(driver->type, type, ZPOOL_MAX_TYPE_NAME)) {
Why strncmp? Please tell me these strings are always null-terminated.
On Tue, Aug 18, 2015 at 6:38 PM, Andrew Morton
<[email protected]> wrote:
> On Tue, 18 Aug 2015 16:06:00 -0400 Dan Streetman <[email protected]> wrote:
>
>> Add ZPOOL_MAX_TYPE_NAME define, and change zpool_driver *type field to
>> type[ZPOOL_MAX_TYPE_NAME]. Remove redundant type field from struct zpool
>> and use zpool->driver->type instead.
>>
>> The define will be used by zswap for its zpool param type name length.
>>
>
> Patchset is fugly. All this putzing around with fixed-length strings,
> worrying about overflow and is-it-null-terminated-or-isnt-it. Shudder.
>
> It's much better to use variable-length strings everywhere. We're not
> operating in contexts which can't use kmalloc, we're not
> performance-intensive and these strings aren't being written to
> fixed-size fields on disk or anything. Why do we need any fixed-length
> strings?
>
> IOW, why not just replace that alloca with a kstrdup()?
for the zpool drivers (zbud and zsmalloc), the type is actually just
statically assigned, e.g. .type = "zbud", so you're right the *type is
better than type[]. I'll update it.
>
>> --- a/include/linux/zpool.h
>> +++ b/include/linux/zpool.h
>>
>> ...
>>
>> @@ -79,7 +77,7 @@ static struct zpool_driver *zpool_get_driver(char *type)
>>
>> spin_lock(&drivers_lock);
>> list_for_each_entry(driver, &drivers_head, list) {
>> - if (!strcmp(driver->type, type)) {
>> + if (!strncmp(driver->type, type, ZPOOL_MAX_TYPE_NAME)) {
>
> Why strncmp? Please tell me these strings are always null-terminated.
Yep, you're right. The driver->type always is, and the type param is
passed in from sysfs, which we can rely on to be null-terminated.
>
>