Re: [PATCH v2 2/5] zram: make dict update in comp_params_store() atomic

[Sergey Senozhatsky <senozhatsky@chromium.org>]

On (26/07/28 17:29), Haoqin Huang wrote:
> comp_params_store() resets old parameters before reading a new dict,
> so if kernel_read_file_from_path() fails the params are left broken
> and the actual error is swallowed. Fix by reading into a temporary
> buffer first, swapping only on success. Use sz <= 0 to also reject
> zero-size dicts.
> 
> Signed-off-by: Haoqin Huang <haoqinhuang@tencent.com>
> Signed-off-by: Rongwei Wang <zigiwang@tencent.com>
> ---
>  drivers/block/zram/zram_drv.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index ace65c586072..9ea7ba9d1ed0 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -1699,21 +1699,23 @@ static int comp_params_store(struct zram *zram, u32 prio, s32 level,
>  			     const char *dict_path,
>  			     struct deflate_params *deflate_params)
>  {
> +	void *new_dict = NULL;
>  	ssize_t sz = 0;
>  
> -	comp_params_reset(zram, prio);

I don't see why is that a problem.  All you wanted to do here is to
handle zero i_size.  Why do we need dict setting to be atomic?

>  	if (dict_path) {
> -		sz = kernel_read_file_from_path(dict_path, 0,
> -						&zram->params[prio].dict,
> -						INT_MAX,
> -						NULL,
> -						READING_POLICY);
> -		if (sz < 0)
> -			return -EINVAL;
> +		sz = kernel_read_file_from_path(dict_path, 0, &new_dict,
> +						INT_MAX, NULL, READING_POLICY);
> +		if (sz <= 0) {
> +			vfree(new_dict);
> +			if (sz == 0)
> +				return -EINVAL;
> +			return sz;
> +		}
>  	}
>  
> +	comp_params_reset(zram, prio);
>  	zram->params[prio].dict_sz = sz;
> +	zram->params[prio].dict = new_dict;
>  	zram->params[prio].level = level;
>  	zram->params[prio].deflate.winbits = deflate_params->winbits;