with the patch all headers are checked. the code should become
more resistant to on-disk corruptions. needless BUG_ON() have
been removed. please, review for inclusion.
Signed-off-by: Alex Tomas <[email protected]>
Signed-off-by: Mingming Cao <[email protected]>
Index: linux-2.6.22-rc4/fs/ext4/extents.c
===================================================================
--- linux-2.6.22-rc4.orig/fs/ext4/extents.c 2007-06-11 17:22:15.000000000 -0700
+++ linux-2.6.22-rc4/fs/ext4/extents.c 2007-06-11 17:27:57.000000000 -0700
@@ -91,36 +91,6 @@
ix->ei_leaf_hi = cpu_to_le16((unsigned long) ((pb >> 31) >> 1) & 0xffff);
}
-static int ext4_ext_check_header(const char *function, struct inode *inode,
- struct ext4_extent_header *eh)
-{
- const char *error_msg = NULL;
-
- if (unlikely(eh->eh_magic != EXT4_EXT_MAGIC)) {
- error_msg = "invalid magic";
- goto corrupted;
- }
- if (unlikely(eh->eh_max == 0)) {
- error_msg = "invalid eh_max";
- goto corrupted;
- }
- if (unlikely(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max))) {
- error_msg = "invalid eh_entries";
- goto corrupted;
- }
- return 0;
-
-corrupted:
- ext4_error(inode->i_sb, function,
- "bad header in inode #%lu: %s - magic %x, "
- "entries %u, max %u, depth %u",
- inode->i_ino, error_msg, le16_to_cpu(eh->eh_magic),
- le16_to_cpu(eh->eh_entries), le16_to_cpu(eh->eh_max),
- le16_to_cpu(eh->eh_depth));
-
- return -EIO;
-}
-
static handle_t *ext4_ext_journal_restart(handle_t *handle, int needed)
{
int err;
@@ -269,6 +239,70 @@
return size;
}
+static inline int
+ext4_ext_max_entries(struct inode *inode, int depth)
+{
+ int max;
+
+ if (depth == ext_depth(inode)) {
+ if (depth == 0)
+ max = ext4_ext_space_root(inode);
+ else
+ max = ext4_ext_space_root_idx(inode);
+ } else {
+ if (depth == 0)
+ max = ext4_ext_space_block(inode);
+ else
+ max = ext4_ext_space_block_idx(inode);
+ }
+
+ return max;
+}
+
+static int __ext4_ext_check_header(const char *function, struct inode *inode,
+ struct ext4_extent_header *eh,
+ int depth)
+{
+ const char *error_msg = NULL;
+ int max = 0;
+
+ if (unlikely(eh->eh_magic != EXT4_EXT_MAGIC)) {
+ error_msg = "invalid magic";
+ goto corrupted;
+ }
+ if (unlikely(le16_to_cpu(eh->eh_depth) != depth)) {
+ error_msg = "unexpected eh_depth";
+ goto corrupted;
+ }
+ if (unlikely(eh->eh_max == 0)) {
+ error_msg = "invalid eh_max";
+ goto corrupted;
+ }
+ max = ext4_ext_max_entries(inode, depth);
+ if (unlikely(le16_to_cpu(eh->eh_max) > max)) {
+ error_msg = "too large eh_max";
+ goto corrupted;
+ }
+ if (unlikely(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max))) {
+ error_msg = "invalid eh_entries";
+ goto corrupted;
+ }
+ return 0;
+
+corrupted:
+ ext4_error(inode->i_sb, function,
+ "bad header in inode #%lu: %s - magic %x, "
+ "entries %u, max %u(%u), depth %u(%u)",
+ inode->i_ino, error_msg, le16_to_cpu(eh->eh_magic),
+ le16_to_cpu(eh->eh_entries), le16_to_cpu(eh->eh_max),
+ max, le16_to_cpu(eh->eh_depth), depth);
+
+ return -EIO;
+}
+
+#define ext4_ext_check_header(inode, eh, depth) \
+ __ext4_ext_check_header(__FUNCTION__, inode, eh, depth)
+
#ifdef EXT_DEBUG
static void ext4_ext_show_path(struct inode *inode, struct ext4_ext_path *path)
{
@@ -329,6 +363,7 @@
/*
* ext4_ext_binsearch_idx:
* binary search for the closest index of the given block
+ * the header must be checked before calling this
*/
static void
ext4_ext_binsearch_idx(struct inode *inode, struct ext4_ext_path *path, int block)
@@ -336,9 +371,6 @@
struct ext4_extent_header *eh = path->p_hdr;
struct ext4_extent_idx *r, *l, *m;
- BUG_ON(eh->eh_magic != EXT4_EXT_MAGIC);
- BUG_ON(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max));
- BUG_ON(le16_to_cpu(eh->eh_entries) <= 0);
ext_debug("binsearch for %d(idx): ", block);
@@ -388,6 +420,7 @@
/*
* ext4_ext_binsearch:
* binary search for closest extent of the given block
+ * the header must be checked before calling this
*/
static void
ext4_ext_binsearch(struct inode *inode, struct ext4_ext_path *path, int block)
@@ -395,9 +428,6 @@
struct ext4_extent_header *eh = path->p_hdr;
struct ext4_extent *r, *l, *m;
- BUG_ON(eh->eh_magic != EXT4_EXT_MAGIC);
- BUG_ON(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max));
-
if (eh->eh_entries == 0) {
/*
* this leaf is empty:
@@ -468,11 +498,10 @@
short int depth, i, ppos = 0, alloc = 0;
eh = ext_inode_hdr(inode);
- BUG_ON(eh == NULL);
- if (ext4_ext_check_header(__FUNCTION__, inode, eh))
+ i = depth = ext_depth(inode);
+ if (ext4_ext_check_header(inode, eh, depth))
return ERR_PTR(-EIO);
- i = depth = ext_depth(inode);
/* account possible depth increase */
if (!path) {
@@ -488,6 +517,7 @@
while (i) {
ext_debug("depth %d: num %d, max %d\n",
ppos, le16_to_cpu(eh->eh_entries), le16_to_cpu(eh->eh_max));
+
ext4_ext_binsearch_idx(inode, path + ppos, block);
path[ppos].p_block = idx_pblock(path[ppos].p_idx);
path[ppos].p_depth = i;
@@ -504,7 +534,7 @@
path[ppos].p_hdr = eh;
i--;
- if (ext4_ext_check_header(__FUNCTION__, inode, eh))
+ if (ext4_ext_check_header(inode, eh, i))
goto err;
}
@@ -513,9 +543,6 @@
path[ppos].p_ext = NULL;
path[ppos].p_idx = NULL;
- if (ext4_ext_check_header(__FUNCTION__, inode, eh))
- goto err;
-
/* find extent */
ext4_ext_binsearch(inode, path + ppos, block);
@@ -1673,13 +1700,12 @@
unsigned short ex_ee_len;
struct ext4_extent *ex;
+ /* the header must be checked already in ext4_ext_remove_space() */
ext_debug("truncate since %lu in leaf\n", start);
if (!path[depth].p_hdr)
path[depth].p_hdr = ext_block_hdr(path[depth].p_bh);
eh = path[depth].p_hdr;
BUG_ON(eh == NULL);
- BUG_ON(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max));
- BUG_ON(eh->eh_magic != EXT4_EXT_MAGIC);
/* find where to start removing */
ex = EXT_LAST_EXTENT(eh);
@@ -1825,7 +1851,7 @@
return -ENOMEM;
}
path[0].p_hdr = ext_inode_hdr(inode);
- if (ext4_ext_check_header(__FUNCTION__, inode, path[0].p_hdr)) {
+ if (ext4_ext_check_header(inode, path[0].p_hdr, depth)) {
err = -EIO;
goto out;
}
@@ -1846,17 +1872,8 @@
if (!path[i].p_hdr) {
ext_debug("initialize header\n");
path[i].p_hdr = ext_block_hdr(path[i].p_bh);
- if (ext4_ext_check_header(__FUNCTION__, inode,
- path[i].p_hdr)) {
- err = -EIO;
- goto out;
- }
}
- BUG_ON(le16_to_cpu(path[i].p_hdr->eh_entries)
- > le16_to_cpu(path[i].p_hdr->eh_max));
- BUG_ON(path[i].p_hdr->eh_magic != EXT4_EXT_MAGIC);
-
if (!path[i].p_idx) {
/* this level hasn't been touched yet */
path[i].p_idx = EXT_LAST_INDEX(path[i].p_hdr);
@@ -1873,17 +1890,24 @@
i, EXT_FIRST_INDEX(path[i].p_hdr),
path[i].p_idx);
if (ext4_ext_more_to_rm(path + i)) {
+ struct buffer_head *bh;
/* go to the next level */
ext_debug("move to level %d (block %llu)\n",
i + 1, idx_pblock(path[i].p_idx));
memset(path + i + 1, 0, sizeof(*path));
- path[i+1].p_bh =
- sb_bread(sb, idx_pblock(path[i].p_idx));
- if (!path[i+1].p_bh) {
+ bh = sb_bread(sb, idx_pblock(path[i].p_idx));
+ if (!bh) {
/* should we reset i_size? */
err = -EIO;
break;
}
+ BUG_ON(i + 1 > depth);
+ if (ext4_ext_check_header(inode, ext_block_hdr(bh),
+ depth - i - 1)) {
+ err = -EIO;
+ break;
+ }
+ path[i+1].p_bh = bh;
/* save actual number of indexes since this
* number is changed at the next iteration */
On Sun, 01 Jul 2007 03:36:22 -0400
Mingming Cao <[email protected]> wrote:
> with the patch all headers are checked. the code should become
> more resistant to on-disk corruptions. needless BUG_ON() have
> been removed. please, review for inclusion.
>
> ...
> @@ -269,6 +239,70 @@
> return size;
> }
>
> +static inline int
> +ext4_ext_max_entries(struct inode *inode, int depth)
Please remove the `inline'.
`inline' is usually wrong. It is wrong here. If this function has a
single callsite (it has) then the compiler will inline it. If the function
later gains more callsites then the compiler will know (correctly) not to
inline it.
We hope. If we find the compiler still inlines a function as large as this
one then we'd need to use noinline and complain at the gcc developers.
> +{
> + int max;
> +
> + if (depth == ext_depth(inode)) {
> + if (depth == 0)
> + max = ext4_ext_space_root(inode);
> + else
> + max = ext4_ext_space_root_idx(inode);
> + } else {
> + if (depth == 0)
> + max = ext4_ext_space_block(inode);
> + else
> + max = ext4_ext_space_block_idx(inode);
> + }
> +
> + return max;
> +}
> +
> +static int __ext4_ext_check_header(const char *function, struct inode *inode,
> + struct ext4_extent_header *eh,
> + int depth)
> +{
> + const char *error_msg = NULL;
Unneeded initialisation.
> + int max = 0;
> +
> + if (unlikely(eh->eh_magic != EXT4_EXT_MAGIC)) {
> + error_msg = "invalid magic";
> + goto corrupted;
> + }
> + if (unlikely(le16_to_cpu(eh->eh_depth) != depth)) {
> + error_msg = "unexpected eh_depth";
> + goto corrupted;
> + }
> + if (unlikely(eh->eh_max == 0)) {
> + error_msg = "invalid eh_max";
> + goto corrupted;
> + }
> + max = ext4_ext_max_entries(inode, depth);
> + if (unlikely(le16_to_cpu(eh->eh_max) > max)) {
> + error_msg = "too large eh_max";
> + goto corrupted;
> + }
> + if (unlikely(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max))) {
> + error_msg = "invalid eh_entries";
> + goto corrupted;
> + }
> + return 0;
> +
> +corrupted:
> + ext4_error(inode->i_sb, function,
> + "bad header in inode #%lu: %s - magic %x, "
> + "entries %u, max %u(%u), depth %u(%u)",
> + inode->i_ino, error_msg, le16_to_cpu(eh->eh_magic),
> + le16_to_cpu(eh->eh_entries), le16_to_cpu(eh->eh_max),
> + max, le16_to_cpu(eh->eh_depth), depth);
> +
> + return -EIO;
> +}
> +
>
> ...
>
> + i = depth = ext_depth(inode);
>
Mulitple assignments like this are somewhat unpopular from the coding-style
POV.
We have a local variable called `i' which is not used as a simple counter
and which has no explanatory comment. This is plain bad. Please find a
better name for this variable. Review the code for other such lack of
clarity - I'm sure there's more.
> - BUG_ON(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max));
> - BUG_ON(eh->eh_magic != EXT4_EXT_MAGIC);
Yeah, this patch improves things a lot.
How well-tested is it? Have any of these errors actually been triggered?
> path[i].p_hdr = ext_block_hdr(path[i].p_bh);
> - if (ext4_ext_check_header(__FUNCTION__, inode,
> - path[i].p_hdr)) {
> - err = -EIO;
> - goto out;
> - }
> }
>
> - BUG_ON(le16_to_cpu(path[i].p_hdr->eh_entries)
> - > le16_to_cpu(path[i].p_hdr->eh_max));
> - BUG_ON(path[i].p_hdr->eh_magic != EXT4_EXT_MAGIC);
> -
> if (!path[i].p_idx) {
> /* this level hasn't been touched yet */
> path[i].p_idx = EXT_LAST_INDEX(path[i].p_hdr);
> @@ -1873,17 +1890,24 @@
> i, EXT_FIRST_INDEX(path[i].p_hdr),
> path[i].p_idx);
> if (ext4_ext_more_to_rm(path + i)) {
> + struct buffer_head *bh;
> /* go to the next level */
> ext_debug("move to level %d (block %llu)\n",
> i + 1, idx_pblock(path[i].p_idx));
> memset(path + i + 1, 0, sizeof(*path));
> - path[i+1].p_bh =
> - sb_bread(sb, idx_pblock(path[i].p_idx));
> - if (!path[i+1].p_bh) {
> + bh = sb_bread(sb, idx_pblock(path[i].p_idx));
> + if (!bh) {
> /* should we reset i_size? */
> err = -EIO;
> break;
> }
> + BUG_ON(i + 1 > depth);
ouch. Couldn't we do WARN_ON then return -EIO here?
> + if (ext4_ext_check_header(inode, ext_block_hdr(bh),
> + depth - i - 1)) {
> + err = -EIO;
> + break;
> + }
> + path[i+1].p_bh = bh;
Really that should have been "i + 1". checkpatch misses this. It seems to
be missing more stuff that it used to lately.
Andrew Morton wrote:
> On Sun, 01 Jul 2007 03:36:22 -0400
> Mingming Cao <[email protected]> wrote:
>
>> with the patch all headers are checked. the code should become
>> more resistant to on-disk corruptions. needless BUG_ON() have
>> been removed. please, review for inclusion.
>>
>> ...
>
>> @@ -269,6 +239,70 @@
>> return size;
>> }
>>
>> +static inline int
>> +ext4_ext_max_entries(struct inode *inode, int depth)
>
> Please remove the `inline'.
>
> `inline' is usually wrong. It is wrong here. If this function has a
> single callsite (it has) then the compiler will inline it. If the function
> later gains more callsites then the compiler will know (correctly) not to
> inline it.
>
> We hope. If we find the compiler still inlines a function as large as this
> one then we'd need to use noinline and complain at the gcc developers.
>
>> +{
>> + int max;
>> +
>> + if (depth == ext_depth(inode)) {
>> + if (depth == 0)
>> + max = ext4_ext_space_root(inode);
>> + else
>> + max = ext4_ext_space_root_idx(inode);
>> + } else {
>> + if (depth == 0)
>> + max = ext4_ext_space_block(inode);
>> + else
>> + max = ext4_ext_space_block_idx(inode);
>> + }
>> +
>> + return max;
>> +}
>> +
>> +static int __ext4_ext_check_header(const char *function, struct inode *inode,
>> + struct ext4_extent_header *eh,
>> + int depth)
>> +{
>> + const char *error_msg = NULL;
>
> Unneeded initialisation.
>
>> + int max = 0;
>> +
>> + if (unlikely(eh->eh_magic != EXT4_EXT_MAGIC)) {
>> + error_msg = "invalid magic";
>> + goto corrupted;
>> + }
>> + if (unlikely(le16_to_cpu(eh->eh_depth) != depth)) {
>> + error_msg = "unexpected eh_depth";
>> + goto corrupted;
>> + }
>> + if (unlikely(eh->eh_max == 0)) {
>> + error_msg = "invalid eh_max";
>> + goto corrupted;
>> + }
>> + max = ext4_ext_max_entries(inode, depth);
>> + if (unlikely(le16_to_cpu(eh->eh_max) > max)) {
>> + error_msg = "too large eh_max";
>> + goto corrupted;
>> + }
>> + if (unlikely(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max))) {
>> + error_msg = "invalid eh_entries";
>> + goto corrupted;
>> + }
>> + return 0;
>> +
>> +corrupted:
>> + ext4_error(inode->i_sb, function,
>> + "bad header in inode #%lu: %s - magic %x, "
>> + "entries %u, max %u(%u), depth %u(%u)",
>> + inode->i_ino, error_msg, le16_to_cpu(eh->eh_magic),
>> + le16_to_cpu(eh->eh_entries), le16_to_cpu(eh->eh_max),
>> + max, le16_to_cpu(eh->eh_depth), depth);
>> +
>> + return -EIO;
>> +}
>> +
>>
>> ...
>>
>> + i = depth = ext_depth(inode);
>>
>
> Mulitple assignments like this are somewhat unpopular from the coding-style
> POV.
>
> We have a local variable called `i' which is not used as a simple counter
> and which has no explanatory comment. This is plain bad. Please find a
> better name for this variable. Review the code for other such lack of
> clarity - I'm sure there's more.
>
>
>> - BUG_ON(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max));
>> - BUG_ON(eh->eh_magic != EXT4_EXT_MAGIC);
>
> Yeah, this patch improves things a lot.
>
> How well-tested is it? Have any of these errors actually been triggered?
>
>> path[i].p_hdr = ext_block_hdr(path[i].p_bh);
>> - if (ext4_ext_check_header(__FUNCTION__, inode,
>> - path[i].p_hdr)) {
>> - err = -EIO;
>> - goto out;
>> - }
>> }
>>
>> - BUG_ON(le16_to_cpu(path[i].p_hdr->eh_entries)
>> - > le16_to_cpu(path[i].p_hdr->eh_max));
>> - BUG_ON(path[i].p_hdr->eh_magic != EXT4_EXT_MAGIC);
>> -
>> if (!path[i].p_idx) {
>> /* this level hasn't been touched yet */
>> path[i].p_idx = EXT_LAST_INDEX(path[i].p_hdr);
>> @@ -1873,17 +1890,24 @@
>> i, EXT_FIRST_INDEX(path[i].p_hdr),
>> path[i].p_idx);
>> if (ext4_ext_more_to_rm(path + i)) {
>> + struct buffer_head *bh;
>> /* go to the next level */
>> ext_debug("move to level %d (block %llu)\n",
>> i + 1, idx_pblock(path[i].p_idx));
>> memset(path + i + 1, 0, sizeof(*path));
>> - path[i+1].p_bh =
>> - sb_bread(sb, idx_pblock(path[i].p_idx));
>> - if (!path[i+1].p_bh) {
>> + bh = sb_bread(sb, idx_pblock(path[i].p_idx));
>> + if (!bh) {
>> /* should we reset i_size? */
>> err = -EIO;
>> break;
>> }
>> + BUG_ON(i + 1 > depth);
>
> ouch. Couldn't we do WARN_ON then return -EIO here?
>
>> + if (ext4_ext_check_header(inode, ext_block_hdr(bh),
>> + depth - i - 1)) {
>> + err = -EIO;
>> + break;
>> + }
>> + path[i+1].p_bh = bh;
>
> Really that should have been "i + 1". checkpatch misses this. It seems to
> be missing more stuff that it used to lately.
This one is difficult. The rules up to now have been consistent spacing
is required on both sides of mathematics operators. I personally like
spaces always, but we do tend to use them without spaces too where the
binding is effectivly part of the value -- the classic case is something
like:
pfn << MAX_ORDER-1
In allowing that sort of thing, we implictly allow the one you note
above. We have tried to be overly annoying on these things, and so the
check is consistancy, spaces both or neither. We could be stricter.
-apw
On Thu, 2007-07-12 at 12:38 +0100, Andy Whitcroft wrote:
> Andrew Morton wrote:
> >> + if (ext4_ext_check_header(inode, ext_block_hdr(bh),
> >> + depth - i - 1)) {
> >> + err = -EIO;
> >> + break;
> >> + }
> >> + path[i+1].p_bh = bh;
> >
> > Really that should have been "i + 1". checkpatch misses this. It seems to
> > be missing more stuff that it used to lately.
>
> This one is difficult. The rules up to now have been consistent spacing
> is required on both sides of mathematics operators. I personally like
> spaces always, but we do tend to use them without spaces too where the
> binding is effectivly part of the value -- the classic case is something
> like:
>
> pfn << MAX_ORDER-1
>
> In allowing that sort of thing, we implictly allow the one you note
> above. We have tried to be overly annoying on these things, and so the
> check is consistancy, spaces both or neither. We could be stricter.
I personally think stricter is better. An occasionally false-positive
isn't going to hurt anyone. (Well, maybe the checkpatch.pl maintainers
will get nagged.) It at least will cause the developer to look at the
line of code in question and make a conscious decision to leave it as it
is. I'm assuming that upstream maintainers use checkpatch.pl with some
constraint, and don't throw every patch that produces a warning back at
the submitter.
Shaggy
--
David Kleikamp
IBM Linux Technology Center
On Thu, 12 Jul 2007 08:57:51 -0500 Dave Kleikamp <[email protected]> wrote:
> On Thu, 2007-07-12 at 12:38 +0100, Andy Whitcroft wrote:
> > Andrew Morton wrote:
> > >> + if (ext4_ext_check_header(inode, ext_block_hdr(bh),
> > >> + depth - i - 1)) {
> > >> + err = -EIO;
> > >> + break;
> > >> + }
> > >> + path[i+1].p_bh = bh;
> > >
> > > Really that should have been "i + 1". checkpatch misses this. It seems to
> > > be missing more stuff that it used to lately.
> >
> > This one is difficult. The rules up to now have been consistent spacing
> > is required on both sides of mathematics operators. I personally like
> > spaces always, but we do tend to use them without spaces too where the
> > binding is effectivly part of the value -- the classic case is something
> > like:
> >
> > pfn << MAX_ORDER-1
> >
> > In allowing that sort of thing, we implictly allow the one you note
> > above. We have tried to be overly annoying on these things, and so the
> > check is consistancy, spaces both or neither. We could be stricter.
>
> I personally think stricter is better. An occasionally false-positive
> isn't going to hurt anyone. (Well, maybe the checkpatch.pl maintainers
> will get nagged.) It at least will cause the developer to look at the
> line of code in question and make a conscious decision to leave it as it
> is. I'm assuming that upstream maintainers use checkpatch.pl with some
> constraint, and don't throw every patch that produces a warning back at
> the submitter.
>
I'm in two minds. Missing-the-spaces is pretty damn common and is sometimes
a reasonable way of saving quite a lot of horizontal space. I spose we could
take it out again if it's causing problems.
On Tue, 2007-07-10 at 16:30 -0700, Andrew Morton wrote:
> On Sun, 01 Jul 2007 03:36:22 -0400
> Mingming Cao <[email protected]> wrote:
>
> > with the patch all headers are checked. the code should become
> > more resistant to on-disk corruptions. needless BUG_ON() have
> > been removed. please, review for inclusion.
> >
> > ...
>
> > @@ -269,6 +239,70 @@
> > return size;
> > }
> >
> > +static inline int
> > +ext4_ext_max_entries(struct inode *inode, int depth)
>
> Please remove the `inline'.
>
done
> `inline' is usually wrong. It is wrong here. If this function has a
> single callsite (it has) then the compiler will inline it. If the function
> later gains more callsites then the compiler will know (correctly) not to
> inline it.
>
> We hope. If we find the compiler still inlines a function as large as this
> one then we'd need to use noinline and complain at the gcc developers.
>
> > +{
> > + int max;
> > +
> > + if (depth == ext_depth(inode)) {
> > + if (depth == 0)
> > + max = ext4_ext_space_root(inode);
> > + else
> > + max = ext4_ext_space_root_idx(inode);
> > + } else {
> > + if (depth == 0)
> > + max = ext4_ext_space_block(inode);
> > + else
> > + max = ext4_ext_space_block_idx(inode);
> > + }
> > +
> > + return max;
> > +}
> > +
> > +static int __ext4_ext_check_header(const char *function, struct inode *inode,
> > + struct ext4_extent_header *eh,
> > + int depth)
> > +{
> > + const char *error_msg = NULL;
>
> Unneeded initialisation.
>
done
> > + int max = 0;
> > +
> > + if (unlikely(eh->eh_magic != EXT4_EXT_MAGIC)) {
> > + error_msg = "invalid magic";
> > + goto corrupted;
> > + }
> > + if (unlikely(le16_to_cpu(eh->eh_depth) != depth)) {
> > + error_msg = "unexpected eh_depth";
> > + goto corrupted;
> > + }
> > + if (unlikely(eh->eh_max == 0)) {
> > + error_msg = "invalid eh_max";
> > + goto corrupted;
> > + }
> > + max = ext4_ext_max_entries(inode, depth);
> > + if (unlikely(le16_to_cpu(eh->eh_max) > max)) {
> > + error_msg = "too large eh_max";
> > + goto corrupted;
> > + }
> > + if (unlikely(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max))) {
> > + error_msg = "invalid eh_entries";
> > + goto corrupted;
> > + }
> > + return 0;
> > +
> > +corrupted:
> > + ext4_error(inode->i_sb, function,
> > + "bad header in inode #%lu: %s - magic %x, "
> > + "entries %u, max %u(%u), depth %u(%u)",
> > + inode->i_ino, error_msg, le16_to_cpu(eh->eh_magic),
> > + le16_to_cpu(eh->eh_entries), le16_to_cpu(eh->eh_max),
> > + max, le16_to_cpu(eh->eh_depth), depth);
> > +
> > + return -EIO;
> > +}
> > +
> >
> > ...
> >
> > + i = depth = ext_depth(inode);
> >
>
> Mulitple assignments like this are somewhat unpopular from the coding-style
> POV.
>
okay.
> We have a local variable called `i' which is not used as a simple counter
> and which has no explanatory comment. This is plain bad. Please find a
> better name for this variable. Review the code for other such lack of
> clarity - I'm sure there's more.
>
"i" is is loop counter. I have moved the i= depth to before the while
loop.
>
> > - BUG_ON(le16_to_cpu(eh->eh_entries) > le16_to_cpu(eh->eh_max));
> > - BUG_ON(eh->eh_magic != EXT4_EXT_MAGIC);
>
> Yeah, this patch improves things a lot.
>
> How well-tested is it? Have any of these errors actually been triggered?
>
> > path[i].p_hdr = ext_block_hdr(path[i].p_bh);
> > - if (ext4_ext_check_header(__FUNCTION__, inode,
> > - path[i].p_hdr)) {
> > - err = -EIO;
> > - goto out;
> > - }
> > }
> >
> > - BUG_ON(le16_to_cpu(path[i].p_hdr->eh_entries)
> > - > le16_to_cpu(path[i].p_hdr->eh_max));
> > - BUG_ON(path[i].p_hdr->eh_magic != EXT4_EXT_MAGIC);
> > -
> > if (!path[i].p_idx) {
> > /* this level hasn't been touched yet */
> > path[i].p_idx = EXT_LAST_INDEX(path[i].p_hdr);
> > @@ -1873,17 +1890,24 @@
> > i, EXT_FIRST_INDEX(path[i].p_hdr),
> > path[i].p_idx);
> > if (ext4_ext_more_to_rm(path + i)) {
> > + struct buffer_head *bh;
> > /* go to the next level */
> > ext_debug("move to level %d (block %llu)\n",
> > i + 1, idx_pblock(path[i].p_idx));
> > memset(path + i + 1, 0, sizeof(*path));
> > - path[i+1].p_bh =
> > - sb_bread(sb, idx_pblock(path[i].p_idx));
> > - if (!path[i+1].p_bh) {
> > + bh = sb_bread(sb, idx_pblock(path[i].p_idx));
> > + if (!bh) {
> > /* should we reset i_size? */
> > err = -EIO;
> > break;
> > }
> > + BUG_ON(i + 1 > depth);
>
> ouch. Couldn't we do WARN_ON then return -EIO here?
Done
>
> > + if (ext4_ext_check_header(inode, ext_block_hdr(bh),
> > + depth - i - 1)) {
> > + err = -EIO;
> > + break;
> > + }
> > + path[i+1].p_bh = bh;
>
> Really that should have been "i + 1". checkpatch misses this. It seems to
> be missing more stuff that it used to lately.
Done.
Hi Andrew,
Attached is incremental changes to address your comments.
Signed-off-by: Mingming Cao <[email protected]>
---
fs/ext4/extents.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
Index: linux-2.6.22/fs/ext4/extents.c
===================================================================
--- linux-2.6.22.orig/fs/ext4/extents.c 2007-07-15 23:25:38.000000000 -0700
+++ linux-2.6.22/fs/ext4/extents.c 2007-07-15 23:44:03.000000000 -0700
@@ -239,7 +239,7 @@ static int ext4_ext_space_root_idx(struc
return size;
}
-static inline int
+static int
ext4_ext_max_entries(struct inode *inode, int depth)
{
int max;
@@ -263,7 +263,7 @@ static int __ext4_ext_check_header(const
struct ext4_extent_header *eh,
int depth)
{
- const char *error_msg = NULL;
+ const char *error_msg;
int max = 0;
if (unlikely(eh->eh_magic != EXT4_EXT_MAGIC)) {
@@ -498,7 +498,7 @@ ext4_ext_find_extent(struct inode *inode
short int depth, i, ppos = 0, alloc = 0;
eh = ext_inode_hdr(inode);
- i = depth = ext_depth(inode);
+ depth = ext_depth(inode);
if (ext4_ext_check_header(inode, eh, depth))
return ERR_PTR(-EIO);
@@ -513,6 +513,7 @@ ext4_ext_find_extent(struct inode *inode
}
path[0].p_hdr = eh;
+ i = depth;
/* walk through the tree */
while (i) {
ext_debug("depth %d: num %d, max %d\n",
@@ -1973,13 +1974,16 @@ int ext4_ext_remove_space(struct inode *
err = -EIO;
break;
}
- BUG_ON(i + 1 > depth);
+ if (WARN_ON(i + 1 > depth)) {
+ err = -EIO;
+ break;
+ }
if (ext4_ext_check_header(inode, ext_block_hdr(bh),
depth - i - 1)) {
err = -EIO;
break;
}
- path[i+1].p_bh = bh;
+ path[i + 1].p_bh = bh;
/* save actual number of indexes since this
* number is changed at the next iteration */