[PATCH 0/3] Fix zswap init failure behavior

[PATCH 0/3] Fix zswap init failure behavior

[Dan Streetman]

If zswap fails to initialize itself at boot, it returns error from its
init function; but for built-in drivers, that does not unload them; and
more importantly, it doesn't prevent their sysfs module param interface
from being created.  In this case, changing the compressor or zpool param
will result in a WARNING because zswap didn't expect them to be changed if
initialization failed.

These patches fix that assumption, as well as allowing pool creation after
a failed initialization, if only the zpool and/or compressor creation
failed.

Dan Streetman (3):
  zswap: disable changing params if init fails
  zswap: allow initialization at boot without pool
  zswap: clear compressor or zpool param if invalid at init

 mm/zswap.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 100 insertions(+), 25 deletions(-)

-- 
2.9.3

--
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 1/3] zswap: disable changing params if init fails

[Dan Streetman]

Add zswap_init_failed bool that prevents changing any of the module
params, if init_zswap() fails, and set zswap_enabled to false.  Change
'enabled' param to a callback, and check zswap_init_failed before
allowing any change to 'enabled', 'zpool', or 'compressor' params.

Any driver that is built-in to the kernel will not be unloaded if its
init function returns error, and its module params remain accessible for
users to change via sysfs.  Since zswap uses param callbacks, which
assume that zswap has been initialized, changing the zswap params after
a failed initialization will result in WARNING due to the param callbacks
expecting a pool to already exist.  This prevents that by immediately
exiting any of the param callbacks if initialization failed.

This was reported here:
https://marc.info/?l=linux-mm&m=147004228125528&w=4

And fixes this WARNING:
[  429.723476] WARNING: CPU: 0 PID: 5140 at mm/zswap.c:503
__zswap_pool_current+0x56/0x60

Fixes: 90b0fc26d5db ("zswap: change zpool/compressor at runtime")
Cc: stable@vger.kernel.org
Signed-off-by: Dan Streetman <dan.streetman@canonical.com>
---
 mm/zswap.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 067a0d6..cabf09e 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -78,7 +78,13 @@ static u64 zswap_duplicate_entry;
 
 /* Enable/disable zswap (disabled by default) */
 static bool zswap_enabled;
-module_param_named(enabled, zswap_enabled, bool, 0644);
+static int zswap_enabled_param_set(const char *,
+				   const struct kernel_param *);
+static struct kernel_param_ops zswap_enabled_param_ops = {
+	.set =		zswap_enabled_param_set,
+	.get =		param_get_bool,
+};
+module_param_cb(enabled, &zswap_enabled_param_ops, &zswap_enabled, 0644);
 
 /* Crypto compressor to use */
 #define ZSWAP_COMPRESSOR_DEFAULT "lzo"
@@ -176,6 +182,9 @@ static atomic_t zswap_pools_count = ATOMIC_INIT(0);
 /* used by param callback function */
 static bool zswap_init_started;
 
+/* fatal error during init */
+static bool zswap_init_failed;
+
 /*********************************
 * helpers and fwd declarations
 **********************************/
@@ -624,6 +633,11 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
 	char *s = strstrip((char *)val);
 	int ret;
 
+	if (zswap_init_failed) {
+		pr_err("can't set param, initialization failed\n");
+		return -ENODEV;
+	}
+
 	/* no change required */
 	if (!strcmp(s, *(char **)kp->arg))
 		return 0;
@@ -703,6 +717,17 @@ static int zswap_zpool_param_set(const char *val,
 	return __zswap_param_set(val, kp, NULL, zswap_compressor);
 }
 
+static int zswap_enabled_param_set(const char *val,
+				   const struct kernel_param *kp)
+{
+	if (zswap_init_failed) {
+		pr_err("can't enable, initialization failed\n");
+		return -ENODEV;
+	}
+
+	return param_set_bool(val, kp);
+}
+
 /*********************************
 * writeback code
 **********************************/
@@ -1201,6 +1226,9 @@ static int __init init_zswap(void)
 dstmem_fail:
 	zswap_entry_cache_destroy();
 cache_fail:
+	/* if built-in, we aren't unloaded on failure; don't allow use */
+	zswap_init_failed = true;
+	zswap_enabled = false;
 	return -ENOMEM;
 }
 /* must be late so crypto has time to come up */
-- 
2.9.3

--
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/3] zswap: disable changing params if init fails

[Andrew Morton]

On Tue, 24 Jan 2017 15:02:57 -0500 Dan Streetman <ddstreet@ieee.org> wrote:

> Add zswap_init_failed bool that prevents changing any of the module
> params, if init_zswap() fails, and set zswap_enabled to false.  Change
> 'enabled' param to a callback, and check zswap_init_failed before
> allowing any change to 'enabled', 'zpool', or 'compressor' params.
> 
> Any driver that is built-in to the kernel will not be unloaded if its
> init function returns error, and its module params remain accessible for
> users to change via sysfs.  Since zswap uses param callbacks, which
> assume that zswap has been initialized, changing the zswap params after
> a failed initialization will result in WARNING due to the param callbacks
> expecting a pool to already exist.  This prevents that by immediately
> exiting any of the param callbacks if initialization failed.
> 
> This was reported here:
> https://marc.info/?l=linux-mm&m=147004228125528&w=4

I added Marcin's reportde-by to the changelog.

> And fixes this WARNING:
> [  429.723476] WARNING: CPU: 0 PID: 5140 at mm/zswap.c:503
> __zswap_pool_current+0x56/0x60
> 
> Fixes: 90b0fc26d5db ("zswap: change zpool/compressor at runtime")
> Cc: stable@vger.kernel.org

Is this really serious enough to justify a -stable backport?  It's just
a bit of extra noise associated with an initialization problem which
the user will be fixing anyway.



--
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/3] zswap: disable changing params if init fails

[Dan Streetman]

On Tue, Jan 24, 2017 at 4:24 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 24 Jan 2017 15:02:57 -0500 Dan Streetman <ddstreet@ieee.org> wrote:
>
>> Add zswap_init_failed bool that prevents changing any of the module
>> params, if init_zswap() fails, and set zswap_enabled to false.  Change
>> 'enabled' param to a callback, and check zswap_init_failed before
>> allowing any change to 'enabled', 'zpool', or 'compressor' params.
>>
>> Any driver that is built-in to the kernel will not be unloaded if its
>> init function returns error, and its module params remain accessible for
>> users to change via sysfs.  Since zswap uses param callbacks, which
>> assume that zswap has been initialized, changing the zswap params after
>> a failed initialization will result in WARNING due to the param callbacks
>> expecting a pool to already exist.  This prevents that by immediately
>> exiting any of the param callbacks if initialization failed.
>>
>> This was reported here:
>> https://marc.info/?l=linux-mm&m=147004228125528&w=4
>
> I added Marcin's reportde-by to the changelog.

Thanks, I missed that.

>
>> And fixes this WARNING:
>> [  429.723476] WARNING: CPU: 0 PID: 5140 at mm/zswap.c:503
>> __zswap_pool_current+0x56/0x60
>>
>> Fixes: 90b0fc26d5db ("zswap: change zpool/compressor at runtime")
>> Cc: stable@vger.kernel.org
>
> Is this really serious enough to justify a -stable backport?  It's just
> a bit of extra noise associated with an initialization problem which
> the user will be fixing anyway.

The warning is just noise, and not serious.  However, when init fails,
zswap frees all its percpu dstmem pages and its kmem cache.  The kmem
cache might be serious, if kmem_cache_alloc(NULL, gfp) has problems;
but the percpu dstmem pages are definitely a problem, as they're used
as temporary buffer for compressed pages before copying into place in
the zpool.

If the user does get zswap enabled after an init failure, then zswap
will likely Oops on the first page it tries to compress (or worse,
start corrupting memory).

I should have added all that to the changelog to make the issue clear, sorry.

>
>
>
> --
> 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>
>

--
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/3] zswap: allow initialization at boot without pool

[Dan Streetman]

Allow zswap to initialize at boot even if it can't create its pool
due to a failure to create a zpool and/or compressor.  Allow those
to be created later, from the sysfs module param interface.

Signed-off-by: Dan Streetman <dan.streetman@canonical.com>
---
 mm/zswap.c | 46 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index cabf09e..77cb847 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -185,6 +185,9 @@ static bool zswap_init_started;
 /* fatal error during init */
 static bool zswap_init_failed;
 
+/* init completed, but couldn't create the initial pool */
+static bool zswap_has_pool;
+
 /*********************************
 * helpers and fwd declarations
 **********************************/
@@ -424,7 +427,8 @@ static struct zswap_pool *__zswap_pool_current(void)
 	struct zswap_pool *pool;
 
 	pool = list_first_or_null_rcu(&zswap_pools, typeof(*pool), list);
-	WARN_ON(!pool);
+	WARN_ONCE(!pool && zswap_has_pool,
+		  "%s: no page storage pool!\n", __func__);
 
 	return pool;
 }
@@ -443,7 +447,7 @@ static struct zswap_pool *zswap_pool_current_get(void)
 	rcu_read_lock();
 
 	pool = __zswap_pool_current();
-	if (!pool || !zswap_pool_get(pool))
+	if (!zswap_pool_get(pool))
 		pool = NULL;
 
 	rcu_read_unlock();
@@ -459,7 +463,9 @@ static struct zswap_pool *zswap_pool_last_get(void)
 
 	list_for_each_entry_rcu(pool, &zswap_pools, list)
 		last = pool;
-	if (!WARN_ON(!last) && !zswap_pool_get(last))
+	WARN_ONCE(!last && zswap_has_pool,
+		  "%s: no page storage pool!\n", __func__);
+	if (!zswap_pool_get(last))
 		last = NULL;
 
 	rcu_read_unlock();
@@ -582,6 +588,9 @@ static void zswap_pool_destroy(struct zswap_pool *pool)
 
 static int __must_check zswap_pool_get(struct zswap_pool *pool)
 {
+	if (!pool)
+		return 0;
+
 	return kref_get_unless_zero(&pool->kref);
 }
 
@@ -639,7 +648,7 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
 	}
 
 	/* no change required */
-	if (!strcmp(s, *(char **)kp->arg))
+	if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
 		return 0;
 
 	/* if this is load-time (pre-init) param setting,
@@ -685,6 +694,7 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
 	if (!ret) {
 		put_pool = zswap_pool_current();
 		list_add_rcu(&pool->list, &zswap_pools);
+		zswap_has_pool = true;
 	} else if (pool) {
 		/* add the possibly pre-existing pool to the end of the pools
 		 * list; if it's new (and empty) then it'll be removed and
@@ -692,6 +702,15 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
 		 */
 		list_add_tail_rcu(&pool->list, &zswap_pools);
 		put_pool = pool;
+	} else if (!zswap_has_pool) {
+		/* if initial pool creation failed, and this pool creation also
+		 * failed, maybe both compressor and zpool params were bad.
+		 * Allow changing this param, so pool creation will succeed
+		 * when the other param is changed. We already verified this
+		 * param is ok in the zpool_has_pool() or crypto_has_comp()
+		 * checks above.
+		 */
+		ret = param_set_charp(s, kp);
 	}
 
 	spin_unlock(&zswap_pools_lock);
@@ -724,6 +743,10 @@ static int zswap_enabled_param_set(const char *val,
 		pr_err("can't enable, initialization failed\n");
 		return -ENODEV;
 	}
+	if (!zswap_has_pool && zswap_init_started) {
+		pr_err("can't enable, no pool configured\n");
+		return -ENODEV;
+	}
 
 	return param_set_bool(val, kp);
 }
@@ -1205,22 +1228,21 @@ static int __init init_zswap(void)
 		goto hp_fail;
 
 	pool = __zswap_pool_create_fallback();
-	if (!pool) {
+	if (pool) {
+		pr_info("loaded using pool %s/%s\n", pool->tfm_name,
+			zpool_get_type(pool->zpool));
+		list_add(&pool->list, &zswap_pools);
+		zswap_has_pool = true;
+	} else {
 		pr_err("pool creation failed\n");
-		goto pool_fail;
+		zswap_enabled = false;
 	}
-	pr_info("loaded using pool %s/%s\n", pool->tfm_name,
-		zpool_get_type(pool->zpool));
-
-	list_add(&pool->list, &zswap_pools);
 
 	frontswap_register_ops(&zswap_frontswap_ops);
 	if (zswap_debugfs_init())
 		pr_warn("debugfs initialization failed\n");
 	return 0;
 
-pool_fail:
-	cpuhp_remove_state_nocalls(CPUHP_MM_ZSWP_POOL_PREPARE);
 hp_fail:
 	cpuhp_remove_state(CPUHP_MM_ZSWP_MEM_PREPARE);
 dstmem_fail:
-- 
2.9.3

--
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 2/3] zswap: allow initialization at boot without pool

[Sergey Senozhatsky]

just a note,

On (01/24/17 15:02), Dan Streetman wrote:
[..]
> @@ -692,6 +702,15 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
>  		 */
>  		list_add_tail_rcu(&pool->list, &zswap_pools);
>  		put_pool = pool;
> +	} else if (!zswap_has_pool) {
> +		/* if initial pool creation failed, and this pool creation also
> +		 * failed, maybe both compressor and zpool params were bad.
> +		 * Allow changing this param, so pool creation will succeed
> +		 * when the other param is changed. We already verified this
> +		 * param is ok in the zpool_has_pool() or crypto_has_comp()
> +		 * checks above.
> +		 */
> +		ret = param_set_charp(s, kp);
>  	}
>  
>  	spin_unlock(&zswap_pools_lock);

looks like there still GFP_KERNEL allocation from atomic section:
param_set_charp()->kmalloc_parameter()->kmalloc(GFP_KERNEL), under
`zswap_pools_lock'.

	-ss

--
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 2/3] zswap: allow initialization at boot without pool

[Dan Streetman]

On Tue, Jan 24, 2017 at 7:24 PM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> just a note,
>
> On (01/24/17 15:02), Dan Streetman wrote:
> [..]
>> @@ -692,6 +702,15 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
>>                */
>>               list_add_tail_rcu(&pool->list, &zswap_pools);
>>               put_pool = pool;
>> +     } else if (!zswap_has_pool) {
>> +             /* if initial pool creation failed, and this pool creation also
>> +              * failed, maybe both compressor and zpool params were bad.
>> +              * Allow changing this param, so pool creation will succeed
>> +              * when the other param is changed. We already verified this
>> +              * param is ok in the zpool_has_pool() or crypto_has_comp()
>> +              * checks above.
>> +              */
>> +             ret = param_set_charp(s, kp);
>>       }
>>
>>       spin_unlock(&zswap_pools_lock);
>
> looks like there still GFP_KERNEL allocation from atomic section:
> param_set_charp()->kmalloc_parameter()->kmalloc(GFP_KERNEL), under
> `zswap_pools_lock'.

thanks, it looks like the other param_set_charp above this new one has
been in the spinlock ever since i added the param callback.  I'll send
a patch.


>
>         -ss
>

--
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 3/3] zswap: clear compressor or zpool param if invalid at init

[Dan Streetman]

If either the compressor and/or zpool param are invalid at boot, and
their default value is also invalid, set the param to the empty string
to indicate there is no compressor and/or zpool configured.  This allows
users to check the sysfs interface to see which param needs changing.

Signed-off-by: Dan Streetman <dan.streetman@canonical.com>
---
 mm/zswap.c | 49 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 12 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 77cb847..9e8565d 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -76,6 +76,8 @@ static u64 zswap_duplicate_entry;
 * tunables
 **********************************/
 
+#define ZSWAP_PARAM_UNSET ""
+
 /* Enable/disable zswap (disabled by default) */
 static bool zswap_enabled;
 static int zswap_enabled_param_set(const char *,
@@ -501,6 +503,17 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
 	gfp_t gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
 	int ret;
 
+	if (!zswap_has_pool) {
+		/* if either are unset, pool initialization failed, and we
+		 * need both params to be set correctly before trying to
+		 * create a pool.
+		 */
+		if (!strcmp(type, ZSWAP_PARAM_UNSET))
+			return NULL;
+		if (!strcmp(compressor, ZSWAP_PARAM_UNSET))
+			return NULL;
+	}
+
 	pool = kzalloc(sizeof(*pool), GFP_KERNEL);
 	if (!pool) {
 		pr_err("pool alloc failed\n");
@@ -550,28 +563,40 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
 
 static __init struct zswap_pool *__zswap_pool_create_fallback(void)
 {
-	if (!crypto_has_comp(zswap_compressor, 0, 0)) {
-		if (!strcmp(zswap_compressor, ZSWAP_COMPRESSOR_DEFAULT)) {
-			pr_err("default compressor %s not available\n",
-			       zswap_compressor);
-			return NULL;
-		}
+	bool has_comp, has_zpool;
+
+	has_comp = crypto_has_comp(zswap_compressor, 0, 0);
+	if (!has_comp && strcmp(zswap_compressor, ZSWAP_COMPRESSOR_DEFAULT)) {
 		pr_err("compressor %s not available, using default %s\n",
 		       zswap_compressor, ZSWAP_COMPRESSOR_DEFAULT);
 		param_free_charp(&zswap_compressor);
 		zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
+		has_comp = crypto_has_comp(zswap_compressor, 0, 0);
 	}
-	if (!zpool_has_pool(zswap_zpool_type)) {
-		if (!strcmp(zswap_zpool_type, ZSWAP_ZPOOL_DEFAULT)) {
-			pr_err("default zpool %s not available\n",
-			       zswap_zpool_type);
-			return NULL;
-		}
+	if (!has_comp) {
+		pr_err("default compressor %s not available\n",
+		       zswap_compressor);
+		param_free_charp(&zswap_compressor);
+		zswap_compressor = ZSWAP_PARAM_UNSET;
+	}
+
+	has_zpool = zpool_has_pool(zswap_zpool_type);
+	if (!has_zpool && strcmp(zswap_zpool_type, ZSWAP_ZPOOL_DEFAULT)) {
 		pr_err("zpool %s not available, using default %s\n",
 		       zswap_zpool_type, ZSWAP_ZPOOL_DEFAULT);
 		param_free_charp(&zswap_zpool_type);
 		zswap_zpool_type = ZSWAP_ZPOOL_DEFAULT;
+		has_zpool = zpool_has_pool(zswap_zpool_type);
 	}
+	if (!has_zpool) {
+		pr_err("default zpool %s not available\n",
+		       zswap_zpool_type);
+		param_free_charp(&zswap_zpool_type);
+		zswap_zpool_type = ZSWAP_PARAM_UNSET;
+	}
+
+	if (!has_comp || !has_zpool)
+		return NULL;
 
 	return zswap_pool_create(zswap_zpool_type, zswap_compressor);
 }
-- 
2.9.3

--
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>