login
Search
Various places in fs/ext3/namei.c use ext3_next_entry to loop over directory entries, but not all of them verify that entries are valid before attempting to move to the next one. In the case where rec_len == 0 this causes an infinite loop. Introduce a new version of ext3_next_entry that checks the validity of the entry before using its data to find the next one. Rename the original function to __ext3_next_entry and use it in places where we already know the data is valid. Note that the changes to empty_dir follow the original code in reporting the directory as empty in the presence of errors. This patch fixes the first case (image hdb.25.softlockup.gz) reported in http://bugzilla.kernel.org/show_bug.cgi?id=10882. Signed-off-by: Duane Griffin <duaneg@dghda.com> -- Please note that I've only properly tested the originally reported failure case (i.e. the changes to ext3_dx_find_entry). Reviewers may want to pay particular attention to the changes to do_split, make_indexed_dir and empty_dir. I've tried to follow the original code's error handling conventions, as noted above for empty_dir, but I'm not sure this is the best way to do things. --- diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c index 0b8cf80..f092208 100644 --- a/fs/ext3/namei.c +++ b/fs/ext3/namei.c @@ -185,13 +185,27 @@ static int ext3_dx_add_entry(handle_t *handle, struct dentry *dentry, * p is at least 6 bytes before the end of page */ static inline struct ext3_dir_entry_2 * -ext3_next_entry(struct ext3_dir_entry_2 *p) +__ext3_next_entry(struct ext3_dir_entry_2 *p) { return (struct ext3_dir_entry_2 *)((char *)p + ext3_rec_len_from_disk(p->rec_len)); } /* + * Checks the directory entry looks sane before using it to find the next one. + * Returns NULL and reports an error if an invalid entry is passed. + */ +static inline struct ext3_dir_entry_2 * +ext3_next_entry(const char *func, struct inode *dir, + struct ext3_dir_entry_2 *de, struct buffer_head *bh, int offset) +{ + if ...
Various places in fs/ext3/namei.c use ext3_next_entry to loop over directory entries, but not all of them verify that entries are valid before attempting to move to the next one. In the case where rec_len == 0 this causes an infinite loop. Introduce a new version of ext3_next_entry that checks the validity of the entry before using its data to find the next one. Rename the original function to __ext3_next_entry and use it in places where we already know the data is valid. Note that the changes to empty_dir follow the original code in reporting the directory as empty in the presence of errors. This patch fixes the first case (image hdb.25.softlockup.gz) reported in http://bugzilla.kernel.org/show_bug.cgi?id=10882. Signed-off-by: Duane Griffin <duaneg@dghda.com> -- This is version 2. The original patch was an earlier version causing warnings that I sent by mistake. This version just fixes those. Please note that I've only properly tested the originally reported failure case (i.e. the changes to ext3_dx_find_entry). Reviewers may want to pay particular attention to the changes to do_split, make_indexed_dir and empty_dir. I've tried to follow the original code's error handling conventions, as noted above for empty_dir, but I'm not sure this is the best way to do things. --- diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c index 0b8cf80..ea0236b 100644 --- a/fs/ext3/namei.c +++ b/fs/ext3/namei.c @@ -185,13 +185,27 @@ static int ext3_dx_add_entry(handle_t *handle, struct dentry *dentry, * p is at least 6 bytes before the end of page */ static inline struct ext3_dir_entry_2 * -ext3_next_entry(struct ext3_dir_entry_2 *p) +__ext3_next_entry(struct ext3_dir_entry_2 *p) { return (struct ext3_dir_entry_2 *)((char *)p + ext3_rec_len_from_disk(p->rec_len)); } /* + * Checks the directory entry looks sane before using it to find the next one. + * Returns NULL and reports an error if an invalid entry is passed. + */ +static inline struct ext3_dir_entry_2 ...
Hi Duane, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ All the best, Jochen -- http://seehuhn.de/ --
Aargh, I just can't seem to get this patch out cleanly! That looks like a vi typo, thanks for catching it so quickly. All but one of the casts should be removed, but I'll wait to see if there is any further Cheers, Duane. -- "I never could learn to drink that blood and call it wine" - Bob Dylan --
Andrew might complain that the above function is too big to be
inlined. I'm not really sure where he draws the borderline but I believe
Here should be __func__ as well - not your fault, I know... Anyway,
Apart from (char *) thing, you also don't need braces above. Maybe the
whole while loop would be nicer as:
de2 = de;
while (de2 != NULL && de2 < top) {
de = de2;
de2 = ext3_next_entry(__func__, dir, de, bh, 0);
Honza
--
Jan Kara <jack@suse.cz>
SuSE CR Labs
--
Done, fixing a couple of incorrect strings along the way, thereby proving the usefulness of the exercise. A macro would make things slightly tidier, but I'm not sure it is worth doing. Let me know if you Cheers, Duane. -- "I never could learn to drink that blood and call it wine" - Bob Dylan --
Various places in fs/ext3/namei.c use ext3_next_entry to loop over directory entries, but not all of them verify that entries are valid before attempting to move to the next one. In the case where rec_len == 0 this causes an infinite loop. Introduce a new version of ext3_next_entry that checks the validity of the entry before using its data to find the next one. Rename the original function to __ext3_next_entry and use it in places where we already know the data is valid. Note that the changes to empty_dir follow the original code in reporting the directory as empty in the presence of errors. This patch fixes the first case (image hdb.25.softlockup.gz) reported in http://bugzilla.kernel.org/show_bug.cgi?id=10882. Signed-off-by: Duane Griffin <duaneg@dghda.com> -- This is version 3. It includes some tidy-ups from v2 as suggested by Jochen Voss and Jan Kara. It also replaces hard-coded function name strings with __func__ in all calls to ext3_check_dir_entry, including one in an otherwise untouched file. I don't think a separate patch for this is necessary, but if it would be preferred I'd be happy to split it out. I've removed some whitespace in a couple of places in order to fit lines into 80 columns, so there are a couple of checkpatch warnings. Let me know if you think it would be better to split the lines or go over 80 cols. Please note that I've only properly tested the originally reported failure case (i.e. the changes to ext3_dx_find_entry). Reviewers may want to pay particular attention to the changes to do_split, make_indexed_dir and empty_dir. I've tried to follow the original code's error handling conventions, as noted above for empty_dir, but I'm not sure this is the best way to do things. --- diff --git a/fs/ext3/dir.c b/fs/ext3/dir.c index 8ca3bfd..2a8ab33 100644 --- a/fs/ext3/dir.c +++ b/fs/ext3/dir.c @@ -187,7 +187,7 @@ revalidate: while (!error && filp->f_pos < inode->i_size && offset < sb->s_blocksize) { de = (struct ...
For what it's worth, you can add Acked-by: Jan Kara <jack@suse.cz> -- Jan Kara <jack@suse.cz> SUSE Labs, CR --
Just as a note, and not to detract from the validity of this patch - in the ext2 page-based directory code is somewhat more efficient in this area. It checks each page a single time when it is first read from disk, marks the page as checked, and then never has to check the page again. This wasn't implemented for ext3 because it never changed from buffer- based directories to page-based directories due to the dependence on buffer heads for the journaling code. It would be possible to implement this for ext3/ext4 readdir/lookup by replacing use of ext3_bread() with a new ext3_bread_dir() - a copy of ext3_bread() that validates the buffer if it actually needs ll_rw_block() to read the buffer from disk. I also note in ext3_readdir() for non-indexed directories that the readahead doesn't check for !buffer_uptodate(bh) before forcing a read of the next block. Cheers, Andreas -- Andreas Dilger Sr. Staff Engineer, Lustre Group Sun Microsystems of Ca