| Message ID | 1358068095-9034-2-git-send-email-wenqing.lz@taobao.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 2013-01-13, at 2:08 AM, Zheng Liu wrote: > Here we reduce the range of cluster-size and check s_blocks_per_group=0 to avoid 'division by zero' error. > > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com> > diff --git a/misc/mke2fs.c b/misc/mke2fs.c > index bf4d7a2..f4140a1 100644 > --- a/misc/mke2fs.c > +++ b/misc/mke2fs.c > @@ -1384,8 +1384,8 @@ profile_error: > break; > case 'C': > cluster_size = strtoul(optarg, &tmp, 0); > - if (cluster_size < EXT2_MIN_CLUSTER_SIZE || > - cluster_size > EXT2_MAX_CLUSTER_SIZE || *tmp) { > + if (cluster_size <= EXT2_MIN_CLUSTER_SIZE || > + cluster_size >= EXT2_MAX_CLUSTER_SIZE || *tmp) { > com_err(program_name, 0, > _("invalid cluster size - %s"), > optarg); Wouldn't it make more sense to change EXT2_MIN_CLUSTER_SIZE and EXT2_MAX_CLUSTER_SIZE? Otherwise, those constants don't really contain the min/max cluster size, and it is confusing to use them in other code. Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 13, 2013 at 05:08:14PM +0800, Zheng Liu wrote: > From: Zheng Liu <wenqing.lz@taobao.com> > > There are two bugs need to be fixed, which are about cluster-size. > Now the range of cluster-size is from 1024 to 512M bytes. Although > with '-C 1024', the cluster-size will be 4096 after making a > filesystem because in ext2fs_initialize() set_field() needs to check > 'param->s_log_cluster_size' and s_log_cluster_size is 0 as > cluster-size is 1024. Then s_log_cluster_size will be assigned to > s_log_block_size+4. So we never set cluster-size to 1024. > > Another bug is that when cluster-size is 512M EXT2FS_C2B will return > 0. So s_blocks_per_group will be assigned to zero and we will meet > a 'division by zero' error. There are a couple of things going on here. The first is that it makes no senes when the cluster size is less than or equal to the block size. (Actually, nothing bad should happen in the case when the cluster size == block size, but if the user specified the bigalloc feature, that's something which they almost certainly don't want.) So the more general check is we should be complaining if the cluster size is <= the block size. That is, the combination of -b 4096 and -C 2048 makes no sense, either. Also, there's technically nothing wrong with a cluster size of 512MB. The problem is in how we calculate the default number of clusters per group --- if it translates to a number of blocks per group which overals 2**32, that's when we run into problems. Which leads to another bug in the current mke2fs command. The range checking for the -g (which allows you to specify the number of blocks per group is bogus in the case when the bigalloc feature is enabled). I think the best way of fixing this is to document that the -g option specifies the number of clusters per block if the bigalloc feature is enabled. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-01-14, at 2:03 PM, Theodore Ts'o wrote: > On Sun, Jan 13, 2013 at 05:08:14PM +0800, Zheng Liu wrote: >> From: Zheng Liu <wenqing.lz@taobao.com> >> >> There are two bugs need to be fixed, which are about cluster-size. >> Now the range of cluster-size is from 1024 to 512M bytes. Although >> with '-C 1024', the cluster-size will be 4096 after making a >> filesystem because in ext2fs_initialize() set_field() needs to check >> 'param->s_log_cluster_size' and s_log_cluster_size is 0 as >> cluster-size is 1024. Then s_log_cluster_size will be assigned to >> s_log_block_size+4. So we never set cluster-size to 1024. >> >> Another bug is that when cluster-size is 512M EXT2FS_C2B will return >> 0. So s_blocks_per_group will be assigned to zero and we will meet >> a 'division by zero' error. > > There are a couple of things going on here. The first is that it > makes no senes when the cluster size is less than or equal to the > block size. (Actually, nothing bad should happen in the case when > cluster size == block size, but if the user specified the bigalloc > feature, that's something which they almost certainly don't want.) > > So the more general check is we should be complaining if the cluster > size is <= the block size. That is, the combination of -b 4096 and > -C 2048 makes no sense, either. > > Also, there's technically nothing wrong with a cluster size of 512MB. > The problem is in how we calculate the default number of clusters per > group --- if it translates to a number of blocks per group which > overals 2**32, that's when we run into problems. > > Which leads to another bug in the current mke2fs command. The range > checking for the -g (which allows you to specify the number of blocks > per group is bogus in the case when the bigalloc feature is enabled). > I think the best way of fixing this is to document that the -g option > specifies the number of clusters per block if the bigalloc feature is > enabled. Presumably you mean "the -g option specifies the number of clusters _per_group_ if the bigalloc feature is enabled"? Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 14, 2013 at 02:07:43PM -0700, Andreas Dilger wrote: > > I think the best way of fixing this is to document that the -g option > > specifies the number of clusters per block if the bigalloc feature is > > enabled. > > Presumably you mean "the -g option specifies the number of clusters > _per_group_ if the bigalloc feature is enabled"? Yes, I meant to say that I want to change the definition of the -g option such that if the bigalloc feature is enabled, it specifies the number of clusters per block group. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/lib/ext2fs/initialize.c b/lib/ext2fs/initialize.c index dca0d9a..0eccd59 100644 --- a/lib/ext2fs/initialize.c +++ b/lib/ext2fs/initialize.c @@ -226,6 +226,10 @@ errcode_t ext2fs_initialize(const char *name, int flags, super->s_clusters_per_group = EXT2_MAX_CLUSTERS_PER_GROUP(super); super->s_blocks_per_group = EXT2FS_C2B(fs, super->s_clusters_per_group); + if (super->s_blocks_per_group == 0) { + retval = EXT2_ET_TOOSMALL; + goto cleanup; + } } else { set_field(s_blocks_per_group, fs->blocksize * 8); if (super->s_blocks_per_group > EXT2_MAX_BLOCKS_PER_GROUP(super)) diff --git a/misc/mke2fs.c b/misc/mke2fs.c index bf4d7a2..f4140a1 100644 --- a/misc/mke2fs.c +++ b/misc/mke2fs.c @@ -1384,8 +1384,8 @@ profile_error: break; case 'C': cluster_size = strtoul(optarg, &tmp, 0); - if (cluster_size < EXT2_MIN_CLUSTER_SIZE || - cluster_size > EXT2_MAX_CLUSTER_SIZE || *tmp) { + if (cluster_size <= EXT2_MIN_CLUSTER_SIZE || + cluster_size >= EXT2_MAX_CLUSTER_SIZE || *tmp) { com_err(program_name, 0, _("invalid cluster size - %s"), optarg);