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:

quoted

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():

quoted

              /*
               * 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():

quoted

      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():

quoted

              if (!page->mapping) {
                      /* The bounce data pages are unmapped. */
                      data_page = page;
                      fscrypt_pullback_bio_page(&page, false);
              }

...

quoted

#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():

quoted

      if ((cstr.len + 
           sizeof(struct fscrypt_symlink_data) - 1) >
          max_size) {

Make this one line?

Done.

In ext4_file_mmap()

quoted

              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():

quoted

      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