Patchwork [2/3] mke2fs: reduce the range of cluster-size

login
register
mail settings
Submitter Zheng Liu
Date Jan. 13, 2013, 9:08 a.m.
Message ID <1358068095-9034-2-git-send-email-wenqing.lz@taobao.com>
Download mbox | patch
Permalink /patch/211603/
State Superseded
Headers show

Comments

Zheng Liu - Jan. 13, 2013, 9:08 a.m.
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.

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>
---
 lib/ext2fs/initialize.c | 4 ++++
 misc/mke2fs.c           | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)
Andreas Dilger - Jan. 14, 2013, 5:41 p.m.
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
Theodore Ts'o - Jan. 14, 2013, 9:03 p.m.
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
Andreas Dilger - Jan. 14, 2013, 9:07 p.m.
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
Theodore Ts'o - Jan. 14, 2013, 9:10 p.m.
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

Patch

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