[PATCH 1/2] zpool: define and use max type length

[PATCH 1/2] zpool: define and use max type length

[Dan Streetman]

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 <ddstreet@ieee.org>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] zswap: use const max length for kparam names

[Dan Streetman]

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 <fengguang.wu@intel.com>
Signed-off-by: Dan Streetman <ddstreet@ieee.org>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] zpool: define and use max type length

[Andrew Morton]

On Tue, 18 Aug 2015 16:06:00 -0400 Dan Streetman <ddstreet@ieee.org> 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.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] zpool: define and use max type length

[Dan Streetman]

On Tue, Aug 18, 2015 at 6:38 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 18 Aug 2015 16:06:00 -0400 Dan Streetman <ddstreet@ieee.org> 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.

>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>