Message ID | C26C67884C4B7A48980D574635594864E16DD8@BPXM20GP.gisp.nec.co.jp |
---|---|
State | Superseded, archived |
Headers | show |
On Wed, Mar 26, 2014 at 05:20:54AM +0000, Kazuya Mio wrote: > When we try to get 2^32-1 block of the file which has the extent > (ee_block=2^32-2, ee_len=1) with FIBMAP ioctl, it causes BUG_ON > in ext4_ext_put_gap_in_cache(). We should be returning an error when we pass in an lblk >= EXT4_MAX_BLOCKS in ext4_map_blocks(), long before we even get to ext4_ext_put_gap_in_cache(). And if we fix it there, we may catch other cases which might lead to the BUG_ON() firing. Also, how ext4_bmap() is currently being implemented is spectacularly ugly; I have no idea why we are calling generic_block_bmap() instead of calling ext4_map_blocks() directly. Since FIBMAP requires root privs, I'd much rather fix this at our leisure post merge-window. If this same bug can be triggered by FIEMAP, then we really do need to fix this right away, since FIEMAP can be used by anybody. (But then we should be fixing it in ext4_map_blocks, not in ext4_bmap.) Did you check whether the same bug can be triggered via FIEMAP? Thanks, - 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
2014/04/01 14:15:16, Theodore Ts'o wrote: > We should be returning an error when we pass in an lblk >= > EXT4_MAX_BLOCKS in ext4_map_blocks(), long before we even get to > ext4_ext_put_gap_in_cache(). And if we fix it there, we may catch > other cases which might lead to the BUG_ON() firing. Agree. I'll send the fixed patch latter. > Did you check whether the same bug can be triggered via FIEMAP? FIEMAP ioctl is safe because ext4_fiemap() has the following check. last_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits; if (last_blk >= EXT_MAX_BLOCKS) last_blk = EXT_MAX_BLOCKS-1; AFAIK, FIBMAP is only the way to pass the block number specified by a user to ext4_map_blocks(). Regards, Kazuya Mio -- 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/inode.c b/fs/ext4/inode.c index 61d49ff..aef3501 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -2873,6 +2873,12 @@ static sector_t ext4_bmap(struct address_space *mapping, sector_t block) if (ext4_has_inline_data(inode)) return 0; + /* + * We cannot get physical block number over EXT_MAX_BLOCKS + */ + if ((ext4_lblk_t)block >= EXT_MAX_BLOCKS) + return 0; + if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) && test_opt(inode->i_sb, DELALLOC)) { /*
When we try to get 2^32-1 block of the file which has the extent (ee_block=2^32-2, ee_len=1) with FIBMAP ioctl, it causes BUG_ON in ext4_ext_put_gap_in_cache(). To avoid the problem, ext4_bmap() needs to check the file logical block number. ext4_ext_put_gap_in_cache() called via ext4_get_block() cannot handle 2^32-1 because the maximum file logical block number is 2^32-2. However, the block number in ext4_bmap() which is gotten from user space is passed to ext4_get_block() directly. Signed-off-by: Kazuya Mio <k-mio@sx.jp.nec.com> --- fs/ext4/inode.c | 6 ++++++ 1 file changed, 6 insertions(+) -- 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