Re: [PATCH 4/7] zram: allocate zsmalloc object under slot lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] zram: data loss on write failure after slot_free() in zram_write_page()
--

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

zram: allocate zsmalloc object under slot lock

This commit moves slot_lock() and slot_free() before zs_malloc() in
write_incompressible_page() and zram_write_page() so that the old slot is
freed under the slot lock prior to allocating a new zsmalloc object.

> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 2c2e11b2b6638..e2710ce35b1f5 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -2260,14 +2260,19 @@ static int write_incompressible_page(struct zram *zram, struct page *page,
>  	struct zs_handle *handle;
>  	void *src;
>  
> +	slot_lock(zram, index);
> +	slot_free(zram, index);
>  	handle = zs_malloc(zram->mem_pool, PAGE_SIZE,
>  			   GFP_NOIO | __GFP_NOWARN |
>  			   __GFP_HIGHMEM | __GFP_MOVABLE, page_to_nid(page));
> -	if (IS_ERR(handle))
> +	if (IS_ERR(handle)) {
> +		slot_unlock(zram, index);
>  		return PTR_ERR(handle);
> +	}
>  
>  	if (!zram_can_store_page(zram)) {
>  		zs_free(zram->mem_pool, handle);
> +		slot_unlock(zram, index);
>  		return -ENOMEM;
>  	}
>  
> @@ -2275,8 +2280,6 @@ static int write_incompressible_page(struct zram *zram, struct page *page,
>  	zs_obj_write(zram->mem_pool, handle, src, PAGE_SIZE);
>  	kunmap_local(src);
>  
> -	slot_lock(zram, index);
> -	slot_free(zram, index);
>  	set_slot_flag(zram, index, ZRAM_HUGE);
>  	zram->table[index].handle = handle;
>  	set_slot_size(zram, index, PAGE_SIZE);
> @@ -2325,25 +2328,27 @@ static int zram_write_page(struct zram *zram, struct page *page,
>  		return write_incompressible_page(zram, page, index);
>  	}
>  
> +	slot_lock(zram, index);
> +	slot_free(zram, index);
>  	handle = zs_malloc(zram->mem_pool, comp_len,
>  			   GFP_NOIO | __GFP_NOWARN |
>  			   __GFP_HIGHMEM | __GFP_MOVABLE, page_to_nid(page));
>  	if (IS_ERR(handle)) {
>  		zcomp_cstrm_put(zstrm);
> +		slot_unlock(zram, index);
>  		return PTR_ERR(handle);
>  	}
>  
>  	if (!zram_can_store_page(zram)) {
>  		zcomp_cstrm_put(zstrm);
>  		zs_free(zram->mem_pool, handle);
> +		slot_unlock(zram, index);
>  		return -ENOMEM;
>  	}

[Severity: Critical]
Does calling slot_free() before zs_malloc() and zram_can_store_page() in
write_incompressible_page() and zram_write_page() cause existing data in the
slot to be lost if the allocation or memory limit check fails?

Previously, both functions allocated and populated the new handle before
calling slot_free(zram, index), so a failed write left the old slot contents
intact. With slot_free() called first, the existing handle (or backing device
block when ZRAM_WB is set, or ZRAM_SAME entry) is freed and
zram->table[index].element is reset to 0 before the new allocation is
attempted.

If zs_malloc() fails under GFP_NOIO memory pressure or zram_can_store_page()
returns false because zram->limit_pages is exceeded, the write returns an
error while leaving the slot empty. Subsequent reads via read_from_zspool()
then see !zram->table[index].handle:

drivers/block/zram/zram_drv.c:read_from_zspool() {
	if (test_slot_flag(zram, index, ZRAM_SAME) ||
	    !zram->table[index].handle)
		return read_same_filled_page(zram, page, index);
}

and silently return a zero-filled page instead of the previous data.

Additionally, on systems with PAGE_SIZE > 4096, a single slot holds multiple
4KB logical blocks, and partial writes go through zram_bvec_write_partial():

drivers/block/zram/zram_drv.c:zram_bvec_write_partial() {
	ret = zram_read_page(zram, page, index, NULL);
	if (!ret) {
		memcpy_from_bvec(page_address(page) + offset, bvec);
		ret = zram_write_page(zram, page, index);
	}
}

If zram_write_page() fails after slot_free() here, wouldn't that also wipe
out all the other unmodified 4KB logical blocks sharing the same PAGE_SIZE
slot?

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