[+Cc linux-fscrypt]

Hi Sheng,

On Tue, Mar 06, 2018 at 11:39:04AM +0800, Sheng Yong wrote:
> This patch introduces a new feature, F2FS_FEATURE_LOST_FOUND, which
> is set by mkfs. It creates a directory named lost+found, which saves
> unreachable files. If fsck finds a file which has no parent, or its
> parent is removed by fsck, the file will be placed under lost+found
> directory by fsck.
> 
> lost+found directory could not be encrypted. As a result, the root
> directory cannot be encrypted too. So if LOST_FOUND feature is enabled,
> let's avoid to encrypt root directory.
> 
> This patch also introduces a new mount option `test_dummy_encryption'
> to allow fscrypt to create a fake fscrypt context. This is used by
> xfstests.

It would be a bit easier to review if this was split into 2 patches:

1. Add LOST_FOUND feature
2. Add test_dummy_encryption mount option

> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 797eb05cb538..7f908be8d525 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -361,6 +361,7 @@ struct page *init_inode_metadata(struct inode *inode, 
> struct inode *dir,
>                       struct page *dpage)
>  {
>       struct page *page;
> +     int encrypt = DUMMY_ENCRYPTION_ENABLED(F2FS_I_SB(dir));
>       int err;
>  
>       if (is_inode_flag_set(inode, FI_NEW_INODE)) {
> @@ -387,7 +388,8 @@ struct page *init_inode_metadata(struct inode *inode, 
> struct inode *dir,
>               if (err)
>                       goto put_error;
>  
> -             if (f2fs_encrypted_inode(dir) && f2fs_may_encrypt(inode)) {
> +             if ((f2fs_encrypted_inode(dir) || encrypt) &&
> +                                     f2fs_may_encrypt(inode)) {
>                       err = fscrypt_inherit_context(dir, inode, page, false);
>                       if (err)
>                               goto put_error;
> @@ -402,7 +404,7 @@ struct page *init_inode_metadata(struct inode *inode, 
> struct inode *dir,
>  
>       if (new_name) {
>               init_dent_inode(new_name, page);
> -             if (f2fs_encrypted_inode(dir))
> +             if (f2fs_encrypted_inode(dir) || encrypt)
>                       file_set_enc_name(inode);
>       }
>  

'enc_name' should only be set if the parent directory is encrypted, right?  So
the condition for file_set_enc_name() should stay 'f2fs_encrypted_inode(dir)'.

> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> @@ -719,6 +721,18 @@ static int parse_options(struct super_block *sb, char 
> *options)
>                       }
>                       kfree(name);
>                       break;
> +#ifdef CONFIG_F2FS_FS_ENCRYPTION
> +             case Opt_test_dummy_encryption:
> +                     sbi->mount_flags |= F2FS_MF_TEST_DUMMY_ENCRYPTION;
> +                     f2fs_msg(sb, KERN_INFO,
> +                                     "Test dummy encryption mode enabled");
> +                     break;
> +#else
> +             case Opt_test_dummy_encryption:
> +                     f2fs_msg(sb, KERN_INFO,
> +                                     "Test dummy encryption mount option 
> ignored");
> +                     break;
> +#endif

You could write the 'case Opt_test_dummy_encryption:" and break just once, and
put the #ifdef inside it.

> @@ -2696,6 +2732,12 @@ static int f2fs_fill_super(struct super_block *sb, 
> void *data, int silent)
>               }
>       }
>  
> +     if (DUMMY_ENCRYPTION_ENABLED(sbi) && !f2fs_readonly(sb) &&
> +                             !f2fs_sb_has_encrypt(sb)) {
> +             f2fs_sb_set_encrypt(sb);
> +             recovery = 1;
> +     }
> +
[...]
>       /* get an inode for meta space */
>       sbi->meta_inode = f2fs_iget(sb, F2FS_META_INO(sbi));
>       if (IS_ERR(sbi->meta_inode)) {
> @@ -2870,12 +2912,16 @@ static int f2fs_fill_super(struct super_block *sb, 
> void *data, int silent)
>       }
>       kfree(options);
>  
> -     /* recover broken superblock */
> +     /* recover broken superblock or enable encrypt feature */
>       if (recovery) {
> -             err = f2fs_commit_super(sbi, true);
> -             f2fs_msg(sb, KERN_INFO,
> -                     "Try to recover %dth superblock, ret: %d",
> -                     sbi->valid_super_block ? 1 : 2, err);
> +             if (sbi->mount_flags & F2FS_MF_TEST_DUMMY_ENCRYPTION) {
> +                     f2fs_commit_super(sbi, false);
> +             } else {
> +                     err = f2fs_commit_super(sbi, true);
> +                     f2fs_msg(sb, KERN_INFO,
> +                             "Try to recover %dth superblock, ret: %d",
> +                             sbi->valid_super_block ? 1 : 2, err);
> +             }
>       }

How about just not allowing mounting with test_dummy_encryption if the 'encrypt'
feature flag isn't already set?  That would be simpler.  I think it was a
mistake that ext4 automatically turns the 'encrypt' flag on when
test_dummy_encryption is specified.

- Eric

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Reply via email to