Re: [PATCH] mm: zswap: set zswap_ever_enabled when enabling zswap at runtime

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm: zswap: redundant zswap_ever_enabled enablement in zswap_enabled_param_set()
--

commit 4d07b90085374936c4c6222ca58eb86f3db68b14
Author: Wenwu Hou <hwenwur@gmail.com>

mm: zswap: set zswap_ever_enabled when enabling zswap at runtime

This commit enables the zswap_ever_enabled static branch in
zswap_enabled_param_set() when zswap is enabled at runtime after the
initial pool was created via the compressor parameter.

>     mm: zswap: set zswap_ever_enabled when enabling zswap at runtime
>     
>     zswap can end up enabled without zswap_ever_enabled being set, which
>     leads to data corruption. With CONFIG_ZSWAP_DEFAULT_ON=n:
>     
>     1. zswap_setup() fails to create the initial pool. Setup still counts
>        as successful, but zswap_has_pool stays false.
>     2. The user writes a compressor name to
>        /sys/module/zswap/parameters/compressor.
>        zswap_compressor_param_set() creates a pool and sets
>        zswap_has_pool = true, but never enables the zswap_ever_enabled
>        static key.
>     3. The user runs "echo Y > /sys/module/zswap/parameters/enabled".
>     4. zswap_store() now stores pages in zswap, and swap_writeout() skips
>        writing them to the swap device.
>     5. On swap-in, zswap_load() first checks zswap_never_enabled(). It
>        returns true, so zswap_load() reports that the page is not in zswap.
>        The page is then read from the swap device, which holds stale data.
>     
>     Fix this by enabling zswap_ever_enabled in zswap_enabled_param_set()
>     before zswap_enabled becomes true.
>     
>     Fixes: 2d4d2b1cfb85 ("mm: zswap: add zswap_never_enabled()")
>     Cc: stable@vger.kernel.org
>     Signed-off-by: Wenwu Hou <hwenwur@gmail.com>
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 572075bb1c1c1..bd97652b682de 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -661,12 +661,29 @@ static int zswap_enabled_param_set(const char *val,
>  		if (zswap_setup())
>  			break;
>  		fallthrough;
> -	case ZSWAP_INIT_SUCCEED:
> -		if (!zswap_has_pool)
> +	case ZSWAP_INIT_SUCCEED: {
> +		bool enable;
> +
> +		if (!zswap_has_pool) {
>  			pr_err("can't enable, no pool configured\n");
> -		else
> -			ret = param_set_bool(val, kp);
> +			break;
> +		}
> +
> +		/*
> +		 * The initial pool may have been created later by the
> +		 * compressor param rather than by zswap_setup(), in which
> +		 * case zswap_ever_enabled is still off. Flip it before
> +		 * zswap_enabled becomes visible to zswap_store(), otherwise
> +		 * zswap_load() would skip stored entries and read stale data
> +		 * from the backing swap device. param_set_bool() treats a
> +		 * NULL value as true.
> +		 */
> +		if (!val || (!kstrtobool(val, &enable) && enable))
> +			static_branch_enable(&zswap_ever_enabled);

[Severity: Medium]
Can zswap_ever_enabled actually still be off when reaching this check in
zswap_enabled_param_set()?

Commit 91b4edb993e1 ("mm/zswap: enable zswap_ever_enabled in
zswap_pool_create()") already moved static_branch_enable(&zswap_ever_enabled)
into zswap_pool_create():

mm/zswap.c:zswap_pool_create() {
	...
	zswap_pool_debug("created", pool);

	static_branch_enable(&zswap_ever_enabled);

	return pool;
	...
}

When a user writes to /sys/module/zswap/parameters/compressor,
zswap_compressor_param_set() creates the pool via zswap_pool_create() before
setting zswap_has_pool = true:

mm/zswap.c:zswap_compressor_param_set() {
	...
	pool = zswap_pool_find_get(s);
	if (!pool) {
		pool = zswap_pool_create(s);
	}
	...
	if (!ret) {
		put_pool = rcu_access_pointer(zswap_current_pool);
		rcu_assign_pointer(zswap_current_pool, pool);
		zswap_has_pool = true;
	}
	...
}

Since zswap_has_pool is only set to true after zswap_pool_create() succeeds
(either in zswap_setup()->__zswap_pool_create_fallback()->zswap_pool_create()
or in zswap_compressor_param_set()->zswap_pool_create()), and
zswap_enabled_param_set() checks !zswap_has_pool and breaks out above, isn't
zswap_ever_enabled guaranteed to already be enabled here?

> +
> +		ret = param_set_bool(val, kp);
>  		break;
> +	}
>  	case ZSWAP_INIT_FAILED:
>  		pr_err("can't enable, initialization failed\n");
>  	}

([ ♦♦ ])

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020122942.3477541-1-hwenwur@gmail.com?part=1