* [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
@ 2014-06-25 18:17 Fabian Frederick
2014-06-25 18:27 ` Linus Torvalds
2014-06-25 19:05 ` Bob Copeland
0 siblings, 2 replies; 6+ messages in thread
From: Fabian Frederick @ 2014-06-25 18:17 UTC (permalink / raw)
To: linux-kernel; +Cc: Fabian Frederick, Bob Copeland, Andrew Morton
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
2014-06-25 18:17 [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc Fabian Frederick
@ 2014-06-25 18:27 ` Linus Torvalds
2014-06-25 19:02 ` Bob Copeland
2014-06-25 19:05 ` Bob Copeland
1 sibling, 1 reply; 6+ messages in thread
From: Linus Torvalds @ 2014-06-25 18:27 UTC (permalink / raw)
To: Fabian Frederick; +Cc: Linux Kernel Mailing List, Bob Copeland, Andrew Morton
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
2014-06-25 18:27 ` Linus Torvalds
@ 2014-06-25 19:02 ` Bob Copeland
2014-06-25 20:03 ` Fabian Frederick
0 siblings, 1 reply; 6+ messages in thread
From: Bob Copeland @ 2014-06-25 19:02 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Fabian Frederick, Linux Kernel Mailing List, Andrew Morton
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
2014-06-25 19:02 ` Bob Copeland
@ 2014-06-25 20:03 ` Fabian Frederick
2014-06-25 20:28 ` Bob Copeland
0 siblings, 1 reply; 6+ messages in thread
From: Fabian Frederick @ 2014-06-25 20:03 UTC (permalink / raw)
To: Bob Copeland, Linus Torvalds; +Cc: Andrew Morton, Linux Kernel Mailing List
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
2014-06-25 20:03 ` Fabian Frederick
@ 2014-06-25 20:28 ` Bob Copeland
0 siblings, 0 replies; 6+ messages in thread
From: Bob Copeland @ 2014-06-25 20:28 UTC (permalink / raw)
To: Fabian Frederick; +Cc: Linus Torvalds, Andrew Morton, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
2014-06-25 18:17 [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc Fabian Frederick
2014-06-25 18:27 ` Linus Torvalds
@ 2014-06-25 19:05 ` Bob Copeland
1 sibling, 0 replies; 6+ messages in thread
From: Bob Copeland @ 2014-06-25 19:05 UTC (permalink / raw)
To: Fabian Frederick; +Cc: linux-kernel, Andrew Morton
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-25 20:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-25 18:17 [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc Fabian Frederick
2014-06-25 18:27 ` Linus Torvalds
2014-06-25 19:02 ` Bob Copeland
2014-06-25 20:03 ` Fabian Frederick
2014-06-25 20:28 ` Bob Copeland
2014-06-25 19:05 ` Bob Copeland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox