Re: [PATCH v2] zram: add restriction on dynamic zram device creation

[Minchan Kim]

Hi Yi,

On Fri, Sep 04, 2020 at 04:52:10PM +0800, Yi Wang wrote:
> From: zhanglin <zhang.lin16@xxxxxxxxxx>
> 
> Add max_num_devices to limit dynamic zram device creation to prevent
>  potential OOM
> 
> Signed-off-by: zhanglin <zhang.lin16@xxxxxxxxxx>
> Signed-off-by: Yi Wang <wang.yi59@xxxxxxxxxx>
> ---
> v1->v2:
> change hard-coded initial max_num_devices into configurable way.
> 
>  drivers/block/zram/Kconfig    |  7 +++++++
>  drivers/block/zram/zram_drv.c | 28 +++++++++++++++++++++-------
>  2 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> index fe7a4b7d30cf..54a369932417 100644
> --- a/drivers/block/zram/Kconfig
> +++ b/drivers/block/zram/Kconfig
> @@ -37,3 +37,10 @@ config ZRAM_MEMORY_TRACKING
>  	  /sys/kernel/debug/zram/zramX/block_state.
>  
>  	  See Documentation/admin-guide/blockdev/zram.rst for more information.
> +
> +config ZRAM_DEV_MAX_COUNT
> +	int "Number of zram devices to be created"
> +	depends on ZRAM
> +	default 256
> +	help
> +	  This option specifies the maximum number of zram devices.
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 36d49159140f..d1022f3c04c4 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -43,8 +43,9 @@ static DEFINE_MUTEX(zram_index_mutex);
>  static int zram_major;
>  static const char *default_compressor = "lzo-rle";
>  
> -/* Module params (documentation at end) */
>  static unsigned int num_devices = 1;
> +/* Module params (documentation at end) */
> +static unsigned int max_num_devices = CONFIG_ZRAM_DEV_MAX_COUNT;
>  /*
>   * Pages that compress to sizes equals or greater than this are stored
>   * uncompressed in memory.
> @@ -2013,10 +2014,16 @@ static ssize_t hot_add_show(struct class *class,
>  			struct class_attribute *attr,
>  			char *buf)
>  {
> -	int ret;
> +	int ret = -ENOSPC;
>  
>  	mutex_lock(&zram_index_mutex);
> +	if (num_devices >= max_num_devices) {
> +		mutex_unlock(&zram_index_mutex);
> +		return ret;
> +	}
>  	ret = zram_add();

> +	if (ret >= 0)
> +		num_devices += 1;

Let's have the num_devices inc/dec in zram_add/zram_remove so any caller
doesn't need to take care of it.

>  	mutex_unlock(&zram_index_mutex);
>  
>  	if (ret < 0)
> @@ -2046,8 +2053,10 @@ static ssize_t hot_remove_store(struct class *class,
>  	zram = idr_find(&zram_index_idr, dev_id);
>  	if (zram) {
>  		ret = zram_remove(zram);
> -		if (!ret)
> +		if (!ret) {
>  			idr_remove(&zram_index_idr, dev_id);
> +			num_devices -= 1;
> +		}
>  	} else {
>  		ret = -ENODEV;
>  	}
> @@ -2089,6 +2098,7 @@ static void destroy_devices(void)
>  static int __init zram_init(void)
>  {
>  	int ret;
> +	unsigned int i;
>  
>  	ret = cpuhp_setup_state_multi(CPUHP_ZCOMP_PREPARE, "block/zram:prepare",
>  				      zcomp_cpu_up_prepare, zcomp_cpu_dead);
> @@ -2111,13 +2121,17 @@ static int __init zram_init(void)
>  		return -EBUSY;
>  	}
>  
> -	while (num_devices != 0) {
> +	if (num_devices > max_num_devices) {
> +		pr_err("Number of pre-created zram devices over limit\n");
> +		goto out_error;
> +	}
> +
> +	for (i = 0; i < num_devices; i++) {
>  		mutex_lock(&zram_index_mutex);
>  		ret = zram_add();
>  		mutex_unlock(&zram_index_mutex);
>  		if (ret < 0)
>  			goto out_error;
> -		num_devices--;
>  	}
>  
>  	return 0;
> @@ -2135,8 +2149,8 @@ static void __exit zram_exit(void)
>  module_init(zram_init);
>  module_exit(zram_exit);
>  
> -module_param(num_devices, uint, 0);
> -MODULE_PARM_DESC(num_devices, "Number of pre-created zram devices");
> +module_param(max_num_devices, uint, 0);
> +MODULE_PARM_DESC(max_num_devices, "Max number of created zram devices");

How about this?
"Max number of zram devices to be created"

>  
>  MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_AUTHOR("Nitin Gupta <ngupta@xxxxxxxxxx>");
> -- 
> 2.17.1
>