Message ID | 1358210232-30578-5-git-send-email-tytso@mit.edu |
---|---|
State | Accepted, archived |
Headers | show |
On Mon, Jan 14, 2013 at 07:37:12PM -0500, Theodore Ts'o wrote: > If the user attemps to create a 512MB cluster, we need to adjust the > defaults to avoid a 32-bit overflow of s_blocks_per_group. Also check > to make sure that the caller of ext2fs_initialize() has not given a > value of s_clusters_per_group that would result in an overflow of > s_blocks_per_group. > > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> The patch itself looks good to me. Feel free to add: Reviewed-by: Zheng Liu <wenqing.lz@taobao.com> FWIW, I wonder why we need to add such complex logical to handle a corner case. I guess no one wants to use a 512MB cluster. So changing max cluster size from 512MB to 256MB is very simple and straightfoward. Regards, - Zheng > --- > lib/ext2fs/initialize.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/lib/ext2fs/initialize.c b/lib/ext2fs/initialize.c > index b0c15d2..5afdc27 100644 > --- a/lib/ext2fs/initialize.c > +++ b/lib/ext2fs/initialize.c > @@ -207,6 +207,8 @@ errcode_t ext2fs_initialize(const char *name, int flags, > super->s_log_block_size; > > if (bigalloc_flag) { > + unsigned long long bpg; > + > if (param->s_blocks_per_group && > param->s_clusters_per_group && > ((param->s_clusters_per_group * EXT2FS_CLUSTER_RATIO(fs)) != > @@ -220,12 +222,19 @@ errcode_t ext2fs_initialize(const char *name, int flags, > super->s_clusters_per_group = > param->s_blocks_per_group / > EXT2FS_CLUSTER_RATIO(fs); > - else > + else if (super->s_log_cluster_size + 15 < 32) > super->s_clusters_per_group = fs->blocksize * 8; > + else > + super->s_clusters_per_group = (fs->blocksize - 1) * 8; > if (super->s_clusters_per_group > EXT2_MAX_CLUSTERS_PER_GROUP(super)) > super->s_clusters_per_group = EXT2_MAX_CLUSTERS_PER_GROUP(super); > - super->s_blocks_per_group = EXT2FS_C2B(fs, > - super->s_clusters_per_group); > + bpg = EXT2FS_C2B(fs, > + (unsigned long long) super->s_clusters_per_group); > + if (bpg >= (((unsigned long long) 1) << 32)) { > + retval = EXT2_ET_INVALID_ARGUMENT; > + goto cleanup; > + } > + super->s_blocks_per_group = bpg; > } else { > set_field(s_blocks_per_group, fs->blocksize * 8); > if (super->s_blocks_per_group > EXT2_MAX_BLOCKS_PER_GROUP(super)) > -- > 1.7.12.rc0.22.gcdd159b > -- 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 Tue, Jan 15, 2013 at 11:33:31PM +0800, Zheng Liu wrote: > On Mon, Jan 14, 2013 at 07:37:12PM -0500, Theodore Ts'o wrote: > > If the user attemps to create a 512MB cluster, we need to adjust the > > defaults to avoid a 32-bit overflow of s_blocks_per_group. Also check > > to make sure that the caller of ext2fs_initialize() has not given a > > value of s_clusters_per_group that would result in an overflow of > > s_blocks_per_group. > > > > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > > The patch itself looks good to me. Feel free to add: > Reviewed-by: Zheng Liu <wenqing.lz@taobao.com> Sorry, forgot to say that '-C' option in manpage will need to be modified from 256MB to 512MB if this patch is applied. Thanks, - Zheng -- 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 Tue, Jan 15, 2013 at 11:33:31PM +0800, Zheng Liu wrote: > > FWIW, I wonder why we need to add such complex logical to handle a > corner case. I guess no one wants to use a 512MB cluster. So changing > max cluster size from 512MB to 256MB is very simple and straightfoward. I agree that it seems very unlikely that there would be much interest in using a 512MB cluster. However, we would still need to do the check in the case of a 16k block size (on an PowerPC or Itanium system) at a 256MB or 128MB cluster. So part of this check would be needed anyway. - 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 Tue, Jan 15, 2013 at 02:10:30PM -0500, Theodore Ts'o wrote: > On Tue, Jan 15, 2013 at 11:33:31PM +0800, Zheng Liu wrote: > > > > FWIW, I wonder why we need to add such complex logical to handle a > > corner case. I guess no one wants to use a 512MB cluster. So changing > > max cluster size from 512MB to 256MB is very simple and straightfoward. > > I agree that it seems very unlikely that there would be much interest > in using a 512MB cluster. However, we would still need to do the > check in the case of a 16k block size (on an PowerPC or Itanium > system) at a 256MB or 128MB cluster. So part of this check would be > needed anyway. Yeah, I see. Thanks for the explanation. Regards, - Zheng -- 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 b0c15d2..5afdc27 100644 --- a/lib/ext2fs/initialize.c +++ b/lib/ext2fs/initialize.c @@ -207,6 +207,8 @@ errcode_t ext2fs_initialize(const char *name, int flags, super->s_log_block_size; if (bigalloc_flag) { + unsigned long long bpg; + if (param->s_blocks_per_group && param->s_clusters_per_group && ((param->s_clusters_per_group * EXT2FS_CLUSTER_RATIO(fs)) != @@ -220,12 +222,19 @@ errcode_t ext2fs_initialize(const char *name, int flags, super->s_clusters_per_group = param->s_blocks_per_group / EXT2FS_CLUSTER_RATIO(fs); - else + else if (super->s_log_cluster_size + 15 < 32) super->s_clusters_per_group = fs->blocksize * 8; + else + super->s_clusters_per_group = (fs->blocksize - 1) * 8; if (super->s_clusters_per_group > EXT2_MAX_CLUSTERS_PER_GROUP(super)) super->s_clusters_per_group = EXT2_MAX_CLUSTERS_PER_GROUP(super); - super->s_blocks_per_group = EXT2FS_C2B(fs, - super->s_clusters_per_group); + bpg = EXT2FS_C2B(fs, + (unsigned long long) super->s_clusters_per_group); + if (bpg >= (((unsigned long long) 1) << 32)) { + retval = EXT2_ET_INVALID_ARGUMENT; + goto cleanup; + } + super->s_blocks_per_group = bpg; } else { set_field(s_blocks_per_group, fs->blocksize * 8); if (super->s_blocks_per_group > EXT2_MAX_BLOCKS_PER_GROUP(super))
If the user attemps to create a 512MB cluster, we need to adjust the defaults to avoid a 32-bit overflow of s_blocks_per_group. Also check to make sure that the caller of ext2fs_initialize() has not given a value of s_clusters_per_group that would result in an overflow of s_blocks_per_group. Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- lib/ext2fs/initialize.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)