introduce range check for extent pblock references
PATCH 1/1 introduce range check for extent pblock references
This time I have aimed to catch all cases in which an invalid physical block might be used and implemented checks directly in ext_pblock() and idx_pblock() following the assumption that most of the times one of these functions is called a device access to that address will follow. If you think this is too heavy, I could also split the check from the pblock calculation, but in that case I could only guess at which of the several accesses to *_pblock() in extents.c a check would be necessary and where it wouldn't and there would be the possibility of missing something.
Another thing I'm unsure about is uninitialised extents, here my heuristic again was that ext_pblock() wouldn't be called if there was not an access to follow, so I didn't include a condition that would excempt them from the check.
The attached patch has only been mildly tested.
And I'm pretty new to linux and ext4, so there might be stupid mistakes.
fs/ext4/ext4_extents.h renamed from linux-2.6.29-rc2-vanilla/fs/ext4/ext4_extents.h +4 -1
@@ -221,7 +221,10 @@ }extern int ext4_ext_calc_metadata_amount (struct inode *inode,int blocks); -extern ext4_fsblk_t idx_pblock (struct ext4_extent_idx *); +#define idx_pblock (inode, eh) \ + __idx_pblock(__func__, inode, eh) +extern ext4_fsblk_t __idx_pblock (const char *function,struct inode *inode, +struct ext4_extent_idx *);extern void ext4_ext_store_pblock (struct ext4_extent *,ext4_fsblk_t );extern int ext4_extent_tree_init (handle_t *,struct inode *); extern int ext4_ext_calc_credits_for_single_extent(struct inode *inode,
Only in linux-2.6.29/fs/ext4: ext4_extents.h~
fs/ext4/extents.c renamed from linux-2.6.29-rc2-vanilla/fs/ext4/extents.c +84 -46
403 lines not on this page · raw message
fs/ext4/migrate.c renamed from linux-2.6.29-rc2-vanilla/fs/ext4/migrate.c +1 -1
@@ -404,7 +404,7 @@struct buffer_head *bh;struct ext4_extent_header *eh; - block =idx_pblock (ix); + block =idx_pblock (inode, ix); bh =sb_bread (inode->i_sb, block);if (!bh)return -EIO ;
Quoted 9 lines · Thiemo Nagel
This time I have aimed to catch all cases in which an invalid physical block might be used and implemented checks directly in
ext_pblock()andidx_pblock()following the assumption that most of the times one of these functions is called a device access to that address will follow. If you think this is too heavy, I could also split the check from the pblock calculation, but in that case I could only guess at which of the several accesses to*_pblock()inextents.ca check would be necessary and where it wouldn't and there would be the possibility of missing something.
Do we want to check for validity every time we look at the physical block of the extent. I guess that would be bad performance wise. I guess we should check only once when we read the extent from the disk. ??
-aneesh
Quoted 7 lines · Aneesh Kumar K.V
Quoted 1 line · Thiemo Nagel
This time I have aimed to catch all cases in which an invalid physical block might be used and implemented checks directly in
ext_pblock()andidx_pblock()following the assumption that most of the times one of these functions is called a device access to that address will follow. If you think this is too heavy, I could also split the check from the pblock calculation, but in that case I could only guess at which of the several accesses to*_pblock()inextents.ca check would be necessary and where it wouldn't and there would be the possibility of missing something.Do we want to check for validity every time we look at the physical block of the extent. I guess that would be bad performance wise. I guess we should check only once when we read the extent from the disk. ??
Then we need to be careful not to miss a case. We would need to check every time we either a) read from the physical block ourselves or b) return the physical block number to a caller outside of ext4. On the other hand I wonder if there are cases where one looks at the physical block number which are not a) or b) and which would not benefit from the added sanity check?
For repeated accesses to the same physical block number I cannot think of a way to bookkeep whether the check already has been done, except if that is implicit in the code flow and I don't know how frequent that case is. If you think the performance gain is worth the risk of missing a case (either now or during some future change), I can try to rewrite the patch to implement the check on a case-by-case basis.
Kind regards,
Thiemo
Quoted 24 lines · Thiemo Nagel
Quoted 12 lines · Aneesh Kumar K.V
Quoted 9 lines · Thiemo Nagel
This time I have aimed to catch all cases in which an invalid physical block might be used and implemented checks directly in
ext_pblock()andidx_pblock()following the assumption that most of the times one of these functions is called a device access to that address will follow. If you think this is too heavy, I could also split the check from the pblock calculation, but in that case I could only guess at which of the several accesses to*_pblock()inextents.ca check would be necessary and where it wouldn't and there would be the possibility of missing something.Quoted 3 lines
Do we want to check for validity every time we look at the physical block of the extent. I guess that would be bad performance wise. I guess we should check only once when we read the extent from the disk. ??
Then we need to be careful not to miss a case. We would need to check every time we either a) read from the physical block ourselves or b) return the physical block number to a caller outside of ext4. On the other hand I wonder if there are cases where one looks at the physical block number which are not a) or b) and which would not benefit from the added sanity check?
Quoted 7 lines
For repeated accesses to the same physical block number I cannot think of a way to bookkeep whether the check already has been done, except if that is implicit in the code flow and I don't know how frequent that case is. If you think the performance gain is worth the risk of missing a case (either now or during some future change), I can try to rewrite the patch to implement the check on a case-by-case basis.
How about the following two patches. The patches have only gone through simple tests.
-aneesh
PATCH 1/1 Add checks to validate extent entries.
In reply to Aneesh Kumar K.VThis patch adds check to validate the extent entries along with extent headers. Should handle crash with corrupt filesystem.
--- fs/ext4/extents.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 71 insertions(+), 10 deletions(-)
fs/ext4/extents.c +71 -10
@@ -301,7 +301,64 @@ ext4_ext_max_entries(struct inode *inode, int depth)return max; } -static int __ext4_ext_check_header (const char *function,struct inode *inode, +static int ext4_valid_extent (struct inode *inode,struct ext4_extent *ext) +{ +ext4_fsblk_t block =ext_pblock (ext); +int len =ext4_ext_get_actual_len (ext); +struct ext4_super_block *es =EXT4_SB (inode->i_sb)->s_es; +if (unlikely (block <le32_to_cpu (es->s_first_data_block) || + ((block + len) >ext4_blocks_count (es)))) +return 0 ; +else +return 1 ; +} + +static int ext4_valid_extent_idx (struct inode *inode, +struct ext4_extent_idx *ext_idx) +{ +ext4_fsblk_t block =idx_pblock (ext_idx); +struct ext4_super_block *es =EXT4_SB (inode->i_sb)->s_es; +if (unlikely (block <le32_to_cpu (es->s_first_data_block) || + (block >ext4_blocks_count (es)))) +return 0 ; +else +return 1 ; +} + +static int ext4_valid_extent_entries (struct inode *inode, +struct ext4_extent_header *eh, +int depth) +{ +struct ext4_extent *ext; +struct ext4_extent_idx *ext_idx; +unsigned short entries; +if (eh->eh_entries ==0 ) +return 1 ; + + entries =le16_to_cpu (eh->eh_entries); + +if (depth ==0 ) { +/* leaf entries */ + ext =EXT_FIRST_EXTENT (eh); +while (entries) { +if (!ext4_valid_extent (inode, ext)) +return 0 ; + ext++; + entries--; + } + }else { + ext_idx =EXT_FIRST_INDEX (eh); +while (entries) { +if (!ext4_valid_extent_idx (inode, ext_idx)) +return 0 ; + ext_idx++; + entries--; + } + } +return 1 ; +} + +static int __ext4_ext_check (const char *function,struct inode *inode,struct ext4_extent_header *eh,int depth) {
@@ -329,11 +386,15 @@ static int __ext4_ext_check_header(const char *function, struct inode *inode, error_msg ="invalid eh_entries" ; goto corrupted; } +if (!ext4_valid_extent_entries (inode, eh, depth)) { + error_msg ="invalid extent entries" ; + goto corrupted; + }return 0 ; corrupted: ext4_error(inode->i_sb, function, - "bad header in inode #%lu: %s - magic %x, " + "bad header/extent 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),
@@ -342,8 +403,8 @@ corrupted:return -EIO ; } -#define ext4_ext_check_header (inode, eh, depth) \ - __ext4_ext_check_header(__func__, inode, eh, depth) +#define ext4_ext_check (inode, eh, depth) \ + __ext4_ext_check(__func__, inode, eh, depth)#ifdef EXT_DEBUG static void ext4_ext_show_path (struct inode *inode,struct ext4_ext_path *path)
@@ -547,7 +608,7 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block, eh =ext_inode_hdr (inode); depth =ext_depth (inode); -if (ext4_ext_check_header (inode, eh, depth)) +if (ext4_ext_check (inode, eh, depth))return ERR_PTR (-EIO );
@@ -584,7 +645,7 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block, path[ppos].p_hdr = eh; i--; -if (ext4_ext_check_header (inode, eh, i)) +if (ext4_ext_check (inode, eh, i)) goto err; }
@@ -1179,7 +1240,7 @@ got_index:if (bh ==NULL )return -EIO ; eh =ext_block_hdr (bh); -if (ext4_ext_check_header (inode, eh, depth)) { +if (ext4_ext_check (inode, eh, depth)) {put_bh (bh);return -EIO ; }
@@ -1192,7 +1253,7 @@ got_index:if (bh ==NULL )return -EIO ; eh =ext_block_hdr (bh); -if (ext4_ext_check_header (inode, eh, path->p_depth - depth)) { +if (ext4_ext_check (inode, eh, path->p_depth - depth)) {put_bh (bh);return -EIO ; }
@@ -2135,7 +2196,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)return -ENOMEM ; } path[0 ].p_hdr =ext_inode_hdr (inode); -if (ext4_ext_check_header (inode, path[0 ].p_hdr, depth)) { +if (ext4_ext_check (inode, path[0 ].p_hdr, depth)) { err = -EIO ; goto out; }
@@ -2189,7 +2250,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start) err = -EIO ;break ; } -if (ext4_ext_check_header (inode,ext_block_hdr (bh), +if (ext4_ext_check (inode,ext_block_hdr (bh), depth - i -1 )) { err = -EIO ;break ;
Signature · 1 line
tg: (ae1a25d..) extent_validate (depends on: master)
PATCH 1/1 Validate extent details only when read from the disk
Make sure we validate extent details only when read from the disk.
--- fs/ext4/ext4_extents.h | 1 + fs/ext4/extents.c | 24 +++++++++++++++++------- fs/ext4/inode.c | 5 +++++ 3 files changed, 23 insertions(+), 7 deletions(-)
fs/ext4/ext4_extents.h +1 -0
@@ -241,5 +241,6 @@ extern int ext4_ext_search_left(struct inode *, struct ext4_ext_path *,extern int ext4_ext_search_right (struct inode *,struct ext4_ext_path *,ext4_lblk_t *,ext4_fsblk_t *);extern void ext4_ext_drop_refs (struct ext4_ext_path *); +extern int ext4_ext_check_inode (struct inode *inode);#endif /* _EXT4_EXTENTS */
fs/ext4/extents.c +17 -7
@@ -406,6 +406,11 @@ corrupted:#define ext4_ext_check (inode, eh, depth) \ __ext4_ext_check(__func__, inode, eh, depth) +int ext4_ext_check_inode (struct inode *inode) +{ +return ext4_ext_check (inode,ext_inode_hdr (inode),ext_depth (inode)); +} +#ifdef EXT_DEBUG static void ext4_ext_show_path (struct inode *inode,struct ext4_ext_path *path) {
@@ -602,15 +607,13 @@ struct ext4_ext_path * ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block, struct ext4_ext_path *path) { + int need_to_validate = 0;struct ext4_extent_header *eh;struct buffer_head *bh;short int depth, i, ppos =0 , alloc =0 ; eh =ext_inode_hdr (inode); depth =ext_depth (inode); -if (ext4_ext_check (inode, eh, depth)) -return ERR_PTR (-EIO ); -/* account possible depth increase */ if (!path) {
@@ -634,10 +637,17 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block, path[ppos].p_depth = i; path[ppos].p_ext =NULL ; - bh =sb_bread (inode->i_sb, path[ppos].p_block); -if (!bh) + bh =sb_getblk (inode->i_sb, path[ppos].p_block); +if (unlikely (!bh)) goto err; - +if (!bh_uptodate_or_lock (bh)) { +if (bh_submit_read (bh) <0 ) { +put_bh (bh); + goto err; + } +/* validate the extent entries */ + need_to_validate =1 ; + } eh =ext_block_hdr (bh); ppos++;BUG_ON (ppos > depth);
@@ -645,7 +655,7 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block, path[ppos].p_hdr = eh; i--; -if (ext4_ext_check (inode, eh, i)) +if (need_to_validate &&ext4_ext_check (inode, eh, i)) goto err; }
fs/ext4/inode.c +5 -0
@@ -4273,6 +4273,11 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino) (__u64)(le32_to_cpu (raw_inode->i_version_hi)) <<32 ; } +if (ei->i_flags &EXT4_EXTENTS_FL ) { +/* Validate extent which is part of inode */ +ext4_ext_check_inode (inode); + } +if (S_ISREG (inode->i_mode)) { inode->i_op = &ext4_file_inode_operations; inode->i_fop = &ext4_file_operations;
Signature · 1 line
tg: (9561928..) extent_validate2 (depends on: extent_validate)
Re: [PATCH] Validate extent details only when read from the disk
Quoted 6 lines · Aneesh Kumar K.V
Make sure we validate extent details only when read from the disk.
fs/ext4/extents.c +0
-0@@ -602,15 +607,13 @@ struct ext4_ext_path * ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block, struct ext4_ext_path *path) {
fs/ext4/extents.c struct ext4_ext_path * · line 613
Quoted 10 lines
fs/ext4/extents.c +1
-3+int need_to_validate =0 ;struct ext4_extent_header *eh;struct buffer_head *bh;short int depth, i, ppos =0 , alloc =0 ; eh =ext_inode_hdr (inode); depth =ext_depth (inode); -if (ext4_ext_check (inode, eh, depth)) -return ERR_PTR (-EIO ); -
This check needs to stay in.
Kind regards,
Thiemo
Signature · 7 lines
+-----------------------------------+--------------------------+ | Dipl.-Phys. Thiemo Nagel | | | Technische Universitaet Muenchen | Room PH1 3276 | | Physik-Department E18 | | | James-Franck-Strasse | Phone +49 89 289-12379 | | D-85747 Garching | Fax +49 89 289-12570 | +-----------------------------------+--------------------------+
Re: [PATCH] Validate extent details only when read from the disk
Quoted 12 lines · Thiemo Nagel
Quoted 11 lines · Aneesh Kumar K.V
Make sure we validate extent details only when read from the disk.
fs/ext4/extents.c +1
-0@@ -602,15 +607,13 @@ struct ext4_ext_path * ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block, struct ext4_ext_path *path) { + int need_to_validate = 0;struct ext4_extent_header *eh;struct buffer_head *bh;short int depth, i, ppos =0 , alloc =0 ;
Quoted 6 lines
fs/ext4/extents.c struct ext4_ext_path * · line 613
Quoted 5 lines
fs/ext4/extents.c +0
-3eh =ext_inode_hdr (inode); depth =ext_depth (inode); -if (ext4_ext_check (inode, eh, depth)) -return ERR_PTR (-EIO ); -
I am doing the check in ext4_iget while reading the inode from disk.
-aneesh
Re: [PATCH] Validate extent details only when read from the disk
Quoted 13 lines · Aneesh Kumar K.V
Quoted 12 lines · Thiemo Nagel
Quoted 11 lines · Aneesh Kumar K.V
Make sure we validate extent details only when read from the disk.
fs/ext4/extents.c +1
-0@@ -602,15 +607,13 @@ struct ext4_ext_path * ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block, struct ext4_ext_path *path) { + int need_to_validate = 0;struct ext4_extent_header *eh;struct buffer_head *bh;short int depth, i, ppos =0 , alloc =0 ;
Quoted 7 lines
Quoted 5 lines
fs/ext4/extents.c struct ext4_ext_path * · line 613
Quoted 5 lines
fs/ext4/extents.c +0
-3eh =ext_inode_hdr (inode); depth =ext_depth (inode); -if (ext4_ext_check (inode, eh, depth)) -return ERR_PTR (-EIO ); -I am doing the check in
ext4_igetwhile reading the inode from disk.
You're right, there is a check. I didn't notice that, because ext4_iget() doesn't return an error.
fs/ext4/inode.c struct inode *ext4_iget(struct super_block *sb, unsigned long ino) · line 4280
Quoted 13 lines
fs/ext4/inode.c +5
-0@@ -4273,6 +4273,11 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino) (__u64)(le32_to_cpu (raw_inode->i_version_hi)) <<32 ; } +if (ei->i_flags &EXT4_EXTENTS_FL ) { +/* Validate extent which is part of inode */ +ext4_ext_check_inode (inode); + } +
I'd propose to change that to:
if (ei->i_flags & EXT4_EXTENTS_FL) {
/* Validate extent which is part of inode */
if ((ret = ext4_ext_check_inode(inode)))
goto bad_inode;
}Kind regards,
Thiemo
Re: [PATCH] Add checks to validate extent entries.
In reply to Aneesh Kumar K.VQuoted 2 lines · Aneesh Kumar K.V
This patch adds check to validate the extent entries along with extent headers. Should handle crash with corrupt filesystem.
While your patch probably decreases the average number of checks that are done, it makes single random accesses quite expensive since always complete extent blocks are checked at once, even if just a single pblock is accessed. No idea whether there are workloads which have that as a typical access pattern...
Kind regards,
Thiemo
Re: [PATCH] Add checks to validate extent entries.
Quoted 9 lines · Thiemo Nagel
Quoted 2 lines · Aneesh Kumar K.V
This patch adds check to validate the extent entries along with extent headers. Should handle crash with corrupt filesystem.
While your patch probably decreases the average number of checks that are done, it makes single random accesses quite expensive since always complete extent blocks are checked at once, even if just a single pblock is accessed. No idea whether there are workloads which have that as a typical access pattern...
The second patch make sure we do it only when we read the extent information from the disk.
-aneesh
Re: [PATCH] Add checks to validate extent entries.
Quoted 4 lines · Aneesh Kumar K.V
Quoted 3 lines · Thiemo Nagel
Quoted 2 lines · Aneesh Kumar K.V
This patch adds check to validate the extent entries along with extent headers. Should handle crash with corrupt filesystem.
Quoted 8 lines
Quoted 5 lines
While your patch probably decreases the average number of checks that are done, it makes single random accesses quite expensive since always complete extent blocks are checked at once, even if just a single pblock is accessed. No idea whether there are workloads which have that as a typical access pattern...
The second patch make sure we do it only when we read the extent information from the disk.
Imagine a random read of 4k somewhere inside a very large file. With your patch, we will loop over several kilobytes of extent tables just for that single read. (The second read will be faster, of course, but who knows whether there will be a second read?) So I think there exist some access patterns, for which your patch slows down things considerably, but I cannot judge whether these are important.
Kind regards,
Thiemo