Re: [PATCH v2 05/11] zram: check mem_limit before allocating zsmalloc object

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] zram: uncached double read of limit_pages in
  zram_can_store_page()
--

commit 29ae8f44e0ebe64efb9439cb2970653d6d8c9390
Author: Sergey Senozhatsky <senozhatsky@chromium.org>

zram: check mem_limit before allocating zsmalloc object

This commit moves the zram_can_store_page() memory limit check ahead of
page compression and zsmalloc object allocation in zram_write_page(), and
updates max_used_pages separately after zs_malloc() succeeds.

> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 2c2e11b2b6638..d38b20a225954 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -177,8 +177,9 @@ static void mark_slot_accessed(struct zram *zram, unsigned long index)
>  #endif
>  }
>  
> -static inline void update_used_max(struct zram *zram, const unsigned long pages)
> +static inline void update_used_max(struct zram *zram)
>  {
> +	unsigned long pages = zs_get_total_pages(zram->mem_pool);
>  	unsigned long cur_max = atomic_long_read(&zram->stats.max_used_pages);
>  
>  	do {
> @@ -190,12 +191,10 @@ static inline void update_used_max(struct zram *zram, const unsigned long pages)
>  
>  static bool zram_can_store_page(struct zram *zram)
>  {
> -	unsigned long alloced_pages;
> -
> -	alloced_pages = zs_get_total_pages(zram->mem_pool);
> -	update_used_max(zram, alloced_pages);
> +	if (!zram->limit_pages)
> +		return true;
>  
> -	return !zram->limit_pages || alloced_pages <= zram->limit_pages;
> +	return zs_get_total_pages(zram->mem_pool) <= zram->limit_pages;
>  }

[Severity: Low]
Can zram_can_store_page() return false when mem_limit is cleared to 0 at
runtime?

The write path does not hold zram->dev_lock, while mem_limit_store()
updates zram->limit_pages under it.  Because zs_get_total_pages() is an
external call, the compiler reloads zram->limit_pages afterwards:

CPU0                              CPU1
zram_write_page()
  zram_can_store_page()
    if (!zram->limit_pages)
      /* sees non-zero limit */
    zs_get_total_pages(...)
                                  mem_limit_store()
                                    zram->limit_pages = 0;
    return pages <= zram->limit_pages;
      /* pages <= 0 is false */

When at least one page is allocated in zram->mem_pool, disabling the limit
causes an in-flight write that was within the previous limit to fail with
-ENOMEM.  Reading zram->limit_pages once into a local variable with
READ_ONCE() avoids the second load.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110054836.1515030-1-senozhatsky@chromium.org?part=5