Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
> Le 25 juin 2014 à 21:02, Bob Copeland 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
On Wed, Jun 25, 2014 at 08:17:17PM +0200, Fabian Frederick wrote: > kcalloc manages count*sizeof overflow. > > Cc: Bob Copeland > Cc: Andrew Morton > Signed-off-by: Fabian Frederick Acked-by: Bob Copeland -- Bob Copeland %% www.bobcopeland.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
On Wed, Jun 25, 2014 at 11:27:21AM -0700, Linus Torvalds wrote: > On Wed, Jun 25, 2014 at 11:17 AM, Fabian Frederick 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
On Wed, Jun 25, 2014 at 11:17 AM, Fabian Frederick 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
kcalloc manages count*sizeof overflow. Cc: Bob Copeland Cc: Andrew Morton Signed-off-by: Fabian Frederick --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
kcalloc manages count*sizeof overflow. Cc: Bob Copeland m...@bobcopeland.com Cc: Andrew Morton a...@linux-foundation.org Signed-off-by: Fabian Frederick f...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
On Wed, Jun 25, 2014 at 11:17 AM, Fabian Frederick f...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
On Wed, Jun 25, 2014 at 11:27:21AM -0700, Linus Torvalds wrote: On Wed, Jun 25, 2014 at 11:17 AM, Fabian Frederick f...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
On Wed, Jun 25, 2014 at 08:17:17PM +0200, Fabian Frederick wrote: kcalloc manages count*sizeof overflow. Cc: Bob Copeland m...@bobcopeland.com Cc: Andrew Morton a...@linux-foundation.org Signed-off-by: Fabian Frederick f...@skynet.be Acked-by: Bob Copeland m...@bobcopeland.com -- Bob Copeland %% www.bobcopeland.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
Le 25 juin 2014 à 21:02, Bob Copeland m...@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 f...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] fs/omfs/inode.c: replace count*size kzalloc by kcalloc
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
