| Message ID | 1343684862-13181-7-git-send-email-tytso@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 2012-07-30, at 14:47, Theodore Ts'o <tytso@mit.edu> wrote: > Since various parts of the library depend on the value of s_desc_size, > check to make sure it is the correct, expected value based on the file > system features. > > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > --- > lib/ext2fs/ext2_err.et.in | 3 +++ > lib/ext2fs/openfs.c | 15 +++++++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in > index ccf1894..5987185 100644 > --- a/lib/ext2fs/ext2_err.et.in > +++ b/lib/ext2fs/ext2_err.et.in > @@ -443,4 +443,7 @@ ec EXT2_ET_MMP_CHANGE_ABORT, > ec EXT2_ET_MMP_OPEN_DIRECT, > "MMP: open with O_DIRECT failed" > > +ec EXT2_ET_BAD_DESC_SIZE, > + "Block group descriptor size incorrect" > + > end > diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c > index 482e4ab..fbe9acd 100644 > --- a/lib/ext2fs/openfs.c > +++ b/lib/ext2fs/openfs.c > @@ -263,6 +263,21 @@ errcode_t ext2fs_open2(const char *name, const char *io_options, > retval = EXT2_ET_CORRUPT_SUPERBLOCK; > goto cleanup; > } > + > + /* Enforce the block group descriptor size */ > + if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) { > + if (fs->super->s_desc_size != EXT2_MIN_DESC_SIZE_64BIT) { It doesn't necessarily make sense to limit this to the minimum size, only that it is at least the minimum size. Cheers, Andreas > + retval = EXT2_ET_BAD_DESC_SIZE; > + goto cleanup; > + } > + } else { > + if (fs->super->s_desc_size && > + fs->super->s_desc_size != EXT2_MIN_DESC_SIZE) { > + retval = EXT2_ET_BAD_DESC_SIZE; > + goto cleanup; > + } > + } > + > fs->cluster_ratio_bits = fs->super->s_log_cluster_size - > fs->super->s_log_block_size; > if (EXT2_BLOCKS_PER_GROUP(fs->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 -- 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, Jul 31, 2012 at 11:38:47AM -0700, Andreas Dilger wrote: > > + /* Enforce the block group descriptor size */ > > + if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) { > > + if (fs->super->s_desc_size != EXT2_MIN_DESC_SIZE_64BIT) { > > It doesn't necessarily make sense to limit this to the minimum size, only that it is at least the minimum size. I'm not at all convinced that the ext2fs library will do the right thing if the block group size is larger than what we expect. At the very least we need to make sure it is a power of two, but even so I'd want to audit the code and do some experiments before I would hang my hat on this actually working correctly... - 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 2012-07-31, at 13:09, Theodore Ts'o <tytso@mit.edu> wrote: > On Tue, Jul 31, 2012 at 11:38:47AM -0700, Andreas Dilger wrote: >>> + /* Enforce the block group descriptor size */ >>> + if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) { >>> + if (fs->super->s_desc_size != EXT2_MIN_DESC_SIZE_64BIT) { >> >> It doesn't necessarily make sense to limit this to the minimum size, only that it is at least the minimum size. > > I'm not at all convinced that the ext2fs library will do the right > thing if the block group size is larger than what we expect. At the > very least we need to make sure it is a power of two, but even so I'd > want to audit the code and do some experiments before I would hang my > hat on this actually working correctly... I'm pretty sure that we've always checked that it is a power-of-two value. The problem with this change is that it makes it much harder to increase the size again in the future . It would definitely need another feature flag, but it isn't clear without more investigation (that i can't do on my phone) if a COMPAT flag would be enough (which we would likely have anyway if we needed a larger descriptor) or if we need a more restrictive feature flag. 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 Wed, Aug 01, 2012 at 01:45:32PM -0700, Andreas Dilger wrote: > > I'm pretty sure that we've always checked that it is a power-of-two > value. The problem with this change is that it makes it much harder > to increase the size again in the future . Unfortunately, no, there aren't any such checks anywhere in either the kernel or e2fsck. In fact, e2fsck wasn't checking s_desc_size at all (although it was depending on it), which is what caused me to get very worried... > It would definitely need another feature flag, but it isn't clear > without more investigation (that i can't do on my phone) if a COMPAT > flag would be enough (which we would likely have anyway if we needed > a larger descriptor) or if we need a more restrictive feature flag. It seems very likely that any new feature that would require us to expand the block group descriptor would require other changes to the file system's structures that would require a more restrictive feature flag.... - 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/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in index ccf1894..5987185 100644 --- a/lib/ext2fs/ext2_err.et.in +++ b/lib/ext2fs/ext2_err.et.in @@ -443,4 +443,7 @@ ec EXT2_ET_MMP_CHANGE_ABORT, ec EXT2_ET_MMP_OPEN_DIRECT, "MMP: open with O_DIRECT failed" +ec EXT2_ET_BAD_DESC_SIZE, + "Block group descriptor size incorrect" + end diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c index 482e4ab..fbe9acd 100644 --- a/lib/ext2fs/openfs.c +++ b/lib/ext2fs/openfs.c @@ -263,6 +263,21 @@ errcode_t ext2fs_open2(const char *name, const char *io_options, retval = EXT2_ET_CORRUPT_SUPERBLOCK; goto cleanup; } + + /* Enforce the block group descriptor size */ + if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) { + if (fs->super->s_desc_size != EXT2_MIN_DESC_SIZE_64BIT) { + retval = EXT2_ET_BAD_DESC_SIZE; + goto cleanup; + } + } else { + if (fs->super->s_desc_size && + fs->super->s_desc_size != EXT2_MIN_DESC_SIZE) { + retval = EXT2_ET_BAD_DESC_SIZE; + goto cleanup; + } + } + fs->cluster_ratio_bits = fs->super->s_log_cluster_size - fs->super->s_log_block_size; if (EXT2_BLOCKS_PER_GROUP(fs->super) !=
Since various parts of the library depend on the value of s_desc_size, check to make sure it is the correct, expected value based on the file system features. Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- lib/ext2fs/ext2_err.et.in | 3 +++ lib/ext2fs/openfs.c | 15 +++++++++++++++ 2 files changed, 18 insertions(+)