Re: [PATCH v2] isofs compress: Remove VLA usage

2018-04-10 Thread Jan Kara 
On Thu 05-04-18 11:17:20, Kyle Spiers wrote:
> As part of the effort to remove VLAs from the kernel[1], this changes
> the allocation of the bhs and pages arrays from being on the stack to being
> kcalloc()ed. This also allows for the removal of the explicit zeroing
> of bhs.
> 
> https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Kyle Spiers 

This is a good cleanup but the error recovery is hosed. See below...

> ---
>  fs/isofs/compress.c | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
> index 9bb2fe35799d..4eba16bf173c 100644
> --- a/fs/isofs/compress.c
> +++ b/fs/isofs/compress.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  
> @@ -59,7 +60,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
> loff_t block_start,
>   >> bufshift;
>   int haveblocks;
>   blkcnt_t blocknum;
> - struct buffer_head *bhs[needblocks + 1];
> + struct buffer_head **bhs;
>   int curbh, curpage;
>  
>   if (block_size > deflateBound(1UL << zisofs_block_shift)) {
> @@ -80,7 +81,9 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
> loff_t block_start,
>  
>   /* Because zlib is not thread-safe, do all the I/O at the top. */
>   blocknum = block_start >> bufshift;
> - memset(bhs, 0, (needblocks + 1) * sizeof(struct buffer_head *));
> + bhs = kcalloc(needblocks + 1, sizeof(*bhs), GFP_KERNEL);
> + if (!bhs)
> + return -ENOMEM;

As Joe pointed out this needs to be:

if (!bhs) {
*errp = -ENOMEM;
return 0;
}

> @@ -330,6 +334,10 @@ static int zisofs_readpage(struct file *file, struct 
> page *page)
>   full_page = 0;
>   pcount = 1;
>   }
> + pages = kcalloc(max_t(unsigned int, zisofs_pages_per_cblock, 1),
> + sizeof(*pages), GFP_KERNEL);
> + if (!pages)
> + return -ENOMEM;

And this is wrong as well. You need to do:

if (!pages) {
unlock_page(page);
return -ENOMEM;
}


Honza
-- 
Jan Kara 
SUSE Labs, CR

Re: [PATCH v2] isofs compress: Remove VLA usage

2018-04-10 Thread Jan Kara 
On Thu 05-04-18 11:17:20, Kyle Spiers wrote:
> As part of the effort to remove VLAs from the kernel[1], this changes
> the allocation of the bhs and pages arrays from being on the stack to being
> kcalloc()ed. This also allows for the removal of the explicit zeroing
> of bhs.
> 
> https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Kyle Spiers 

This is a good cleanup but the error recovery is hosed. See below...

> ---
>  fs/isofs/compress.c | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
> index 9bb2fe35799d..4eba16bf173c 100644
> --- a/fs/isofs/compress.c
> +++ b/fs/isofs/compress.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  
> @@ -59,7 +60,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
> loff_t block_start,
>   >> bufshift;
>   int haveblocks;
>   blkcnt_t blocknum;
> - struct buffer_head *bhs[needblocks + 1];
> + struct buffer_head **bhs;
>   int curbh, curpage;
>  
>   if (block_size > deflateBound(1UL << zisofs_block_shift)) {
> @@ -80,7 +81,9 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
> loff_t block_start,
>  
>   /* Because zlib is not thread-safe, do all the I/O at the top. */
>   blocknum = block_start >> bufshift;
> - memset(bhs, 0, (needblocks + 1) * sizeof(struct buffer_head *));
> + bhs = kcalloc(needblocks + 1, sizeof(*bhs), GFP_KERNEL);
> + if (!bhs)
> + return -ENOMEM;

As Joe pointed out this needs to be:

if (!bhs) {
*errp = -ENOMEM;
return 0;
}

> @@ -330,6 +334,10 @@ static int zisofs_readpage(struct file *file, struct 
> page *page)
>   full_page = 0;
>   pcount = 1;
>   }
> + pages = kcalloc(max_t(unsigned int, zisofs_pages_per_cblock, 1),
> + sizeof(*pages), GFP_KERNEL);
> + if (!pages)
> + return -ENOMEM;

And this is wrong as well. You need to do:

if (!pages) {
unlock_page(page);
return -ENOMEM;
}


Honza
-- 
Jan Kara 
SUSE Labs, CR

Re: [PATCH v2] isofs compress: Remove VLA usage

2018-04-06 Thread Joe Perches 
On Thu, 2018-04-05 at 11:17 -0700, Kyle Spiers wrote:
> As part of the effort to remove VLAs from the kernel[1], this changes
> the allocation of the bhs and pages arrays from being on the stack to being
> kcalloc()ed. This also allows for the removal of the explicit zeroing
> of bhs.
> 
> https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Kyle Spiers 
> ---
>  fs/isofs/compress.c | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
> index 9bb2fe35799d..4eba16bf173c 100644
> --- a/fs/isofs/compress.c
> +++ b/fs/isofs/compress.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  
> @@ -59,7 +60,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
> loff_t block_start,
>   >> bufshift;
>   int haveblocks;
>   blkcnt_t blocknum;
> - struct buffer_head *bhs[needblocks + 1];
> + struct buffer_head **bhs;
>   int curbh, curpage;
>  
>   if (block_size > deflateBound(1UL << zisofs_block_shift)) {
> @@ -80,7 +81,9 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
> loff_t block_start,
>  
>   /* Because zlib is not thread-safe, do all the I/O at the top. */
>   blocknum = block_start >> bufshift;
> - memset(bhs, 0, (needblocks + 1) * sizeof(struct buffer_head *));
> + bhs = kcalloc(needblocks + 1, sizeof(*bhs), GFP_KERNEL);
> + if (!bhs)
> + return -ENOMEM;

This direct return appears incorrect.

It seems it should be:

if (!bhs) {
*errp = -ENOMEM;
return 0;
}

Re: [PATCH v2] isofs compress: Remove VLA usage

2018-04-06 Thread Joe Perches 
On Thu, 2018-04-05 at 11:17 -0700, Kyle Spiers wrote:
> As part of the effort to remove VLAs from the kernel[1], this changes
> the allocation of the bhs and pages arrays from being on the stack to being
> kcalloc()ed. This also allows for the removal of the explicit zeroing
> of bhs.
> 
> https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Kyle Spiers 
> ---
>  fs/isofs/compress.c | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
> index 9bb2fe35799d..4eba16bf173c 100644
> --- a/fs/isofs/compress.c
> +++ b/fs/isofs/compress.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  
> @@ -59,7 +60,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
> loff_t block_start,
>   >> bufshift;
>   int haveblocks;
>   blkcnt_t blocknum;
> - struct buffer_head *bhs[needblocks + 1];
> + struct buffer_head **bhs;
>   int curbh, curpage;
>  
>   if (block_size > deflateBound(1UL << zisofs_block_shift)) {
> @@ -80,7 +81,9 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
> loff_t block_start,
>  
>   /* Because zlib is not thread-safe, do all the I/O at the top. */
>   blocknum = block_start >> bufshift;
> - memset(bhs, 0, (needblocks + 1) * sizeof(struct buffer_head *));
> + bhs = kcalloc(needblocks + 1, sizeof(*bhs), GFP_KERNEL);
> + if (!bhs)
> + return -ENOMEM;

This direct return appears incorrect.

It seems it should be:

if (!bhs) {
*errp = -ENOMEM;
return 0;
}

Re: [PATCH v2] isofs compress: Remove VLA usage

2018-04-05 Thread Kees Cook 
On Thu, Apr 5, 2018 at 11:17 AM, Kyle Spiers  wrote:
> As part of the effort to remove VLAs from the kernel[1], this changes
> the allocation of the bhs and pages arrays from being on the stack to being
> kcalloc()ed. This also allows for the removal of the explicit zeroing
> of bhs.
>
> https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Kyle Spiers 

Looks good! Thanks for fixing the sparc64 build failure. :)

Reviewed-by: Kees Cook 

-Kees

> ---
>  fs/isofs/compress.c | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
> index 9bb2fe35799d..4eba16bf173c 100644
> --- a/fs/isofs/compress.c
> +++ b/fs/isofs/compress.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>
> +#include 
>  #include 
>  #include 
>
> @@ -59,7 +60,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
> loff_t block_start,
> >> bufshift;
> int haveblocks;
> blkcnt_t blocknum;
> -   struct buffer_head *bhs[needblocks + 1];
> +   struct buffer_head **bhs;
> int curbh, curpage;
>
> if (block_size > deflateBound(1UL << zisofs_block_shift)) {
> @@ -80,7 +81,9 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
> loff_t block_start,
>
> /* Because zlib is not thread-safe, do all the I/O at the top. */
> blocknum = block_start >> bufshift;
> -   memset(bhs, 0, (needblocks + 1) * sizeof(struct buffer_head *));
> +   bhs = kcalloc(needblocks + 1, sizeof(*bhs), GFP_KERNEL);
> +   if (!bhs)
> +   return -ENOMEM;
> haveblocks = isofs_get_blocks(inode, blocknum, bhs, needblocks);
> ll_rw_block(REQ_OP_READ, 0, haveblocks, bhs);
>
> @@ -190,6 +193,7 @@ static loff_t zisofs_uncompress_block(struct inode 
> *inode, loff_t block_start,
>  b_eio:
> for (i = 0; i < haveblocks; i++)
> brelse(bhs[i]);
> +   kfree(bhs);
> return stream.total_out;
>  }
>
> @@ -305,7 +309,7 @@ static int zisofs_readpage(struct file *file, struct page 
> *page)
> unsigned int zisofs_pages_per_cblock =
> PAGE_SHIFT <= zisofs_block_shift ?
> (1 << (zisofs_block_shift - PAGE_SHIFT)) : 0;
> -   struct page *pages[max_t(unsigned, zisofs_pages_per_cblock, 1)];
> +   struct page **pages;
> pgoff_t index = page->index, end_index;
>
> end_index = (inode->i_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> @@ -330,6 +334,10 @@ static int zisofs_readpage(struct file *file, struct 
> page *page)
> full_page = 0;
> pcount = 1;
> }
> +   pages = kcalloc(max_t(unsigned int, zisofs_pages_per_cblock, 1),
> +   sizeof(*pages), GFP_KERNEL);
> +   if (!pages)
> +   return -ENOMEM;
> pages[full_page] = page;
>
> for (i = 0; i < pcount; i++, index++) {
> @@ -357,6 +365,7 @@ static int zisofs_readpage(struct file *file, struct page 
> *page)
> }
>
> /* At this point, err contains 0 or -EIO depending on the "critical" 
> page */
> +   kfree(pages);
> return err;
>  }
>
> --
> 2.17.0.484.g0c8726318c-goog
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v2] isofs compress: Remove VLA usage

2018-04-05 Thread Kees Cook 
On Thu, Apr 5, 2018 at 11:17 AM, Kyle Spiers  wrote:
> As part of the effort to remove VLAs from the kernel[1], this changes
> the allocation of the bhs and pages arrays from being on the stack to being
> kcalloc()ed. This also allows for the removal of the explicit zeroing
> of bhs.
>
> https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Kyle Spiers 

Looks good! Thanks for fixing the sparc64 build failure. :)

Reviewed-by: Kees Cook 

-Kees

> ---
>  fs/isofs/compress.c | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
> index 9bb2fe35799d..4eba16bf173c 100644
> --- a/fs/isofs/compress.c
> +++ b/fs/isofs/compress.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>
> +#include 
>  #include 
>  #include 
>
> @@ -59,7 +60,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
> loff_t block_start,
> >> bufshift;
> int haveblocks;
> blkcnt_t blocknum;
> -   struct buffer_head *bhs[needblocks + 1];
> +   struct buffer_head **bhs;
> int curbh, curpage;
>
> if (block_size > deflateBound(1UL << zisofs_block_shift)) {
> @@ -80,7 +81,9 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
> loff_t block_start,
>
> /* Because zlib is not thread-safe, do all the I/O at the top. */
> blocknum = block_start >> bufshift;
> -   memset(bhs, 0, (needblocks + 1) * sizeof(struct buffer_head *));
> +   bhs = kcalloc(needblocks + 1, sizeof(*bhs), GFP_KERNEL);
> +   if (!bhs)
> +   return -ENOMEM;
> haveblocks = isofs_get_blocks(inode, blocknum, bhs, needblocks);
> ll_rw_block(REQ_OP_READ, 0, haveblocks, bhs);
>
> @@ -190,6 +193,7 @@ static loff_t zisofs_uncompress_block(struct inode 
> *inode, loff_t block_start,
>  b_eio:
> for (i = 0; i < haveblocks; i++)
> brelse(bhs[i]);
> +   kfree(bhs);
> return stream.total_out;
>  }
>
> @@ -305,7 +309,7 @@ static int zisofs_readpage(struct file *file, struct page 
> *page)
> unsigned int zisofs_pages_per_cblock =
> PAGE_SHIFT <= zisofs_block_shift ?
> (1 << (zisofs_block_shift - PAGE_SHIFT)) : 0;
> -   struct page *pages[max_t(unsigned, zisofs_pages_per_cblock, 1)];
> +   struct page **pages;
> pgoff_t index = page->index, end_index;
>
> end_index = (inode->i_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> @@ -330,6 +334,10 @@ static int zisofs_readpage(struct file *file, struct 
> page *page)
> full_page = 0;
> pcount = 1;
> }
> +   pages = kcalloc(max_t(unsigned int, zisofs_pages_per_cblock, 1),
> +   sizeof(*pages), GFP_KERNEL);
> +   if (!pages)
> +   return -ENOMEM;
> pages[full_page] = page;
>
> for (i = 0; i < pcount; i++, index++) {
> @@ -357,6 +365,7 @@ static int zisofs_readpage(struct file *file, struct page 
> *page)
> }
>
> /* At this point, err contains 0 or -EIO depending on the "critical" 
> page */
> +   kfree(pages);
> return err;
>  }
>
> --
> 2.17.0.484.g0c8726318c-goog
>



-- 
Kees Cook
Pixel Security

[PATCH v2] isofs compress: Remove VLA usage

2018-04-05 Thread Kyle Spiers 
As part of the effort to remove VLAs from the kernel[1], this changes
the allocation of the bhs and pages arrays from being on the stack to being
kcalloc()ed. This also allows for the removal of the explicit zeroing
of bhs.

https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kyle Spiers 
---
 fs/isofs/compress.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
index 9bb2fe35799d..4eba16bf173c 100644
--- a/fs/isofs/compress.c
+++ b/fs/isofs/compress.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 
@@ -59,7 +60,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
loff_t block_start,
>> bufshift;
int haveblocks;
blkcnt_t blocknum;
-   struct buffer_head *bhs[needblocks + 1];
+   struct buffer_head **bhs;
int curbh, curpage;
 
if (block_size > deflateBound(1UL << zisofs_block_shift)) {
@@ -80,7 +81,9 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
loff_t block_start,
 
/* Because zlib is not thread-safe, do all the I/O at the top. */
blocknum = block_start >> bufshift;
-   memset(bhs, 0, (needblocks + 1) * sizeof(struct buffer_head *));
+   bhs = kcalloc(needblocks + 1, sizeof(*bhs), GFP_KERNEL);
+   if (!bhs)
+   return -ENOMEM;
haveblocks = isofs_get_blocks(inode, blocknum, bhs, needblocks);
ll_rw_block(REQ_OP_READ, 0, haveblocks, bhs);
 
@@ -190,6 +193,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
loff_t block_start,
 b_eio:
for (i = 0; i < haveblocks; i++)
brelse(bhs[i]);
+   kfree(bhs);
return stream.total_out;
 }
 
@@ -305,7 +309,7 @@ static int zisofs_readpage(struct file *file, struct page 
*page)
unsigned int zisofs_pages_per_cblock =
PAGE_SHIFT <= zisofs_block_shift ?
(1 << (zisofs_block_shift - PAGE_SHIFT)) : 0;
-   struct page *pages[max_t(unsigned, zisofs_pages_per_cblock, 1)];
+   struct page **pages;
pgoff_t index = page->index, end_index;
 
end_index = (inode->i_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
@@ -330,6 +334,10 @@ static int zisofs_readpage(struct file *file, struct page 
*page)
full_page = 0;
pcount = 1;
}
+   pages = kcalloc(max_t(unsigned int, zisofs_pages_per_cblock, 1),
+   sizeof(*pages), GFP_KERNEL);
+   if (!pages)
+   return -ENOMEM;
pages[full_page] = page;
 
for (i = 0; i < pcount; i++, index++) {
@@ -357,6 +365,7 @@ static int zisofs_readpage(struct file *file, struct page 
*page)
}   
 
/* At this point, err contains 0 or -EIO depending on the "critical" 
page */
+   kfree(pages);
return err;
 }
 
-- 
2.17.0.484.g0c8726318c-goog

[PATCH v2] isofs compress: Remove VLA usage

2018-04-05 Thread Kyle Spiers 
As part of the effort to remove VLAs from the kernel[1], this changes
the allocation of the bhs and pages arrays from being on the stack to being
kcalloc()ed. This also allows for the removal of the explicit zeroing
of bhs.

https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kyle Spiers 
---
 fs/isofs/compress.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
index 9bb2fe35799d..4eba16bf173c 100644
--- a/fs/isofs/compress.c
+++ b/fs/isofs/compress.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 
@@ -59,7 +60,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
loff_t block_start,
>> bufshift;
int haveblocks;
blkcnt_t blocknum;
-   struct buffer_head *bhs[needblocks + 1];
+   struct buffer_head **bhs;
int curbh, curpage;
 
if (block_size > deflateBound(1UL << zisofs_block_shift)) {
@@ -80,7 +81,9 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
loff_t block_start,
 
/* Because zlib is not thread-safe, do all the I/O at the top. */
blocknum = block_start >> bufshift;
-   memset(bhs, 0, (needblocks + 1) * sizeof(struct buffer_head *));
+   bhs = kcalloc(needblocks + 1, sizeof(*bhs), GFP_KERNEL);
+   if (!bhs)
+   return -ENOMEM;
haveblocks = isofs_get_blocks(inode, blocknum, bhs, needblocks);
ll_rw_block(REQ_OP_READ, 0, haveblocks, bhs);
 
@@ -190,6 +193,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, 
loff_t block_start,
 b_eio:
for (i = 0; i < haveblocks; i++)
brelse(bhs[i]);
+   kfree(bhs);
return stream.total_out;
 }
 
@@ -305,7 +309,7 @@ static int zisofs_readpage(struct file *file, struct page 
*page)
unsigned int zisofs_pages_per_cblock =
PAGE_SHIFT <= zisofs_block_shift ?
(1 << (zisofs_block_shift - PAGE_SHIFT)) : 0;
-   struct page *pages[max_t(unsigned, zisofs_pages_per_cblock, 1)];
+   struct page **pages;
pgoff_t index = page->index, end_index;
 
end_index = (inode->i_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
@@ -330,6 +334,10 @@ static int zisofs_readpage(struct file *file, struct page 
*page)
full_page = 0;
pcount = 1;
}
+   pages = kcalloc(max_t(unsigned int, zisofs_pages_per_cblock, 1),
+   sizeof(*pages), GFP_KERNEL);
+   if (!pages)
+   return -ENOMEM;
pages[full_page] = page;
 
for (i = 0; i < pcount; i++, index++) {
@@ -357,6 +365,7 @@ static int zisofs_readpage(struct file *file, struct page 
*page)
}   
 
/* At this point, err contains 0 or -EIO depending on the "critical" 
page */
+   kfree(pages);
return err;
 }
 
-- 
2.17.0.484.g0c8726318c-goog