[PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc

[PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc

[Fabian Frederick]

kcalloc manages count*sizeof overflow.

Cc: Bob Copeland <me@bobcopeland.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/omfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/omfs/inode.c b/fs/omfs/inode.c
index ec58c76..ba88197 100644
--- a/fs/omfs/inode.c
+++ b/fs/omfs/inode.c
@@ -321,7 +321,7 @@ static int omfs_get_imap(struct super_block *sb)
 		goto out;
 
 	sbi->s_imap_size = array_size;
-	sbi->s_imap = kzalloc(array_size * sizeof(unsigned long *), GFP_KERNEL);
+	sbi->s_imap = kcalloc(array_size, sizeof(unsigned long *), GFP_KERNEL);
 	if (!sbi->s_imap)
 		goto nomem;
 
-- 
1.9.1

Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc

[Linus Torvalds]

On Wed, Jun 25, 2014 at 11:17 AM, Fabian Frederick <fabf@skynet.be> wrote:
> kcalloc manages count*sizeof overflow.

As far as I can tell, any overflow has happened long before, in

    bitmap_size = DIV_ROUND_UP(sbi->s_num_blocks, 8);

where 'sbi->s_num_blocks' i san u64, and 'bitmap_size' is an 'int'.

I don't think the patch is necessarily a bad thing, but I think it
might be more important to sanity-check that part instead.

              Linus

Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc

[Bob Copeland]

On Wed, Jun 25, 2014 at 11:27:21AM -0700, Linus Torvalds wrote:
> On Wed, Jun 25, 2014 at 11:17 AM, Fabian Frederick <fabf@skynet.be> wrote:
> > kcalloc manages count*sizeof overflow.
> 
> As far as I can tell, any overflow has happened long before, in
> 
>     bitmap_size = DIV_ROUND_UP(sbi->s_num_blocks, 8);
> 
> where 'sbi->s_num_blocks' i san u64, and 'bitmap_size' is an 'int'.
> 
> I don't think the patch is necessarily a bad thing, but I think it
> might be more important to sanity-check that part instead.

Agreed - even though the FS data structures support 64-bit block
count, I've never seen an OMFS fs with more than about 2M blocks
(typical device had 20 gigs w/ 8k blocks).  So it would make
sense to bail in omfs_fill_super if that number is greater than
2^31 or so.

(I am fine with the kcalloc patch too, though.)

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc

[Bob Copeland]

On Wed, Jun 25, 2014 at 08:17:17PM +0200, Fabian Frederick wrote:
> kcalloc manages count*sizeof overflow.
> 
> Cc: Bob Copeland <me@bobcopeland.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Fabian Frederick <fabf@skynet.be>

Acked-by: Bob Copeland <me@bobcopeland.com>

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc

[Fabian Frederick]

> Le 25 juin 2014 à 21:02, Bob Copeland <me@bobcopeland.com> a écrit :
>
>
> On Wed, Jun 25, 2014 at 11:27:21AM -0700, Linus Torvalds wrote:
> > On Wed, Jun 25, 2014 at 11:17 AM, Fabian Frederick <fabf@skynet.be> wrote:
> > > kcalloc manages count*sizeof overflow.
> >
> > As far as I can tell, any overflow has happened long before, in
> >
> >     bitmap_size = DIV_ROUND_UP(sbi->s_num_blocks, 8);
> >
> > where 'sbi->s_num_blocks' i san u64, and 'bitmap_size' is an 'int'.
> >
> > I don't think the patch is necessarily a bad thing, but I think it
> > might be more important to sanity-check that part instead.
>
> Agreed - even though the FS data structures support 64-bit block
> count, I've never seen an OMFS fs with more than about 2M blocks
> (typical device had 20 gigs w/ 8k blocks).  So it would make
> sense to bail in omfs_fill_super if that number is greater than
> 2^31 or so.
We could use unsigned int for bitmap instead of int or simply u64 ?
 
>
> (I am fine with the kcalloc patch too, though.)

>
> --
> Bob Copeland %% www.bobcopeland.com

Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc

[Bob Copeland]

On Wed, Jun 25, 2014 at 10:03:26PM +0200, Fabian Frederick wrote:
> > >     bitmap_size = DIV_ROUND_UP(sbi->s_num_blocks, 8);
> > >
> > Agreed - even though the FS data structures support 64-bit block
> > count, I've never seen an OMFS fs with more than about 2M blocks
> > (typical device had 20 gigs w/ 8k blocks).  So it would make
> > sense to bail in omfs_fill_super if that number is greater than
> > 2^31 or so.
> We could use unsigned int for bitmap instead of int or simply u64 ?

It doesn't really make sense to be a signed int, sure -- but even so
making it a u64 without at least including a sanity check is probably
not the way to go.

OMFS allocates space for the entire free-space bitmap in memory, rather
than loading its blocks on demand.  That's admittedly pretty dumb, but I
did it so that I could eventually support those FSes without a free-space
bitmap (I've never been asked for that feature, though, and didn't have
ReplayTV myself, so I don't believe that actually happened).

If s_num_blocks won't fit in a u32, well then that's a pretty huge chunk of
memory to allocate, and would represent a disk much bigger than the ones
that were available when this FS was used on a few devices.

(As for why the designers used u64 for all data structures, I guess just
optimism?)

-- 
Bob Copeland %% www.bobcopeland.com