Re: [RFC PATCH v3] f2fs: introduce F2FS_FEATURE_LOST_FOUND feature

[Eric Biggers via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

[+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