| Message ID | 20210120212641.526556-7-user@harshads-520.kir.corp.google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fast commit changes for e2fsprogs | expand |
On Wed, Jan 20, 2021 at 01:26:32PM -0800, Harshad Shirwadkar wrote: > From: Harshad Shirwadkar <harshadshirwadkar@gmail.com> > > This patch adds the following new APIs: > > Count the total number of blocks occupied by inode including > intermediate extent tree nodes. > extern blk64_t ext2fs_count_blocks(ext2_filsys fs, ext2_ino_t ino, > struct ext2_inode *inode); I wonder if this should be something like this instead: extern errcode_t ext2fs_count_blocks(ext2_filsys fs, ext2_ino_t ino, struct ext2_inode *inode, blk64_t *ret_count); The problem is that ext2fs_count_blocks() calls a whole series of ext2fs functions which could return errors: > + errcode = ext2fs_extent_open2(fs, ino, inode, &handle); > + if (errcode) > + goto out; > + > + errcode = ext2fs_extent_get(handle, EXT2_EXTENT_ROOT, &extent); > + if (errcode) > + goto out; ... and any of these functions could return an error. So we need to make sure errors are faithfully returned to the caller and handled correctly, instead of just having ext2fs_count_blocks returning a value of 0. I then started taking a look at the users of ext2fs_count_blocks() in e2fsck, and I ran into more concerns. One of the problems here is that some of these functions get called by kernel code --- and kernel code has a different error handling convetion of negative errno's. And in some cases, I see we are doing this: static int ext4_fc_handle_inode(e2fsck_t ctx, struct ext4_fc_tl *tl) { ... ret = ext2fs_read_inode_full(ctx->fs, ino, (struct ext2_inode *)inode, inode_len); if (ret) goto out; ... out: ext2fs_free_mem(&inode); return ret; } The problem here is that ext2fs_read_inode_full() returns an errcode_t, and this is getting cast to an int and returned as if it were a kernel error code. Also note that ext4_fc_replay() can return 0 or 1: #define JBD2_FC_REPLAY_STOP 0 #define JBD2_FC_REPLAY_CONTINUE 1 Fortunately, none of the functions that ext4_fc_*() call seem to be ones which could return in an ext2fs library returning EPERM (which is errno 1), but you see the potential risks of conflating an errcode_t and kernel negative errno convention. This is going to be a bit tricky to deal with, since an errcode_t can be a errno code, but it can also be one of the codes defined in lib/ext2fs/ext2_err.et, which get translated to numbers like: #define EXT2_ET_DIR_CORRUPTED (2133571363L) #define EXT2_ET_SHORT_READ (2133571364L) #define EXT2_ET_SHORT_WRITE (2133571365L) (See lib/ext2fs/ext2_err.h in the build directory of e2fsprogs and the com_err library found in lib/et.) So what we may need to do is to create a function which does a simple mapping of errcode_t values to negative errno's. It doesn't need to be exact; in fact, a first pass might just map all errcode_t's greater than 256 to something like -EFAULT, and all normal errno's to -errno. We might also want to have it print a diagnistic message to stderr that prints error_message(retval) was encoutered in function __func__ at line __LINE__. Hopefully in actual practice they won't happen (unless a malicious attacker is feeding us a fuzzed file sytem), but if it does, it would be good if there is a useful message so we can actually debug what happened. - Ted
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h index ec841006..fdcb28f6 100644 --- a/lib/ext2fs/ext2fs.h +++ b/lib/ext2fs/ext2fs.h @@ -1332,6 +1332,10 @@ extern errcode_t ext2fs_extent_fix_parents(ext2_extent_handle_t handle); extern size_t ext2fs_max_extent_depth(ext2_extent_handle_t handle); extern errcode_t ext2fs_fix_extents_checksums(ext2_filsys fs, ext2_ino_t ino, struct ext2_inode *inode); +extern blk64_t ext2fs_count_blocks(ext2_filsys fs, ext2_ino_t ino, + struct ext2_inode *inode); +extern errcode_t ext2fs_decode_extent(struct ext2fs_extent *to, void *from, + int len); /* fallocate.c */ #define EXT2_FALLOCATE_ZERO_BLOCKS (0x1) diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c index ac3dbfec..8d5fc1ab 100644 --- a/lib/ext2fs/extent.c +++ b/lib/ext2fs/extent.c @@ -1785,6 +1785,69 @@ out: return errcode; } +errcode_t ext2fs_decode_extent(struct ext2fs_extent *to, void *addr, int len) +{ + struct ext3_extent *from = (struct ext3_extent *)addr; + + if (len != sizeof(struct ext3_extent)) + return EXT2_ET_INVALID_ARGUMENT; + + to->e_pblk = ext2fs_le32_to_cpu(from->ee_start) + + ((__u64) ext2fs_le16_to_cpu(from->ee_start_hi) + << 32); + to->e_lblk = ext2fs_le32_to_cpu(from->ee_block); + to->e_len = ext2fs_le16_to_cpu(from->ee_len); + to->e_flags |= EXT2_EXTENT_FLAGS_LEAF; + if (to->e_len > EXT_INIT_MAX_LEN) { + to->e_len -= EXT_INIT_MAX_LEN; + to->e_flags |= EXT2_EXTENT_FLAGS_UNINIT; + } + + return 0; +} + +blk64_t ext2fs_count_blocks(ext2_filsys fs, ext2_ino_t ino, + struct ext2_inode *inode) +{ + ext2_extent_handle_t handle; + struct ext2fs_extent extent; + errcode_t errcode; + int i; + blk64_t blkcount = 0; + blk64_t *intermediate_nodes; + + errcode = ext2fs_extent_open2(fs, ino, inode, &handle); + if (errcode) + goto out; + + errcode = ext2fs_extent_get(handle, EXT2_EXTENT_ROOT, &extent); + if (errcode) + goto out; + + ext2fs_get_array(handle->max_depth, sizeof(blk64_t), + &intermediate_nodes); + blkcount = handle->level; + while (!errcode) { + if (extent.e_flags & EXT2_EXTENT_FLAGS_LEAF) { + blkcount += extent.e_len; + for (i = 0; i < handle->level; i++) { + if (intermediate_nodes[i] != + handle->path[i].end_blk) { + blkcount++; + intermediate_nodes[i] = + handle->path[i].end_blk; + } + } + } + errcode = ext2fs_extent_get(handle, EXT2_EXTENT_NEXT, &extent); + } + ext2fs_free_mem(&intermediate_nodes); +out: + ext2fs_extent_free(handle); + + return blkcount; +} + #ifdef DEBUG /* * Override debugfs's prompt