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

[Joe Perches]

On Wed, 2018-04-11 at 07:39 +0100, Al Viro wrote:
> Shit happens...
> 
> Al, fully expecting to discover having made something spectacularly bonehead
> 2 minutes after sending a patch out - happens often enough.

I've done that.  Again, no worries.

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

[Joe Perches]

On Wed, 2018-04-11 at 07:39 +0100, Al Viro wrote:
> Shit happens...
> 
> Al, fully expecting to discover having made something spectacularly bonehead
> 2 minutes after sending a patch out - happens often enough.

I've done that.  Again, no worries.

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

[Al Viro]

On Tue, Apr 10, 2018 at 11:15:28PM -0700, Joe Perches wrote:
> On Tue, 2018-04-10 at 17:51 -0700, Kees Cook wrote:
> > On Tue, Apr 10, 2018 at 5:33 PM, Al Viro  wrote:
> > > On Tue, Apr 10, 2018 at 04:45:32PM -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
> > > 
> > > Do you even bother reading the feedback given to such patches?  I'm just
> > > curious,
> > 
> > He made a tool mistake (sent v2 again) -- please see v4.
> 
> No worries but Kyle didn't just mistakenly resend v2.
> It was a v3 with a different below the dashes changelog.
> 
> Kyle, it would be good if next time you were to add a
> listing of changes in each revision below the dashes.

Shit happens...

Al, fully expecting to discover having made something spectacularly bonehead
2 minutes after sending a patch out - happens often enough.

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

[Al Viro]

On Tue, Apr 10, 2018 at 11:15:28PM -0700, Joe Perches wrote:
> On Tue, 2018-04-10 at 17:51 -0700, Kees Cook wrote:
> > On Tue, Apr 10, 2018 at 5:33 PM, Al Viro  wrote:
> > > On Tue, Apr 10, 2018 at 04:45:32PM -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
> > > 
> > > Do you even bother reading the feedback given to such patches?  I'm just
> > > curious,
> > 
> > He made a tool mistake (sent v2 again) -- please see v4.
> 
> No worries but Kyle didn't just mistakenly resend v2.
> It was a v3 with a different below the dashes changelog.
> 
> Kyle, it would be good if next time you were to add a
> listing of changes in each revision below the dashes.

Shit happens...

Al, fully expecting to discover having made something spectacularly bonehead
2 minutes after sending a patch out - happens often enough.

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

[Joe Perches]

On Tue, 2018-04-10 at 17:51 -0700, Kees Cook wrote:
> On Tue, Apr 10, 2018 at 5:33 PM, Al Viro  wrote:
> > On Tue, Apr 10, 2018 at 04:45:32PM -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
> > 
> > Do you even bother reading the feedback given to such patches?  I'm just
> > curious,
> 
> He made a tool mistake (sent v2 again) -- please see v4.

No worries but Kyle didn't just mistakenly resend v2.
It was a v3 with a different below the dashes changelog.

Kyle, it would be good if next time you were to add a
listing of changes in each revision below the dashes.

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

[Joe Perches]

On Tue, 2018-04-10 at 17:51 -0700, Kees Cook wrote:
> On Tue, Apr 10, 2018 at 5:33 PM, Al Viro  wrote:
> > On Tue, Apr 10, 2018 at 04:45:32PM -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
> > 
> > Do you even bother reading the feedback given to such patches?  I'm just
> > curious,
> 
> He made a tool mistake (sent v2 again) -- please see v4.

No worries but Kyle didn't just mistakenly resend v2.
It was a v3 with a different below the dashes changelog.

Kyle, it would be good if next time you were to add a
listing of changes in each revision below the dashes.

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

[Kees Cook]

On Tue, Apr 10, 2018 at 5:33 PM, Al Viro  wrote:
> On Tue, Apr 10, 2018 at 04:45:32PM -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
>
> Do you even bother reading the feedback given to such patches?  I'm just
> curious,

He made a tool mistake (sent v2 again) -- please see v4.

-Kees

-- 
Kees Cook
Pixel Security

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

[Kees Cook]

On Tue, Apr 10, 2018 at 5:33 PM, Al Viro  wrote:
> On Tue, Apr 10, 2018 at 04:45:32PM -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
>
> Do you even bother reading the feedback given to such patches?  I'm just
> curious,

He made a tool mistake (sent v2 again) -- please see v4.

-Kees

-- 
Kees Cook
Pixel Security

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

[Al Viro]

On Tue, Apr 10, 2018 at 04:45:32PM -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

Do you even bother reading the feedback given to such patches?  I'm just
curious,

> @@ -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;

... because I distinctly remember comments along the lines of "check what
other failure exits look like in there, such as
*errp = -EIO;
return 0;
nearby".

> @@ -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;

What, in your opinion, is going to unlock that page after that failure exit?

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

[Al Viro]

On Tue, Apr 10, 2018 at 04:45:32PM -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

Do you even bother reading the feedback given to such patches?  I'm just
curious,

> @@ -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;

... because I distinctly remember comments along the lines of "check what
other failure exits look like in there, such as
*errp = -EIO;
return 0;
nearby".

> @@ -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;

What, in your opinion, is going to unlock that page after that failure exit?

[PATCH v3] isofs compress: Remove VLA usage

[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 
---
Fixed build for sparc64
Fixed error recovery mistakes
---
 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 v3] isofs compress: Remove VLA usage

[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 
---
Fixed build for sparc64
Fixed error recovery mistakes
---
 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