zram: factor-out zram_decompress_page() function

zram: factor-out zram_decompress_page() function

zram_bvec_read() shared decompress functionality with zram_read_before_write() function. Factor-out and make commonly used zram_decompress_page() function, which also simplified error handling in zram_bvec_read().

Signed-off-by Sergey Senozhatsky <[email protected]>

---

  drivers/staging/zram/zram_drv.c | 115 +++++++++++++++++-----------------------
  1 file changed, 50 insertions(+), 65 deletions(-)

drivers/staging/zram/zram_drv.c +16 -56

index 6edefde..7585467 100644
@@ -183,62 +183,25 @@ static inline int is_partial_io(struct bio_vec *bvec)
  	return bvec->bv_len != PAGE_SIZE;
  }
 
-static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
-			  u32 index, int offset, struct bio *bio)
+static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
  {
-	int ret;
-	size_t clen;
-	struct page *page;
-	unsigned char *user_mem, *cmem, *uncmem = NULL;
-
-	page = bvec->bv_page;
-
-	if (zram_test_flag(zram, index, ZRAM_ZERO)) {
-		handle_zero_page(bvec);
-		return 0;
-	}
+	int ret = LZO_E_OK;
+	size_t clen = PAGE_SIZE;
+	unsigned char *cmem;
+	unsigned long handle = zram->table[index].handle;
 
-	/* Requested page is not present in compressed area */
-	if (unlikely(!zram->table[index].handle)) {
-		pr_debug("Read before write: sector=%lu, size=%u",
-			 (ulong)(bio->bi_sector), bio->bi_size);
-		handle_zero_page(bvec);
+	if (!handle || zram_test_flag(zram, index, ZRAM_ZERO)) {
+		memset(mem, 0, PAGE_SIZE);
  		return 0;
  	}
 
-	if (is_partial_io(bvec)) {
-		/* Use  a temporary buffer to decompress the page */
-		uncmem = kmalloc(PAGE_SIZE, GFP_KERNEL);
-		if (!uncmem) {
-			pr_info("Error allocating temp memory!\n");
-			return -ENOMEM;
-		}
-	}
-
-	user_mem = kmap_atomic(page);
-	if (!is_partial_io(bvec))
-		uncmem = user_mem;
-	clen = PAGE_SIZE;
-
-	cmem = zs_map_object(zram->mem_pool, zram->table[index].handle,
-				ZS_MM_RO);
-
-	if (zram->table[index].size == PAGE_SIZE) {
-		memcpy(uncmem, cmem, PAGE_SIZE);
-		ret = LZO_E_OK;
-	} else {
+	cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
+	if (zram->table[index].size == PAGE_SIZE)
+		memcpy(mem, cmem, PAGE_SIZE);
+	else
  		ret = lzo1x_decompress_safe(cmem, zram->table[index].size,
-				    uncmem, &clen);
-	}
-
-	if (is_partial_io(bvec)) {
-		memcpy(user_mem + bvec->bv_offset, uncmem + offset,
-		       bvec->bv_len);
-		kfree(uncmem);
-	}
-
-	zs_unmap_object(zram->mem_pool, zram->table[index].handle);
-	kunmap_atomic(user_mem);
+						mem, &clen);
+	zs_unmap_object(zram->mem_pool, handle);
 
  	/* Should NEVER happen. Return bio error if it does. */
  	if (unlikely(ret != LZO_E_OK)) {

@@ -247,36 +210,58 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
  		return ret;
  	}
 
-	flush_dcache_page(page);
-
  	return 0;
  }
 
-static int zram_read_before_write(struct zram *zram, char *mem, u32 index)
+static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
+			  u32 index, int offset, struct bio *bio)
  {
  	int ret;
-	size_t clen = PAGE_SIZE;
-	unsigned char *cmem;
-	unsigned long handle = zram->table[index].handle;
+	struct page *page;

"Read before write" message is not valid in case ZRAM_ZERO flag is set. Its true only in !handle case.

Otherwise, the patch looks good to me.

On a side note, zram still contains a known use-after-free bug reported by Fengguang Wu (CC'ed) which happens in the "partial I/O" i.e. non PAGE_SIZE'ed I/O case which is fixed by the following patch.

Please let me know if you can include the following patch when you resend this patch series, or I can do the same or will wait for this to be merged and then send it later.

======
zram: Fix use-after-free in partial I/O case

When the compressed size of a page exceeds a threshold, the page is stored as-is i.e. in uncompressed form. In the partial I/O i.e. non-PAGE_SIZE'ed I/O case, however, the uncompressed memory was being freed before it could be copied into the zsmalloc pool resulting in use-after-free bug.

Signed-off-by Nitin Gupta <[email protected]>

---

drivers/staging/zram/zram_drv.c +1 -3

index 7585467..635736b 100644
@@ -288,10 +288,8 @@ static int zram_bvec_write(struct zram *zram,
 struct bio_vec *bvec, u32 index,
 			goto out;
 		}
 		ret = zram_decompress_page(zram, uncmem, index);
-		if (ret) {
-			kfree(uncmem);
+		if (ret)
 			goto out;
-		}
 	}
 

drivers/staging/zram/zram_drv.c +0 -2

 	/*

@@ -312,8 +310,6 @@ static int zram_bvec_write(struct zram *zram,
 struct bio_vec *bvec, u32 index,
 
 	if (page_zero_filled(uncmem)) {
 		kunmap_atomic(user_mem);
-		if (is_partial_io(bvec))
-			kfree(uncmem);
 		zram_stat_inc(&zram->stats.pages_zero);
 		zram_set_flag(zram, index, ZRAM_ZERO);

drivers/staging/zram/zram_drv.c +0 -2

 		ret = 0;

@@ -324,8 +320,6 @@ static int zram_bvec_write(struct zram *zram,
 struct bio_vec *bvec, u32 index,
 			       zram->compress_workmem);
 
 	kunmap_atomic(user_mem);
-	if (is_partial_io(bvec))
-			kfree(uncmem);
 
 	if (unlikely(ret != LZO_E_OK)) {

drivers/staging/zram/zram_drv.c +5 -1

 		pr_err("Compression failed! err=%d\n", ret);

@@ -360,11 +354,15 @@ static int zram_bvec_write(struct zram *zram,
 struct bio_vec *bvec, u32 index,
 	if (clen <= PAGE_SIZE / 2)
 		zram_stat_inc(&zram->stats.good_compress);
 
-	return 0;
+	ret = 0;
 
 out:
 	if (ret)
 		zram_stat64_inc(zram, &zram->stats.failed_writes);
+
+	if (is_partial_io(bvec))
+		kfree(uncmem);
+
 	return ret;
 }

BTW, I could not trigger this partial I/O case, so please let me know if you hit any issue during your testing.

There is another sparse warning to be fixed: zram_reset_device should be static.

Thanks,
Nitin

zram: factor-out zram_decompress_page() function

zram_bvec_read() shared decompress functionality with zram_read_before_write() function. Factor-out and make commonly used zram_decompress_page() function, which also simplified error handling in zram_bvec_read().

V2: changed debug message for handle_zero_page() case to a more general one

Signed-off-by Sergey Senozhatsky <[email protected]>

---

 drivers/staging/zram/zram_drv.c | 115 +++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 65 deletions(-)

drivers/staging/zram/zram_drv.c +17 -56

index 6edefde..bd5efdf 100644
@@ -183,62 +183,25 @@ static inline int is_partial_io(struct bio_vec *bvec)
        return bvec->bv_len != PAGE_SIZE;
 }
 
-static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
-                         u32 index, int offset, struct bio *bio)
+static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 {
-       int ret;
-       size_t clen;
-       struct page *page;
-       unsigned char *user_mem, *cmem, *uncmem = NULL;
-
-       page = bvec->bv_page;
-
-       if (zram_test_flag(zram, index, ZRAM_ZERO)) {
-               handle_zero_page(bvec);
-               return 0;
-       }
+       int ret = LZO_E_OK;
+       size_t clen = PAGE_SIZE;
+       unsigned char *cmem;
+       unsigned long handle = zram->table[index].handle;
 
-       /* Requested page is not present in compressed area */
-       if (unlikely(!zram->table[index].handle)) {
-               pr_debug("Read before write: sector=%lu, size=%u",
-                        (ulong)(bio->bi_sector), bio->bi_size);
-               handle_zero_page(bvec);
+       if (!handle || zram_test_flag(zram, index, ZRAM_ZERO)) {
+               memset(mem, 0, PAGE_SIZE);
                return 0;
        }
 
-       if (is_partial_io(bvec)) {
-               /* Use  a temporary buffer to decompress the page */
-               uncmem = kmalloc(PAGE_SIZE, GFP_KERNEL);
-               if (!uncmem) {
-                       pr_info("Error allocating temp memory!\n");
-                       return -ENOMEM;
-               }
-       }
-
-       user_mem = kmap_atomic(page);
-       if (!is_partial_io(bvec))
-               uncmem = user_mem;
-       clen = PAGE_SIZE;
-
-       cmem = zs_map_object(zram->mem_pool, zram->table[index].handle,
-                               ZS_MM_RO);
-
-       if (zram->table[index].size == PAGE_SIZE) {
-               memcpy(uncmem, cmem, PAGE_SIZE);
-               ret = LZO_E_OK;
-       } else {
+       cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
+       if (zram->table[index].size == PAGE_SIZE)
+               memcpy(mem, cmem, PAGE_SIZE);
+       else
                ret = lzo1x_decompress_safe(cmem, zram->table[index].size,
-                                   uncmem, &clen);
-       }
-
-       if (is_partial_io(bvec)) {
-               memcpy(user_mem + bvec->bv_offset, uncmem + offset,
-                      bvec->bv_len);
-               kfree(uncmem);
-       }
-
-       zs_unmap_object(zram->mem_pool, zram->table[index].handle);
-       kunmap_atomic(user_mem);
+                                               mem, &clen);
+       zs_unmap_object(zram->mem_pool, handle);
 
        /* Should NEVER happen. Return bio error if it does. */
        if (unlikely(ret != LZO_E_OK)) {

@@ -247,36 +210,58 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
                return ret;
        }
 
-       flush_dcache_page(page);
-
        return 0;
 }
 
-static int zram_read_before_write(struct zram *zram, char *mem, u32 index)
+static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
+                         u32 index, int offset, struct bio *bio)
 {
        int ret;
-       size_t clen = PAGE_SIZE;
-       unsigned char *cmem;
-       unsigned long handle = zram->table[index].handle;
+       struct page *page;
+       unsigned char *user_mem, *uncmem = NULL;

Nothing should be printed (even a debug only message) for the ZRAM_ZERO case. This case can be quite common for certain kinds of data and would cause a huge log spew. Also (!handle) case is not the same as zero-filled page case, so this message would be misleading.

So, we should either get rid of this warning entirely or only do pr_debug("Read before write ....") for (!handle) case and log nothing for ZRAM_ZERO case.

======
zram: Fix use-after-free in partial I/O case

When the compressed size of a page exceeds a threshold, the page is stored as-is i.e. in uncompressed form. In the partial I/O i.e. non-PAGE_SIZE'ed I/O case, however, the uncompressed memory was being freed before it could be copied into the zsmalloc pool resulting in use-after-free bug.

Quoted 9 lines · Nitin Gupta

======
zram: Fix use-after-free in partial I/O case

When the compressed size of a page exceeds a threshold, the page is stored as-is i.e. in uncompressed form. In the partial I/O i.e. non-PAGE_SIZE'ed I/O case, however, the uncompressed memory was being freed before it could be copied into the zsmalloc pool resulting in use-after-free bug.

Hello Nitin,
hope you are fine.

how about the following one? I moved some of the code to zram_compress_page() (very similar to zram_decompress_page()), so errors are easier to care in zram_bvec_write(). now we handle both use after-kfree (as you did in your patch), and use after-kunmap.

please review.

Signed-off-by Sergey Senozhatsky <[email protected]>

---

  drivers/staging/zram/zram_drv.c | 91 +++++++++++++++++++++--------------------
  1 file changed, 46 insertions(+), 45 deletions(-)

drivers/staging/zram/zram_drv.c +43 -42

index 47f2e3a..5f37be1 100644
@@ -213,6 +213,44 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
  	return 0;
  }
 
+static int zram_compress_page(struct zram *zram, char *uncmem, u32 index)
+{
+	int ret;
+	size_t clen;
+	unsigned long handle;
+	unsigned char *cmem, *src;
+
+	src = zram->compress_buffer;
+	ret = lzo1x_1_compress(uncmem, PAGE_SIZE, src, &clen,
+			       zram->compress_workmem);
+	if (unlikely(ret != LZO_E_OK)) {
+		pr_err("Page compression failed: err=%d\n", ret);
+		return ret;
+	}
+
+	if (unlikely(clen > max_zpage_size)) {
+		zram_stat_inc(&zram->stats.bad_compress);
+		src = uncmem;
+		clen = PAGE_SIZE;
+	}
+
+	handle = zs_malloc(zram->mem_pool, clen);
+	if (!handle) {
+		pr_info("Error allocating memory for compressed "
+			"page: %u, size=%zu\n", index, clen);
+		return -ENOMEM;
+	}
+
+	cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_WO);
+	memcpy(cmem, src, clen);
+	zs_unmap_object(zram->mem_pool, handle);
+
+	zram->table[index].handle = handle;
+	zram->table[index].size = clen;
+
+	return 0;
+}
+
  static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
  			  u32 index, int offset, struct bio *bio)
  {

@@ -267,13 +305,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
  {
  	int ret;
  	size_t clen;
-	unsigned long handle;
  	struct page *page;
-	unsigned char *user_mem, *cmem, *src, *uncmem = NULL;
+	unsigned char *user_mem, *uncmem = NULL;
 
  	page = bvec->bv_page;
-	src = zram->compress_buffer;
-
  	if (is_partial_io(bvec)) {
  		/*
  		 * This is a partial IO. We need to read the full page

@@ -286,10 +321,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
  			goto out;
  		}
  		ret = zram_decompress_page(zram, uncmem, index);
-		if (ret) {
-			kfree(uncmem);
+		if (ret)
  			goto out;
-		}
  	}
 
  	/*

@@ -309,58 +342,26 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
  		uncmem = user_mem;
 
  	if (page_zero_filled(uncmem)) {
-		kunmap_atomic(user_mem);
-		if (is_partial_io(bvec))
-			kfree(uncmem);
  		zram_stat_inc(&zram->stats.pages_zero);
  		zram_set_flag(zram, index, ZRAM_ZERO);
  		ret = 0;
  		goto out;
  	}
 
-	ret = lzo1x_1_compress(uncmem, PAGE_SIZE, src, &clen,
-			       zram->compress_workmem);
-
-	kunmap_atomic(user_mem);
-	if (is_partial_io(bvec))
-			kfree(uncmem);
-
-	if (unlikely(ret != LZO_E_OK)) {
-		pr_err("Compression failed! err=%d\n", ret);
-		goto out;
-	}
-
-	if (unlikely(clen > max_zpage_size)) {
-		zram_stat_inc(&zram->stats.bad_compress);
-		src = uncmem;
-		clen = PAGE_SIZE;
-	}
-
-	handle = zs_malloc(zram->mem_pool, clen);
-	if (!handle) {
-		pr_info("Error allocating memory for compressed "
-			"page: %u, size=%zu\n", index, clen);
-		ret = -ENOMEM;
+	ret = zram_compress_page(zram, uncmem, index);
+	if (ret)
  		goto out;
-	}
-	cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_WO);
-
-	memcpy(cmem, src, clen);
-
-	zs_unmap_object(zram->mem_pool, handle);
-
-	zram->table[index].handle = handle;
-	zram->table[index].size = clen;
 
+	clen = zram->table[index].size;

Quoted 156 lines

Quoted 9 lines

======
zram: Fix use-after-free in partial I/O case

When the compressed size of a page exceeds a threshold, the page is stored as-is i.e. in uncompressed form. In the partial I/O i.e. non-PAGE_SIZE'ed I/O case, however, the uncompressed memory was being freed before it could be copied into the zsmalloc pool resulting in use-after-free bug.

Hello Nitin,
hope you are fine.

how about the following one? I moved some of the code to zram_compress_page() (very similar to zram_decompress_page()), so errors are easier to care in zram_bvec_write(). now we handle both use after-kfree (as you did in your patch), and use after-kunmap.

 drivers/staging/zram/zram_drv.c | 91 +++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 45 deletions(-)

drivers/staging/zram/zram_drv.c +43 -45

index 47f2e3a..5f37be1 100644
@@ -213,6 +213,44 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 	return 0;
 }
 
+static int zram_compress_page(struct zram *zram, char *uncmem, u32 index)
+{
+	int ret;
+	size_t clen;
+	unsigned long handle;
+	unsigned char *cmem, *src;
+
+	src = zram->compress_buffer;
+	ret = lzo1x_1_compress(uncmem, PAGE_SIZE, src, &clen,
+			       zram->compress_workmem);
+	if (unlikely(ret != LZO_E_OK)) {
+		pr_err("Page compression failed: err=%d\n", ret);
+		return ret;
+	}
+
+	if (unlikely(clen > max_zpage_size)) {
+		zram_stat_inc(&zram->stats.bad_compress);
+		src = uncmem;
+		clen = PAGE_SIZE;
+	}
+
+	handle = zs_malloc(zram->mem_pool, clen);
+	if (!handle) {
+		pr_info("Error allocating memory for compressed "
+			"page: %u, size=%zu\n", index, clen);
+		return -ENOMEM;
+	}
+
+	cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_WO);
+	memcpy(cmem, src, clen);
+	zs_unmap_object(zram->mem_pool, handle);
+
+	zram->table[index].handle = handle;
+	zram->table[index].size = clen;
+
+	return 0;
+}
+
 static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
 			  u32 index, int offset, struct bio *bio)
 {

@@ -267,13 +305,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 {
 	int ret;
 	size_t clen;
-	unsigned long handle;
 	struct page *page;
-	unsigned char *user_mem, *cmem, *src, *uncmem = NULL;
+	unsigned char *user_mem, *uncmem = NULL;
 
 	page = bvec->bv_page;
-	src = zram->compress_buffer;
-
 	if (is_partial_io(bvec)) {
 		/*
 		 * This is a partial IO. We need to read the full page

@@ -286,10 +321,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 			goto out;
 		}
 		ret = zram_decompress_page(zram, uncmem, index);
-		if (ret) {
-			kfree(uncmem);
+		if (ret)
 			goto out;
-		}
 	}
 
 	/*

@@ -309,58 +342,26 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		uncmem = user_mem;
 
 	if (page_zero_filled(uncmem)) {
-		kunmap_atomic(user_mem);
-		if (is_partial_io(bvec))
-			kfree(uncmem);
 		zram_stat_inc(&zram->stats.pages_zero);
 		zram_set_flag(zram, index, ZRAM_ZERO);
 		ret = 0;
 		goto out;
 	}
 
-	ret = lzo1x_1_compress(uncmem, PAGE_SIZE, src, &clen,
-			       zram->compress_workmem);
-
-	kunmap_atomic(user_mem);
-	if (is_partial_io(bvec))
-			kfree(uncmem);
-
-	if (unlikely(ret != LZO_E_OK)) {
-		pr_err("Compression failed! err=%d\n", ret);
-		goto out;
-	}
-
-	if (unlikely(clen > max_zpage_size)) {
-		zram_stat_inc(&zram->stats.bad_compress);
-		src = uncmem;
-		clen = PAGE_SIZE;
-	}
-
-	handle = zs_malloc(zram->mem_pool, clen);
-	if (!handle) {
-		pr_info("Error allocating memory for compressed "
-			"page: %u, size=%zu\n", index, clen);
-		ret = -ENOMEM;
+	ret = zram_compress_page(zram, uncmem, index);
+	if (ret)
 		goto out;
-	}
-	cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_WO);
-
-	memcpy(cmem, src, clen);
-
-	zs_unmap_object(zram->mem_pool, handle);
-
-	zram->table[index].handle = handle;
-	zram->table[index].size = clen;
 
+	clen = zram->table[index].size;
 	/* Update stats */
 	zram_stat64_add(zram, &zram->stats.compr_size, clen);
 	zram_stat_inc(&zram->stats.pages_stored);
 	if (clen <= PAGE_SIZE / 2)
 		zram_stat_inc(&zram->stats.good_compress);
-
-	return 0;
-
 out:

drivers/staging/zram/zram_drv.c static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index, · line 362

Quoted 1 line

drivers/staging/zram/zram_drv.c +1 -0

+	kunmap_atomic(user_mem);

We are doing zs_malloc + zs_map/unmap + memcpy while keeping a page kmap'ed. We must really release it as soon as possible, so zs_compress_page() should just do compression and return with results, or just keep direct can to lzo_compress in bvec_write() since we are not gaining anything by this refactoring, unlike the decompress case.

Do we have a way to generate these partial I/Os so we can exercise these code paths?

Thanks,
Nitin