[PATCH 1/2] zram: factor-out zram_decompress_page() function

[PATCH 1/2] zram: factor-out zram_decompress_page() function

[Sergey Senozhatsky]

  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 <sergey.senozhatsky@gmail.com>

---

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

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 6edefde..7585467 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -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;
 
-	if (zram_test_flag(zram, index, ZRAM_ZERO) || !handle) {
-		memset(mem, 0, PAGE_SIZE);
+	page = bvec->bv_page;
+
+	if (unlikely(!zram->table[index].handle) ||
+			zram_test_flag(zram, index, ZRAM_ZERO)) {
+		pr_debug("Read before write: sector=%lu, size=%u",
+			 (ulong)(bio->bi_sector), bio->bi_size);
+		handle_zero_page(bvec);
 		return 0;
 	}
 
-	cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
-	ret = lzo1x_decompress_safe(cmem, zram->table[index].size,
-				    mem, &clen);
-	zs_unmap_object(zram->mem_pool, handle);
+	user_mem = kmap_atomic(page);
+	if (is_partial_io(bvec))
+		/* Use  a temporary buffer to decompress the page */
+		uncmem = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	else
+		uncmem = user_mem;
 
+	if (!uncmem) {
+		pr_info("Unable to allocate temp memory\n");
+		ret = -ENOMEM;
+		goto out_cleanup;
+	}
+
+	ret = zram_decompress_page(zram, uncmem, index);
 	/* Should NEVER happen. Return bio error if it does. */
 	if (unlikely(ret != LZO_E_OK)) {
 		pr_err("Decompression failed! err=%d, page=%u\n", ret, index);
 		zram_stat64_inc(zram, &zram->stats.failed_reads);
-		return ret;
+		goto out_cleanup;
 	}
 
-	return 0;
+	if (is_partial_io(bvec))
+		memcpy(user_mem + bvec->bv_offset, uncmem + offset,
+				bvec->bv_len);
+
+	flush_dcache_page(page);
+	ret = 0;
+out_cleanup:
+	kunmap_atomic(user_mem);
+	if (is_partial_io(bvec))
+		kfree(uncmem);
+	return ret;
 }
 
 static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
@@ -302,7 +287,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 			ret = -ENOMEM;
 			goto out;
 		}
-		ret = zram_read_before_write(zram, uncmem, index);
+		ret = zram_decompress_page(zram, uncmem, index);
 		if (ret) {
 			kfree(uncmem);
 			goto out;

Re: [PATCH 1/2] zram: factor-out zram_decompress_page() function

[Nitin Gupta]

On 10/27/2012 09:00 AM, Sergey Senozhatsky wrote:
>    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 <sergey.senozhatsky@gmail.com>
>
> ---
>
>   drivers/staging/zram/zram_drv.c | 115 +++++++++++++++++-----------------------
>   1 file changed, 50 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 6edefde..7585467 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -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;
>
> -	if (zram_test_flag(zram, index, ZRAM_ZERO) || !handle) {
> -		memset(mem, 0, PAGE_SIZE);
> +	page = bvec->bv_page;
> +
> +	if (unlikely(!zram->table[index].handle) ||
> +			zram_test_flag(zram, index, ZRAM_ZERO)) {
> +		pr_debug("Read before write: sector=%lu, size=%u",
> +			 (ulong)(bio->bi_sector), bio->bi_size);


"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 <ngupta@vflare.org>
---

diff --git a/drivers/staging/zram/zram_drv.c 
b/drivers/staging/zram/zram_drv.c
index 7585467..635736b 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -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;
-		}
  	}

  	/*
@@ -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);
  		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)) {
  		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

Re: [PATCH 1/2] zram: factor-out zram_decompress_page() function

[Sergey Senozhatsky]

On (10/29/12 10:14), Nitin Gupta wrote:
> 
> "Read before write" message is not valid in case ZRAM_ZERO flag is
> set. Its true only in !handle case.
> 

do we actually need this message?


> 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.
>

Nitin, I think let's deal with one change at a time. I'll try to resend my patch
shortly, then we can continue with your fix (I didn't hit that problem, though
will be happy to help with testing).

	-ss

 
> ======
> 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 <ngupta@vflare.org>
> ---
> 
> diff --git a/drivers/staging/zram/zram_drv.c
> b/drivers/staging/zram/zram_drv.c
> index 7585467..635736b 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -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;
> -		}
>  	}
> 
>  	/*
> @@ -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);
>  		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)) {
>  		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
> 

[PATCH 1/2] zram: factor-out zram_decompress_page() function (v2)

[Sergey Senozhatsky]

  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 <sergey.senozhatsky@gmail.com>

---

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

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 6edefde..bd5efdf 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -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;
 
-	if (zram_test_flag(zram, index, ZRAM_ZERO) || !handle) {
-		memset(mem, 0, PAGE_SIZE);
+	page = bvec->bv_page;
+
+	if (unlikely(!zram->table[index].handle) ||
+			zram_test_flag(zram, index, ZRAM_ZERO)) {
+		pr_debug("Handle zero page: sector=%lu, size=%u",
+			 (ulong)(bio->bi_sector), bio->bi_size);
+		handle_zero_page(bvec);
 		return 0;
 	}
 
-	cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
-	ret = lzo1x_decompress_safe(cmem, zram->table[index].size,
-				    mem, &clen);
-	zs_unmap_object(zram->mem_pool, handle);
+	user_mem = kmap_atomic(page);
+	if (is_partial_io(bvec))
+		/* Use  a temporary buffer to decompress the page */
+		uncmem = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	else
+		uncmem = user_mem;
 
+	if (!uncmem) {
+		pr_info("Unable to allocate temp memory\n");
+		ret = -ENOMEM;
+		goto out_cleanup;
+	}
+
+	ret = zram_decompress_page(zram, uncmem, index);
 	/* Should NEVER happen. Return bio error if it does. */
 	if (unlikely(ret != LZO_E_OK)) {
 		pr_err("Decompression failed! err=%d, page=%u\n", ret, index);
 		zram_stat64_inc(zram, &zram->stats.failed_reads);
-		return ret;
+		goto out_cleanup;
 	}
 
-	return 0;
+	if (is_partial_io(bvec))
+		memcpy(user_mem + bvec->bv_offset, uncmem + offset,
+				bvec->bv_len);
+
+	flush_dcache_page(page);
+	ret = 0;
+out_cleanup:
+	kunmap_atomic(user_mem);
+	if (is_partial_io(bvec))
+		kfree(uncmem);
+	return ret;
 }
 
 static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
@@ -302,7 +287,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 			ret = -ENOMEM;
 			goto out;
 		}
-		ret = zram_read_before_write(zram, uncmem, index);
+		ret = zram_decompress_page(zram, uncmem, index);
 		if (ret) {
 			kfree(uncmem);
 			goto out;

Re: [PATCH 1/2] zram: factor-out zram_decompress_page() function (v2)

[Nitin Gupta]

On Mon, Oct 29, 2012 at 11:05 AM, Sergey Senozhatsky
<sergey.senozhatsky@gmail.com> wrote:
>   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 <sergey.senozhatsky@gmail.com>
>
> ---
>
>  drivers/staging/zram/zram_drv.c | 115 +++++++++++++++++-----------------------
>  1 file changed, 50 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 6edefde..bd5efdf 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -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;
>
> -       if (zram_test_flag(zram, index, ZRAM_ZERO) || !handle) {
> -               memset(mem, 0, PAGE_SIZE);
> +       page = bvec->bv_page;
> +
> +       if (unlikely(!zram->table[index].handle) ||
> +                       zram_test_flag(zram, index, ZRAM_ZERO)) {
> +               pr_debug("Handle zero page: sector=%lu, size=%u",
> +                        (ulong)(bio->bi_sector), bio->bi_size);
> +               handle_zero_page(bvec);

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.

Nitin

Re: [PATCH 1/2] zram: factor-out zram_decompress_page() function (v2)

[Sergey Senozhatsky]

On (10/29/12 11:32), Nitin Gupta wrote:
> 
> 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.
> 

I'd rather remove this message. Will resend.



By the way, about use after-free. I'm afraid you fix is not covering 100% of the
cases.

The problem is with this case:
[..]
334 
335         if (unlikely(clen > max_zpage_size)) {
336                 zram_stat_inc(&zram->stats.bad_compress);
337                 src = uncmem;
338                 clen = PAGE_SIZE;
339         }
340 
[..]

where uncmem could be:
-- kmap'ed page
-- kmalloc'ed page

both of which were unmap'ed/kfree'd before. you moved kfree to the end of the function,
while kunmap_atomic(user_mem) is still happening before src = uncmem/memcpy(cmem, src, clen)
pair.


	-ss

Re: [PATCH 1/2] zram: factor-out zram_decompress_page() function (v3)

[Sergey Senozhatsky]

  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().

  v3: remove debug message for handle_zero_page() case
  v2: changed debug message for handle_zero_page() case to a more general one

  Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

---

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

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 6edefde..fb4a7c9 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -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,56 @@ 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;
 
-	if (zram_test_flag(zram, index, ZRAM_ZERO) || !handle) {
-		memset(mem, 0, PAGE_SIZE);
+	page = bvec->bv_page;
+
+	if (unlikely(!zram->table[index].handle) ||
+			zram_test_flag(zram, index, ZRAM_ZERO)) {
+		handle_zero_page(bvec);
 		return 0;
 	}
 
-	cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
-	ret = lzo1x_decompress_safe(cmem, zram->table[index].size,
-				    mem, &clen);
-	zs_unmap_object(zram->mem_pool, handle);
+	user_mem = kmap_atomic(page);
+	if (is_partial_io(bvec))
+		/* Use  a temporary buffer to decompress the page */
+		uncmem = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	else
+		uncmem = user_mem;
 
+	if (!uncmem) {
+		pr_info("Unable to allocate temp memory\n");
+		ret = -ENOMEM;
+		goto out_cleanup;
+	}
+
+	ret = zram_decompress_page(zram, uncmem, index);
 	/* Should NEVER happen. Return bio error if it does. */
 	if (unlikely(ret != LZO_E_OK)) {
 		pr_err("Decompression failed! err=%d, page=%u\n", ret, index);
 		zram_stat64_inc(zram, &zram->stats.failed_reads);
-		return ret;
+		goto out_cleanup;
 	}
 
-	return 0;
+	if (is_partial_io(bvec))
+		memcpy(user_mem + bvec->bv_offset, uncmem + offset,
+				bvec->bv_len);
+
+	flush_dcache_page(page);
+	ret = 0;
+out_cleanup:
+	kunmap_atomic(user_mem);
+	if (is_partial_io(bvec))
+		kfree(uncmem);
+	return ret;
 }
 
 static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
@@ -302,7 +285,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 			ret = -ENOMEM;
 			goto out;
 		}
-		ret = zram_read_before_write(zram, uncmem, index);
+		ret = zram_decompress_page(zram, uncmem, index);
 		if (ret) {
 			kfree(uncmem);
 			goto out;

Re: [PATCH 1/2] zram: factor-out zram_decompress_page() function

[Sergey Senozhatsky]

On (10/29/12 10:14), Nitin Gupta wrote:
> ======
> 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 <sergey.senozhatsky@gmail.com>

---

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

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 47f2e3a..5f37be1 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -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:
+	kunmap_atomic(user_mem);
+	if (is_partial_io(bvec))
+		kfree(uncmem);
 	if (ret)
 		zram_stat64_inc(zram, &zram->stats.failed_writes);
 	return ret;

Re: [PATCH 1/2] zram: factor-out zram_decompress_page() function

[Nitin Gupta]

On 10/30/2012 02:04 PM, Sergey Senozhatsky wrote:
> On (10/29/12 10:14), Nitin Gupta wrote:
>> ======
>> 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 <sergey.senozhatsky@gmail.com>
>
> ---
>
>   drivers/staging/zram/zram_drv.c | 91 +++++++++++++++++++++--------------------
>   1 file changed, 46 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 47f2e3a..5f37be1 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -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:
> +	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

Re: zram: use after free

[Sergey Senozhatsky]

I've renamed the topic.


On (10/30/12 20:55), Nitin Gupta wrote:
> >>======
> >>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(-)
> >
> >diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> >index 47f2e3a..5f37be1 100644
> >--- a/drivers/staging/zram/zram_drv.c
> >+++ b/drivers/staging/zram/zram_drv.c
> >@@ -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:
> >+	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.
>

yeah, we can unmap and map again as a slow-path for
	`clen > max_zpage_size'
 

in that case zs_compress_page() is not helping. the main reason behind this
function was to make bvec_write() easier, if we count our fingers for
what bvec_write() is capable of, it turns out to be a pretty mighty function.
I'll think about this.


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

I'll try.

	-ss

> Thanks,
> Nitin
> 

[PATCH 2/2] zram: permit sleeping while in pool zs_malloc()

[Sergey Senozhatsky]

    zram: permit sleeping while in pool zs_malloc()
    
    zram pool is created with GFP_NOIO flag, which may trigger errors because nested allocation
    are able to sleep. set __GFP_WAIT pool flag in zram_init_device() to allow sleeping.
    
     BUG: sleeping function called from invalid context at mm/page_alloc.c:2603
     in_atomic(): 1, irqs_disabled(): 0, pid: 2555, name: mkfs.reiserfs
     2 locks held by mkfs.reiserfs/2555:
      #0:  (&zram->init_lock){+++++.}, at: [<ffffffffa0127d18>] zram_make_request+0x48/0x270 [zram]
      #1:  (&zram->lock){++++..}, at: [<ffffffffa012742b>] zram_bvec_rw+0x3b/0x510 [zram]
     Pid: 2555, comm: mkfs.reiserfs Tainted: G         C   3.7.0-rc3-dbg-01664-gf2d9543-dirty #1401
     Call Trace:
      [<ffffffff8107984a>] __might_sleep+0x15a/0x250
      [<ffffffff8111df9b>] __alloc_pages_nodemask+0x1bb/0x920
      [<ffffffffa00f0b93>] ? zs_malloc+0x63/0x480 [zsmalloc]
      [<ffffffff81320e2d>] ? do_raw_spin_unlock+0x5d/0xb0
      [<ffffffffa00f0cf5>] zs_malloc+0x1c5/0x480 [zsmalloc]
      [<ffffffffa0127574>] zram_bvec_rw+0x184/0x510 [zram]
      [<ffffffffa0127e85>] zram_make_request+0x1b5/0x270 [zram]
      [<ffffffff812ec0c2>] generic_make_request+0xc2/0x110
      [<ffffffff812ec17a>] submit_bio+0x6a/0x140
      [<ffffffff8119f27b>] submit_bh+0xfb/0x130
      [<ffffffff811a2710>] __block_write_full_page+0x220/0x3d0
      [<ffffffff810a7784>] ? __lock_is_held+0x54/0x80
      [<ffffffff8119ffb0>] ? end_buffer_async_read+0x210/0x210
      [<ffffffff811a7aa0>] ? blkdev_get_blocks+0xd0/0xd0
      [<ffffffff811a7aa0>] ? blkdev_get_blocks+0xd0/0xd0
      [<ffffffff8119ffb0>] ? end_buffer_async_read+0x210/0x210
      [<ffffffff811a298f>] block_write_full_page_endio+0xcf/0x100
      [<ffffffff8111f555>] ? clear_page_dirty_for_io+0x105/0x130
      [<ffffffff811a29d5>] block_write_full_page+0x15/0x20
      [<ffffffff811a7038>] blkdev_writepage+0x18/0x20
      [<ffffffff8111f3aa>] __writepage+0x1a/0x50
      [<ffffffff8111f8b0>] write_cache_pages+0x200/0x630
      [<ffffffff8111e883>] ? free_hot_cold_page+0x113/0x1a0
      [<ffffffff8111f390>] ? global_dirtyable_memory+0x40/0x40
      [<ffffffff8111fd2d>] generic_writepages+0x4d/0x70
      [<ffffffff81121071>] do_writepages+0x21/0x50
      [<ffffffff81116939>] __filemap_fdatawrite_range+0x59/0x60
      [<ffffffff81116a40>] filemap_write_and_wait_range+0x50/0x70
      [<ffffffff811a73a4>] blkdev_fsync+0x24/0x50
      [<ffffffff8119d5bd>] do_fsync+0x5d/0x90
      [<ffffffff8119d990>] sys_fsync+0x10/0x20
      [<ffffffff815dce06>] tracesys+0xd4/0xd9
    
    Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

---

 drivers/staging/zram/zram_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index d2e0a85..47f2e3a 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -576,7 +576,7 @@ int zram_init_device(struct zram *zram)
 	/* zram devices sort of resembles non-rotational disks */
 	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
 
-	zram->mem_pool = zs_create_pool("zram", GFP_NOIO | __GFP_HIGHMEM);
+	zram->mem_pool = zs_create_pool("zram", GFP_NOIO | __GFP_WAIT | __GFP_HIGHMEM);
 	if (!zram->mem_pool) {
 		pr_err("Error creating memory pool\n");
 		ret = -ENOMEM;

Re: [PATCH 2/2] zram: permit sleeping while in pool zs_malloc()

[Greg Kroah-Hartman]

On Tue, Oct 30, 2012 at 12:03:19PM +0300, Sergey Senozhatsky wrote:
>     zram: permit sleeping while in pool zs_malloc()

2/2?  Huh?  Where is 1/2?

I have a raft of patches from you, all out of order, and full of
responses from Nitin, and so, I really have no idea what should and
should not be applied here.

So, I'm dropping them all.  Please work with Nitin to get a series of
patches that are acceptable and resend them with the proper numbering.

thanks,

greg k-h

[PATCH 1/2] zram: factor-out zram_decompress_page() function

[Sergey Senozhatsky]

  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 <sergey.senozhatsky@gmail.com>

---

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

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 6edefde..fb4a7c9 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -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,56 @@ 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;
 
-	if (zram_test_flag(zram, index, ZRAM_ZERO) || !handle) {
-		memset(mem, 0, PAGE_SIZE);
+	page = bvec->bv_page;
+
+	if (unlikely(!zram->table[index].handle) ||
+			zram_test_flag(zram, index, ZRAM_ZERO)) {
+		handle_zero_page(bvec);
 		return 0;
 	}
 
-	cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
-	ret = lzo1x_decompress_safe(cmem, zram->table[index].size,
-				    mem, &clen);
-	zs_unmap_object(zram->mem_pool, handle);
+	user_mem = kmap_atomic(page);
+	if (is_partial_io(bvec))
+		/* Use  a temporary buffer to decompress the page */
+		uncmem = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	else
+		uncmem = user_mem;
 
+	if (!uncmem) {
+		pr_info("Unable to allocate temp memory\n");
+		ret = -ENOMEM;
+		goto out_cleanup;
+	}
+
+	ret = zram_decompress_page(zram, uncmem, index);
 	/* Should NEVER happen. Return bio error if it does. */
 	if (unlikely(ret != LZO_E_OK)) {
 		pr_err("Decompression failed! err=%d, page=%u\n", ret, index);
 		zram_stat64_inc(zram, &zram->stats.failed_reads);
-		return ret;
+		goto out_cleanup;
 	}
 
-	return 0;
+	if (is_partial_io(bvec))
+		memcpy(user_mem + bvec->bv_offset, uncmem + offset,
+				bvec->bv_len);
+
+	flush_dcache_page(page);
+	ret = 0;
+out_cleanup:
+	kunmap_atomic(user_mem);
+	if (is_partial_io(bvec))
+		kfree(uncmem);
+	return ret;
 }
 
 static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
@@ -302,7 +285,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 			ret = -ENOMEM;
 			goto out;
 		}
-		ret = zram_read_before_write(zram, uncmem, index);
+		ret = zram_decompress_page(zram, uncmem, index);
 		if (ret) {
 			kfree(uncmem);
 			goto out;

Re: [PATCH 1/2] zram: factor-out zram_decompress_page() function

[Greg Kroah-Hartman]

On Tue, Oct 30, 2012 at 09:58:42PM +0300, Sergey Senozhatsky wrote:
>   zram: factor-out zram_decompress_page() function

What's with the indentation?

And including the Subject: again here?

> 
>   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 <sergey.senozhatsky@gmail.com>

Where did Nitin's ack go?

I would have to edit these by hand to apply them, which isn't ok.
Please fix this up, add Nitin's acks, and resend properly.

thanks,

greg k-h