Re: [PATCH] ext4 crypto: migrate into vfs's crypto engine

[Jaegeuk Kim]

Hi Eric,

Thank you for the review.

On Fri, May 06, 2016 at 09:31:02PM -0500, Eric Biggers wrote:
> Hi Jaegeuk,
> 
> On Mon, Apr 25, 2016 at 05:15:36PM -0700, Jaegeuk Kim wrote:
> > This patch removes the most parts of internal crypto codes.
> > And then, it modifies and adds some ext4-specific crypt codes to use the generic
> > facility.
> 
> Except for the key name prefix issue that Ted pointed out, this overall seems
> good, although I didn't read into every detail and haven't yet tested the code.
> A few comments:
> 
> There are compiler errors and warnings in the function 'dx_show_leaf()', which
> is not compiled by default.

Fixed.

> 
> In ext4_lookup():
> >               /*
> >                * DCACHE_ENCRYPTED_WITH_KEY is set if the dentry is
> >                * created while the directory was encrypted and we
> >                * don't have access to the key.
> >                */
> >               if (fscrypt_has_encryption_key(dir))
> >                       fscrypt_set_encrypted_dentry(dentry);
> 
> Shouldn't this say "and we have access to the key"?  Or is the code wrong?

Done.

> 
> In ext4_empty_dir():
> >       bool err = false;
> 
> Since this is a bool it should not be called "err".  Maybe call it "empty"
> instead.

Removed and worked around it.

> 
> In ext4_finish_bio():
> >               if (!page->mapping) {
> >                       /* The bounce data pages are unmapped. */
> >                       data_page = page;
> >                       fscrypt_pullback_bio_page(&page, false);
> >               }
> ...
> >#ifdef CONFIG_EXT4_FS_ENCRYPTION
> >                       if (data_page)
> >                               fscrypt_restore_control_page(data_page);
> >#endif
> 
> Does this always do the same thing as the previous code?  Does !page->mapping
> always imply that the page was involved in encrypted I/O?

I think so.

> 
> In ext4_encrypted_get_link():
> >       if ((cstr.len + 
> >            sizeof(struct fscrypt_symlink_data) - 1) >
> >           max_size) {
> 
> Make this one line?

Done.

> 
> In ext4_file_mmap()
> >               int err = fscrypt_get_encryption_info(inode);
> >               if (err)
> >                       return 0;
> 
> Should the error code be propagated to the caller?

This patch tries to keep existing flow as much as possible.

> 
> In ext4_ioctl():
> >       case EXT4_IOC_GET_ENCRYPTION_POLICY: {
> >#ifdef CONFIG_EXT4_FS_ENCRYPTION
> >               struct fscrypt_policy policy;
> >               int err = 0;
> >
> >               if (!ext4_encrypted_inode(inode))
> >                       return -ENOENT;
> 
> This is existing code and I do not know if it can be changed, but I feel that
> ENOENT is a not good error code here.  If the ext4_encrypted_inode() check were
> to be removed, the implementation would match f2fs and the error code would be
> ENODATA instead.

ditto.

> 
> - Eric