| Message ID | 1348512100-23323-1-git-send-email-cmaiolino@redhat.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 2012-09-24, at 20:41, Carlos Maiolino <cmaiolino@redhat.com> wrote: > Audit the usage of ext4_bread() due a possible misinterpretion of its return > value. > Focused on directory blocks, a NULL value returned from ext4_bread() means a > hole, which cannot exist into a directory inode. It can pass undetected after a > fix in an uninitialized error variable. > > The (now) initialized variable into ext4_getblk() may lead to a zero'ed return > value of ext4_bread() to its callers, which can make the caller do not detect > the hole in the directory inode. > > This checks for directory holes when buffer_head and error value are both > zero'ed returning -EIO to their callers > > Some ext4_bread callers do not needed any changes either because they already > had its own hole detector paths or because these are deprecaded (like > dx_show_entries) Note that in the "errors=continue" case, it is desirable to allow the Filesystem to continue even if a hole is found in the directory, at least for non-htree directories, since it is possible the the entry being looked up is in a different block. > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > fs/ext4/namei.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 69 insertions(+), 11 deletions(-) > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 2a42cc0..1d5619b 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -67,6 +67,11 @@ static struct buffer_head *ext4_append(handle_t *handle, > bh = NULL; > } > } > + if (!bh && !(*err)) { > + *err = -EIO; > + ext4_error(inode->i_sb, "Directory hole detected on inode %lu\n", > + inode->i_ino); > + } > return bh; > } > > @@ -696,8 +701,11 @@ dx_probe(const struct qstr *d_name, struct inode *dir, > frame->entries = entries; > frame->at = at; > if (!indirect--) return frame; > - if (!(bh = ext4_bread (NULL,dir, dx_get_block(at), 0, err))) > + if (!(bh = ext4_bread (NULL,dir, dx_get_block(at), 0, err))) { Please keep the original code style, do not Di the assignment inside the if() check. That is incorrect in several places in this patch. > + if (!(*err)) > + *err = ERR_BAD_DX_DIR; > goto fail2; > + } > at = entries = ((struct dx_node *) bh->b_data)->entries; > > if (!buffer_verified(bh) && > @@ -807,8 +815,15 @@ static int ext4_htree_next_block(struct inode *dir, __u32 hash, > */ > while (num_frames--) { > if (!(bh = ext4_bread(NULL, dir, dx_get_block(p->at), > - 0, &err))) > + 0, &err))) { > + if (!err) { > + ext4_error(dir->i_sb, > + "Directory hole detected on inode %lu\n", > + dir->i_ino); > + return -EIO; > + } > return err; /* Failure */ > + } > > if (!buffer_verified(bh) && > !ext4_dx_csum_verify(dir, > @@ -843,8 +858,14 @@ static int htree_dirblock_to_tree(struct file *dir_file, > > dxtrace(printk(KERN_INFO "In htree dirblock_to_tree: block %lu\n", > (unsigned long)block)); > - if (!(bh = ext4_bread (NULL, dir, block, 0, &err))) > + if (!(bh = ext4_bread (NULL, dir, block, 0, &err))) { > + if (!err) { > + err = -EIO; > + ext4_error(dir->i_sb, "Directory hole detected on inode %lu\n", > + dir->i_ino); > + } > return err; > + } > > if (!buffer_verified(bh) && > !ext4_dirent_csum_verify(dir, (struct ext4_dir_entry *)bh->b_data)) > @@ -1267,8 +1288,15 @@ static struct buffer_head * ext4_dx_find_entry(struct inode *dir, const struct q > return NULL; > do { > block = dx_get_block(frame->at); > - if (!(bh = ext4_bread(NULL, dir, block, 0, err))) > + if (!(bh = ext4_bread(NULL, dir, block, 0, err))) { > + if (!(*err)) { > + *err = -EIO; > + ext4_error(dir->i_sb, > + "Directory hole detected on inode %lu\n", > + dir->i_ino); > + } > goto errout; > + } > > if (!buffer_verified(bh) && > !ext4_dirent_csum_verify(dir, > @@ -1801,9 +1829,15 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, > } > blocks = dir->i_size >> sb->s_blocksize_bits; > for (block = 0; block < blocks; block++) { > - bh = ext4_bread(handle, dir, block, 0, &retval); > - if(!bh) > + if (!(bh = ext4_bread(handle, dir, block, 0, &retval))) { > + if (!retval) { > + retval = -EIO; > + ext4_error(inode->i_sb, > + "Directory hole detected on inode %lu\n", > + inode->i_ino); > + } > return retval; > + } > if (!buffer_verified(bh) && > !ext4_dirent_csum_verify(dir, > (struct ext4_dir_entry *)bh->b_data)) > @@ -1860,8 +1894,15 @@ static int ext4_dx_add_entry(handle_t *handle, struct dentry *dentry, > entries = frame->entries; > at = frame->at; > > - if (!(bh = ext4_bread(handle,dir, dx_get_block(frame->at), 0, &err))) > + if (!(bh = ext4_bread(handle,dir, dx_get_block(frame->at), 0, &err))) { > + if (!err) { > + err = -EIO; > + ext4_error(dir->i_sb, > + "Directory hole detected on inode %lu\n", > + dir->i_ino); > + } > goto cleanup; > + } > > if (!buffer_verified(bh) && > !ext4_dirent_csum_verify(dir, (struct ext4_dir_entry *)bh->b_data)) > @@ -2199,9 +2240,15 @@ retry: > inode->i_op = &ext4_dir_inode_operations; > inode->i_fop = &ext4_dir_operations; > inode->i_size = EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize; > - dir_block = ext4_bread(handle, inode, 0, 1, &err); > - if (!dir_block) > + if (!(dir_block = ext4_bread(handle, inode, 0, 1, &err))) { > + if (!err) { > + err = -EIO; > + ext4_error(inode->i_sb, > + "Directory hole detected on inode %lu\n", > + inode->i_ino); > + } > goto out_clear_inode; > + } > BUFFER_TRACE(dir_block, "get_write_access"); > err = ext4_journal_get_write_access(handle, dir_block); > if (err) > @@ -2318,6 +2365,11 @@ static int empty_dir(struct inode *inode) > EXT4_ERROR_INODE(inode, > "error %d reading directory " > "lblock %u", err, lblock); > + else > + ext4_warning(inode->i_sb, > + "bad directory (dir #%lu) - no data block", > + inode->i_ino); > + > offset += sb->s_blocksize; > continue; > } > @@ -2826,9 +2878,15 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry, > goto end_rename; > } > retval = -EIO; > - dir_bh = ext4_bread(handle, old_inode, 0, 0, &retval); > - if (!dir_bh) > + if(!(dir_bh = ext4_bread(handle, old_inode, 0, 0, &retval))) { > + if (!retval) { > + retval = -EIO; > + ext4_error(old_inode->i_sb, > + "Directory hole detected on inode %lu\n", > + old_inode->i_ino); > + } > goto end_rename; > + } > if (!buffer_verified(dir_bh) && > !ext4_dirent_csum_verify(old_inode, > (struct ext4_dir_entry *)dir_bh->b_data)) > -- > 1.7.11.4 > > -- > 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 Mon, Sep 24, 2012 at 03:41:40PM -0300, Carlos Maiolino wrote: > > Some ext4_bread callers do not needed any changes either because they already > had its own hole detector paths or because these are deprecaded (like > dx_show_entries) BTW, dx_show entries isn't really deprecated; is debugging code which is usually not compiled in, but it does get used from time to time by developers who are debugging the directory hash tree code... So having it show the holes by printing a note that there _was_ a whole is probably a good thing from a debugging point of view... - 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 9/25/12 10:42 PM, Theodore Ts'o wrote: > On Mon, Sep 24, 2012 at 03:41:40PM -0300, Carlos Maiolino wrote: >> >> Some ext4_bread callers do not needed any changes either because they already >> had its own hole detector paths or because these are deprecaded (like >> dx_show_entries) > > BTW, dx_show entries isn't really deprecated; is debugging code which > is usually not compiled in, but it does get used from time to time by > developers who are debugging the directory hash tree code... > > So having it show the holes by printing a note that there _was_ a > whole is probably a good thing from a debugging point of view... I think what Carlos meant is that there are no callers in the tree... so even with debug options, it's not used. It takes a custom patch... Does it still work? :) -Eric > - 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 > -- 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, Sep 25, 2012 at 10:48:17PM -0500, Eric Sandeen wrote: > On 9/25/12 10:42 PM, Theodore Ts'o wrote: > > On Mon, Sep 24, 2012 at 03:41:40PM -0300, Carlos Maiolino wrote: > >> > >> Some ext4_bread callers do not needed any changes either because they already > >> had its own hole detector paths or because these are deprecaded (like > >> dx_show_entries) > > > > BTW, dx_show entries isn't really deprecated; is debugging code which > > is usually not compiled in, but it does get used from time to time by > > developers who are debugging the directory hash tree code... > > > > So having it show the holes by printing a note that there _was_ a > > whole is probably a good thing from a debugging point of view... > > I think what Carlos meant is that there are no callers in the tree... > so even with debug options, it's not used. It takes a custom patch... > Does it still work? :) > Yeah, that's what I meant, no callers of dx_show_entries() even with debug enabled. In regards to the coding style of if conditionals, I just followed the coding style of most places in the code, I also changed some of the if conditionals to match the rest of the code. i.e.: if(!(bh = ext4_bread())) { do_something(); } if you take a look at the code, most if conditionals regarding the calls to ext4_bread() are in the above coding style. I just followed it, but, I'm not against change that to another one, but agree the above looks better and save some code lines > -Eric > > > - 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 > > > > -- > 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
Thanks, applied.
I fixed it the commit description so it was correctly word wrapped and
to make the English easier to read. Also, I fixed up a missing check
at the beginning of dx_probe() where a check was missing:
if (!(bh = ext4_bread(NULL, dir, 0, 0, err))) {
if (*err == 0)
*err = ERR_BAD_DX_DIR;
goto fail;
}
Thanks for the patch!!
- Ted
ext4: ext4_bread usage audit
When ext4_bread() returns NULL and err is set to zero, this means
there is no phyical block mapped to the specified logical block
number. (Previous to commit 90b0a97323, err was uninitialized in this
case, which caused other problems.)
The directory handling routines use ext4_bread() in many places, the
fact that ext4_bread() now returns NULL with err set to zero could
cause problems since a number of these functions will simply return
the value of err if the result of ext4_bread() was the NULL pointer,
causing the caller of the function to think that the function was
successful.
Since directories should never contain holes, this case can only
happen if the file system is corrupted. This commit audits all of the
callers of ext4_bread(), and makes sure they do the right thing if a
hole in a directory is found by ext4_bread().
Some ext4_bread() callers did not need any changes either because they
already had its own hole detector paths.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
--
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-09-26, at 4:14 PM, Carlos Maiolino wrote: > In regards to the coding style of if conditionals, I just followed the coding > style of most places in the code, I also changed some of the if conditionals to > match the rest of the code. i.e.: > > if(!(bh = ext4_bread())) { > do_something(); > } > > if you take a look at the code, most if conditionals regarding the calls to > ext4_bread() are in the above coding style. I just followed it, but, I'm not > against change that to another one, but agree the above looks better and save > some code lines. The reason that having assignments in conditionals is bad is that it allows hard-to-find bugs to be in the code: if ((bh == ext4_bread()) and if ((bh = ext4_bread()) look very similar to the reader, but the use of extra "()" around the assignment quiets any compiler warnings about incorrect assignments. In this specific case you _do_ want the assignment to bh, but in many other cases you do not: if (bh = NULL) would set bh to NULL instead of comparing it, and is IMHO bad coding style. 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
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 2a42cc0..1d5619b 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -67,6 +67,11 @@ static struct buffer_head *ext4_append(handle_t *handle, bh = NULL; } } + if (!bh && !(*err)) { + *err = -EIO; + ext4_error(inode->i_sb, "Directory hole detected on inode %lu\n", + inode->i_ino); + } return bh; } @@ -696,8 +701,11 @@ dx_probe(const struct qstr *d_name, struct inode *dir, frame->entries = entries; frame->at = at; if (!indirect--) return frame; - if (!(bh = ext4_bread (NULL,dir, dx_get_block(at), 0, err))) + if (!(bh = ext4_bread (NULL,dir, dx_get_block(at), 0, err))) { + if (!(*err)) + *err = ERR_BAD_DX_DIR; goto fail2; + } at = entries = ((struct dx_node *) bh->b_data)->entries; if (!buffer_verified(bh) && @@ -807,8 +815,15 @@ static int ext4_htree_next_block(struct inode *dir, __u32 hash, */ while (num_frames--) { if (!(bh = ext4_bread(NULL, dir, dx_get_block(p->at), - 0, &err))) + 0, &err))) { + if (!err) { + ext4_error(dir->i_sb, + "Directory hole detected on inode %lu\n", + dir->i_ino); + return -EIO; + } return err; /* Failure */ + } if (!buffer_verified(bh) && !ext4_dx_csum_verify(dir, @@ -843,8 +858,14 @@ static int htree_dirblock_to_tree(struct file *dir_file, dxtrace(printk(KERN_INFO "In htree dirblock_to_tree: block %lu\n", (unsigned long)block)); - if (!(bh = ext4_bread (NULL, dir, block, 0, &err))) + if (!(bh = ext4_bread (NULL, dir, block, 0, &err))) { + if (!err) { + err = -EIO; + ext4_error(dir->i_sb, "Directory hole detected on inode %lu\n", + dir->i_ino); + } return err; + } if (!buffer_verified(bh) && !ext4_dirent_csum_verify(dir, (struct ext4_dir_entry *)bh->b_data)) @@ -1267,8 +1288,15 @@ static struct buffer_head * ext4_dx_find_entry(struct inode *dir, const struct q return NULL; do { block = dx_get_block(frame->at); - if (!(bh = ext4_bread(NULL, dir, block, 0, err))) + if (!(bh = ext4_bread(NULL, dir, block, 0, err))) { + if (!(*err)) { + *err = -EIO; + ext4_error(dir->i_sb, + "Directory hole detected on inode %lu\n", + dir->i_ino); + } goto errout; + } if (!buffer_verified(bh) && !ext4_dirent_csum_verify(dir, @@ -1801,9 +1829,15 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, } blocks = dir->i_size >> sb->s_blocksize_bits; for (block = 0; block < blocks; block++) { - bh = ext4_bread(handle, dir, block, 0, &retval); - if(!bh) + if (!(bh = ext4_bread(handle, dir, block, 0, &retval))) { + if (!retval) { + retval = -EIO; + ext4_error(inode->i_sb, + "Directory hole detected on inode %lu\n", + inode->i_ino); + } return retval; + } if (!buffer_verified(bh) && !ext4_dirent_csum_verify(dir, (struct ext4_dir_entry *)bh->b_data)) @@ -1860,8 +1894,15 @@ static int ext4_dx_add_entry(handle_t *handle, struct dentry *dentry, entries = frame->entries; at = frame->at; - if (!(bh = ext4_bread(handle,dir, dx_get_block(frame->at), 0, &err))) + if (!(bh = ext4_bread(handle,dir, dx_get_block(frame->at), 0, &err))) { + if (!err) { + err = -EIO; + ext4_error(dir->i_sb, + "Directory hole detected on inode %lu\n", + dir->i_ino); + } goto cleanup; + } if (!buffer_verified(bh) && !ext4_dirent_csum_verify(dir, (struct ext4_dir_entry *)bh->b_data)) @@ -2199,9 +2240,15 @@ retry: inode->i_op = &ext4_dir_inode_operations; inode->i_fop = &ext4_dir_operations; inode->i_size = EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize; - dir_block = ext4_bread(handle, inode, 0, 1, &err); - if (!dir_block) + if (!(dir_block = ext4_bread(handle, inode, 0, 1, &err))) { + if (!err) { + err = -EIO; + ext4_error(inode->i_sb, + "Directory hole detected on inode %lu\n", + inode->i_ino); + } goto out_clear_inode; + } BUFFER_TRACE(dir_block, "get_write_access"); err = ext4_journal_get_write_access(handle, dir_block); if (err) @@ -2318,6 +2365,11 @@ static int empty_dir(struct inode *inode) EXT4_ERROR_INODE(inode, "error %d reading directory " "lblock %u", err, lblock); + else + ext4_warning(inode->i_sb, + "bad directory (dir #%lu) - no data block", + inode->i_ino); + offset += sb->s_blocksize; continue; } @@ -2826,9 +2878,15 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry, goto end_rename; } retval = -EIO; - dir_bh = ext4_bread(handle, old_inode, 0, 0, &retval); - if (!dir_bh) + if(!(dir_bh = ext4_bread(handle, old_inode, 0, 0, &retval))) { + if (!retval) { + retval = -EIO; + ext4_error(old_inode->i_sb, + "Directory hole detected on inode %lu\n", + old_inode->i_ino); + } goto end_rename; + } if (!buffer_verified(dir_bh) && !ext4_dirent_csum_verify(old_inode, (struct ext4_dir_entry *)dir_bh->b_data))
Audit the usage of ext4_bread() due a possible misinterpretion of its return value. Focused on directory blocks, a NULL value returned from ext4_bread() means a hole, which cannot exist into a directory inode. It can pass undetected after a fix in an uninitialized error variable. The (now) initialized variable into ext4_getblk() may lead to a zero'ed return value of ext4_bread() to its callers, which can make the caller do not detect the hole in the directory inode. This checks for directory holes when buffer_head and error value are both zero'ed returning -EIO to their callers Some ext4_bread callers do not needed any changes either because they already had its own hole detector paths or because these are deprecaded (like dx_show_entries) Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- fs/ext4/namei.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 69 insertions(+), 11 deletions(-)