On Wed, Jan 20, 2021 at 01:26:32PM -0800, Harshad Shirwadkar wrote:
> From: Harshad Shirwadkar <harshadshirwadkar@xxxxxxxxx>
>
> 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
