On Apr 2, 2015, at 4:10 PM, Theodore Ts'o <tytso@xxxxxxx> wrote:
>
> From: Michael Halcrow <mhalcrow@xxxxxxxxxx>
>
> Change-Id: I1057e08bf05741b963705f2850710ec5d7d8bd72
> Signed-off-by: Uday Savagaonkar <savagaon@xxxxxxxxxx>
> Signed-off-by: Ildar Muslukhov <ildarm@xxxxxxxxxx>
> Signed-off-by: Michael Halcrow <mhalcrow@xxxxxxxxxx>
> Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>
> ---
> fs/ext4/dir.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
> fs/ext4/ialloc.c | 21 ++++++++++++--
> 2 files changed, 96 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index f67f955..a77900f 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -111,6 +111,11 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
> struct inode *inode = file_inode(file);
> struct super_block *sb = inode->i_sb;
> int dir_has_error = 0;
> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> + struct ext4_fname_crypto_ctx *enc_ctx = NULL;
> + struct ext4_str fname_crypto_str = {.name = NULL, .len = 0};
> +#endif
> + int res;
Having both "res" and "err" in the same function seems prone to mistakes.
>
> if (is_dx_dir(inode)) {
> err = ext4_dx_readdir(file, ctx);
> @@ -127,11 +132,27 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
>
> if (ext4_has_inline_data(inode)) {
> int has_inline_data = 1;
> - int ret = ext4_read_inline_dir(file, ctx,
> + res = ext4_read_inline_dir(file, ctx,
> &has_inline_data);
> if (has_inline_data)
> - return ret;
> + return res;
Is there any reason this can't use "err"? Or for that matter, "ret" (or
more commonly "rc") could stay local to this scope since it isn't actually
used elsewhere.
> + }
> +
> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> + enc_ctx = ext4_get_fname_crypto_ctx(inode, EXT4_NAME_LEN);
> + if (IS_ERR(enc_ctx))
> + return PTR_ERR(enc_ctx);
> + if (enc_ctx) {
> + res = ext4_fname_crypto_alloc_buffer(enc_ctx,
> + &fname_crypto_str.name,
> + &fname_crypto_str.len,
> + EXT4_NAME_LEN);
> + if (res < 0) {
> + ext4_put_fname_crypto_ctx(&enc_ctx);
> + return res;
> + }
Likewise here, "res" isn't used outside the scope of this if {} block
and could be declared locally. That shouldn't increase stack usage, and
makes it more clear to the reader that this value isn't used elsewhere.
> }
> +#endif
>
> offset = ctx->pos & (sb->s_blocksize - 1);
>
> @@ -226,13 +247,53 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
> offset += ext4_rec_len_from_disk(de->rec_len,
> sb->s_blocksize);
> if (le32_to_cpu(de->inode)) {
> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> + if (enc_ctx == NULL) {
> + /* Directory is not encrypted */
> + if (!dir_emit(ctx, de->name,
> + de->name_len,
> + le32_to_cpu(de->inode),
> + get_dtype(sb, de->file_type))) {
> + ext4_put_fname_crypto_ctx(
> + &enc_ctx);
> + ext4_fname_crypto_free_buffer(
> + (void **)&fname_crypto_str.name);
> + brelse(bh);
> + return 0;
> + }
This #ifdef and code should be restructured to avoid duplicating the
unencrypted case. Otherwise it is prone to bugs in one copy or the other.
> + } else {
> + /* Directory is encrypted */
> + err = ext4_fname_disk_to_usr(enc_ctx,
> + de, &fname_crypto_str);
> + if (err < 0) {
> + ext4_put_fname_crypto_ctx(
> + &enc_ctx);
> + ext4_fname_crypto_free_buffer(
> + (void **)&fname_crypto_str.name);
> + brelse(bh);
> + return err;
> + }
"err" isn't used outside the scope of this while {} block and could be
declared inside, preferably keeping the same name as the other local
temporary variables above.
> + if (!dir_emit(ctx,
> + fname_crypto_str.name, err,
> + le32_to_cpu(de->inode),
> + get_dtype(sb, de->file_type))) {
> + ext4_put_fname_crypto_ctx(
> + &enc_ctx);
> + ext4_fname_crypto_free_buffer(
> + (void **)&fname_crypto_str.name);
> + brelse(bh);
> + return 0;
> + }
> + }
> +#else
> if (!dir_emit(ctx, de->name,
> - de->name_len,
> - le32_to_cpu(de->inode),
> - get_dtype(sb, de->file_type))) {
> + de->name_len,
> + le32_to_cpu(de->inode),
> + get_dtype(sb, de->file_type))) {
> brelse(bh);
> return 0;
> }
> +#endif
> }
> ctx->pos += ext4_rec_len_from_disk(de->rec_len,
> sb->s_blocksize);
> @@ -240,10 +301,20 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
> offset = 0;
> brelse(bh);
> if (ctx->pos < inode->i_size) {
> - if (!dir_relax(inode))
> + if (!dir_relax(inode)) {
> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> + ext4_put_fname_crypto_ctx(&enc_ctx);
> + ext4_fname_crypto_free_buffer(
> + (void **)&fname_crypto_str.name);
> +#endif
Could ext4_put_fname_crypto_ctx() and ext4_fname_crypto_free_buffer()
be made no-ops in the !CONFIG_EXT4_FS_ENCRYPTION case? That would avoid
the #ifdefs here.
In general, there are a LOT of #ifdefs being being added by this patch
series, and it would be good to avoid it as much as possible.
> return 0;
> + }
> }
> }
> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> + ext4_put_fname_crypto_ctx(&enc_ctx);
> + ext4_fname_crypto_free_buffer((void **)&fname_crypto_str.name);
> +#endif
> return 0;
> }
>
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index e554ca3..8f37c9e 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -1034,11 +1034,28 @@ got:
> ext4_set_inode_state(inode, EXT4_STATE_NEW);
>
> ei->i_extra_isize = EXT4_SB(sb)->s_want_extra_isize;
> -
> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> + if ((sbi->s_file_encryption_mode == EXT4_ENCRYPTION_MODE_INVALID) &&
> + (sbi->s_dir_encryption_mode == EXT4_ENCRYPTION_MODE_INVALID)) {
> + ei->i_inline_off = 0;
> + if (EXT4_HAS_INCOMPAT_FEATURE(sb,
> + EXT4_FEATURE_INCOMPAT_INLINE_DATA))
This should be indented more, so it is more clear what is being continued.
> + ext4_set_inode_state(inode,
> + EXT4_STATE_MAY_INLINE_DATA);
Align after '(' so it is more clear what is being continued.
> + } else {
> + /* Inline data and encryption are incompatible
> + * We turn off inline data since encryption is enabled */
> + ei->i_inline_off = 1;
> + if (EXT4_HAS_INCOMPAT_FEATURE(sb,
> + EXT4_FEATURE_INCOMPAT_INLINE_DATA))
> + ext4_clear_inode_state(inode,
> + EXT4_STATE_MAY_INLINE_DATA);
Same.
> + }
> +#else
> ei->i_inline_off = 0;
> if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_INLINE_DATA))
> ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
It looks like the logic could be reversed here so that the non-encrypted
case doesn't need to be duplicated:
#ifdef CONFIG_EXT4_FS_ENCRYPTION
if (file_mode != INVALID || dir_mode != INVALID) {
/* Inline data and encryption are incompatible ... */
} else
#endif
{
ei->i_inline_off = 0;
if (EXT4_HAS_INCOMPAT_FEATURE(sb, ...)
ext4_set_inode_state(inode, MAY_INLINE_DATA);
}
Cheers, Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
