[2/7] f2fs: use IS_ENCRYPTED() to check encryption status
diff mbox series

Message ID 20181119052324.31456-3-chandan@linux.vnet.ibm.com
State Superseded
Headers show
Series
  • Remove fs specific fscrypt and fsverity build config options
Related show

Commit Message

Chandan Rajendra Nov. 19, 2018, 5:23 a.m. UTC
This commit removes the f2fs specific f2fs_encrypted_inode() and makes
use of the generic IS_ENCRYPTED() macro to check for the encryption
status of an inode.

Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
---
 fs/f2fs/data.c  |  4 ++--
 fs/f2fs/dir.c   | 10 +++++-----
 fs/f2fs/file.c  | 10 +++++-----
 fs/f2fs/namei.c |  6 +++---
 4 files changed, 15 insertions(+), 15 deletions(-)

Comments

Chao Yu Nov. 19, 2018, 6:24 a.m. UTC | #1
On 2018/11/19 13:23, Chandan Rajendra wrote:
> This commit removes the f2fs specific f2fs_encrypted_inode() and makes
> use of the generic IS_ENCRYPTED() macro to check for the encryption
> status of an inode.
> 
> Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>

Acked-by: Chao Yu <yuchao0@huawei.com>

Thanks,
Jaegeuk Kim Nov. 19, 2018, 9:23 p.m. UTC | #2
Hi Chandan,

Let me try to queue and test this patch separately from the patch-set in order
to avoid merge conflicts. So, I think IS_ENCRYPTED should be merged in f2fs/ext4
branches, and the others can be applied to fscrypt and fsverity into each trees.

Thanks,

On 11/19, Chandan Rajendra wrote:
> This commit removes the f2fs specific f2fs_encrypted_inode() and makes
> use of the generic IS_ENCRYPTED() macro to check for the encryption
> status of an inode.
> 
> Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> ---
>  fs/f2fs/data.c  |  4 ++--
>  fs/f2fs/dir.c   | 10 +++++-----
>  fs/f2fs/file.c  | 10 +++++-----
>  fs/f2fs/namei.c |  6 +++---
>  4 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 09d9fc1676a7..844ec573263e 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1491,7 +1491,7 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  	}
>  
>  	if (size) {
> -		if (f2fs_encrypted_inode(inode))
> +		if (IS_ENCRYPTED(inode))
>  			flags |= FIEMAP_EXTENT_DATA_ENCRYPTED;
>  
>  		ret = fiemap_fill_next_extent(fieinfo, logical,
> @@ -1764,7 +1764,7 @@ static inline bool check_inplace_update_policy(struct inode *inode,
>  	if (policy & (0x1 << F2FS_IPU_ASYNC) &&
>  			fio && fio->op == REQ_OP_WRITE &&
>  			!(fio->op_flags & REQ_SYNC) &&
> -			!f2fs_encrypted_inode(inode))
> +			!IS_ENCRYPTED(inode))
>  		return true;
>  
>  	/* this is only set during fdatasync */
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index bacc667950b6..cf9e2564388d 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -385,7 +385,7 @@ struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir,
>  		if (err)
>  			goto put_error;
>  
> -		if ((f2fs_encrypted_inode(dir) || dummy_encrypt) &&
> +		if ((IS_ENCRYPTED(dir) || dummy_encrypt) &&
>  					f2fs_may_encrypt(inode)) {
>  			err = fscrypt_inherit_context(dir, inode, page, false);
>  			if (err)
> @@ -399,7 +399,7 @@ struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir,
>  
>  	if (new_name) {
>  		init_dent_inode(new_name, page);
> -		if (f2fs_encrypted_inode(dir))
> +		if (IS_ENCRYPTED(dir))
>  			file_set_enc_name(inode);
>  	}
>  
> @@ -808,7 +808,7 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
>  		de_name.name = d->filename[bit_pos];
>  		de_name.len = le16_to_cpu(de->name_len);
>  
> -		if (f2fs_encrypted_inode(d->inode)) {
> +		if (IS_ENCRYPTED(d->inode)) {
>  			int save_len = fstr->len;
>  
>  			err = fscrypt_fname_disk_to_usr(d->inode,
> @@ -852,7 +852,7 @@ static int f2fs_readdir(struct file *file, struct dir_context *ctx)
>  	struct fscrypt_str fstr = FSTR_INIT(NULL, 0);
>  	int err = 0;
>  
> -	if (f2fs_encrypted_inode(inode)) {
> +	if (IS_ENCRYPTED(inode)) {
>  		err = fscrypt_get_encryption_info(inode);
>  		if (err && err != -ENOKEY)
>  			goto out;
> @@ -914,7 +914,7 @@ static int f2fs_readdir(struct file *file, struct dir_context *ctx)
>  
>  static int f2fs_dir_open(struct inode *inode, struct file *filp)
>  {
> -	if (f2fs_encrypted_inode(inode))
> +	if (IS_ENCRYPTED(inode))
>  		return fscrypt_get_encryption_info(inode) ? -EACCES : 0;
>  	return 0;
>  }
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 87794b2a45ff..6c7ad15000b9 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -585,7 +585,7 @@ static int truncate_partial_data_page(struct inode *inode, u64 from,
>  	zero_user(page, offset, PAGE_SIZE - offset);
>  
>  	/* An encrypted inode should have a key and truncate the last page. */
> -	f2fs_bug_on(F2FS_I_SB(inode), cache_only && f2fs_encrypted_inode(inode));
> +	f2fs_bug_on(F2FS_I_SB(inode), cache_only && IS_ENCRYPTED(inode));
>  	if (!cache_only)
>  		set_page_dirty(page);
>  	f2fs_put_page(page, 1);
> @@ -730,7 +730,7 @@ int f2fs_getattr(const struct path *path, struct kstat *stat,
>  		stat->attributes |= STATX_ATTR_APPEND;
>  	if (flags & F2FS_COMPR_FL)
>  		stat->attributes |= STATX_ATTR_COMPRESSED;
> -	if (f2fs_encrypted_inode(inode))
> +	if (IS_ENCRYPTED(inode))
>  		stat->attributes |= STATX_ATTR_ENCRYPTED;
>  	if (flags & F2FS_IMMUTABLE_FL)
>  		stat->attributes |= STATX_ATTR_IMMUTABLE;
> @@ -1587,7 +1587,7 @@ static long f2fs_fallocate(struct file *file, int mode,
>  	if (!S_ISREG(inode->i_mode))
>  		return -EINVAL;
>  
> -	if (f2fs_encrypted_inode(inode) &&
> +	if (IS_ENCRYPTED(inode) &&
>  		(mode & (FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_INSERT_RANGE)))
>  		return -EOPNOTSUPP;
>  
> @@ -1671,7 +1671,7 @@ static int f2fs_ioc_getflags(struct file *filp, unsigned long arg)
>  	struct f2fs_inode_info *fi = F2FS_I(inode);
>  	unsigned int flags = fi->i_flags;
>  
> -	if (f2fs_encrypted_inode(inode))
> +	if (IS_ENCRYPTED(inode))
>  		flags |= F2FS_ENCRYPT_FL;
>  	if (f2fs_has_inline_data(inode) || f2fs_has_inline_dentry(inode))
>  		flags |= F2FS_INLINE_DATA_FL;
> @@ -2430,7 +2430,7 @@ static int f2fs_move_file_range(struct file *file_in, loff_t pos_in,
>  	if (!S_ISREG(src->i_mode) || !S_ISREG(dst->i_mode))
>  		return -EINVAL;
>  
> -	if (f2fs_encrypted_inode(src) || f2fs_encrypted_inode(dst))
> +	if (IS_ENCRYPTED(src) || IS_ENCRYPTED(dst))
>  		return -EOPNOTSUPP;
>  
>  	if (src == dst) {
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 99299ede7429..6ae37e0cf6e3 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -75,7 +75,7 @@ static struct inode *f2fs_new_inode(struct inode *dir, umode_t mode)
>  	set_inode_flag(inode, FI_NEW_INODE);
>  
>  	/* If the directory encrypted, then we should encrypt the inode. */
> -	if ((f2fs_encrypted_inode(dir) || DUMMY_ENCRYPTION_ENABLED(sbi)) &&
> +	if ((IS_ENCRYPTED(dir) || DUMMY_ENCRYPTION_ENABLED(sbi)) &&
>  				f2fs_may_encrypt(inode))
>  		f2fs_set_encrypted_inode(inode);
>  
> @@ -476,7 +476,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
>  		if (err)
>  			goto out_iput;
>  	}
> -	if (f2fs_encrypted_inode(dir) &&
> +	if (IS_ENCRYPTED(dir) &&
>  	    (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) &&
>  	    !fscrypt_has_permitted_context(dir, inode)) {
>  		f2fs_msg(inode->i_sb, KERN_WARNING,
> @@ -803,7 +803,7 @@ static int f2fs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
>  	if (unlikely(f2fs_cp_error(sbi)))
>  		return -EIO;
>  
> -	if (f2fs_encrypted_inode(dir) || DUMMY_ENCRYPTION_ENABLED(sbi)) {
> +	if (IS_ENCRYPTED(dir) || DUMMY_ENCRYPTION_ENABLED(sbi)) {
>  		int err = fscrypt_get_encryption_info(dir);
>  		if (err)
>  			return err;
> -- 
> 2.19.1
> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Theodore Ts'o Nov. 26, 2018, 3:41 a.m. UTC | #3
On Mon, Nov 19, 2018 at 01:23:32PM -0800, Jaegeuk Kim wrote:
> Hi Chandan,
> 
> Let me try to queue and test this patch separately from the patch-set in order
> to avoid merge conflicts. So, I think IS_ENCRYPTED should be merged in f2fs/ext4
> branches, and the others can be applied to fscrypt and fsverity into each trees.

Hi Jaeguk,

What conflicts are you anticipating?  I can't apply the rest of
Chandan's patches without unless we apply them in order, and I'd
prefer to avoid cross-merging, or spreading out this series across two
kernel releases.

						- Ted
Theodore Ts'o Nov. 26, 2018, 4 a.m. UTC | #4
On Sun, Nov 25, 2018 at 10:41:32PM -0500, Theodore Y. Ts'o wrote:
> On Mon, Nov 19, 2018 at 01:23:32PM -0800, Jaegeuk Kim wrote:
> > Hi Chandan,
> > 
> > Let me try to queue and test this patch separately from the patch-set in order
> > to avoid merge conflicts. So, I think IS_ENCRYPTED should be merged in f2fs/ext4
> > branches, and the others can be applied to fscrypt and fsverity into each trees.
> 
> Hi Jaeguk,
> 
> What conflicts are you anticipating?  I can't apply the rest of
> Chandan's patches without unless we apply them in order, and I'd
> prefer to avoid cross-merging, or spreading out this series across two
> kernel releases.

Oh, I see the problem.  It's commit 79c66e7572 ("f2fs: clean up
f2fs_sb_has_##feature_name") on the f2fs dev branch.  It changes the
function signature of the f2fs_sb_has_<feature>() functions.

This is going to be a problem for merging the fsverity patch series as
well.   How stable are these patches on the f2fs dev branch?

79c66e75720c f2fs: clean up f2fs_sb_has_##feature_name
6a917e69d3b8 f2fs: remove codes of unused wio_mutex
67bdd2a68f0a f2fs: fix count of seg_freed to make sec_freed correct
a5c7029ba357 f2fs: fix to account preflush command for noflush_merge mode
83effa8865cc f2fs: avoid GC causing encrypted file corrupted

It might be that the simplest way to solve things is to merge the f2fs
dev branch up to 79c66e75720c.  This will have the net effect of
including the five patches listed above onto the fscrypt git tree.  So
long you don't plan to rebase or otherwise change these five patches,
it should avoid any merge conflicts.

						- Ted
Theodore Ts'o Nov. 26, 2018, 5:34 p.m. UTC | #5
On Sun, Nov 25, 2018 at 11:00:38PM -0500, Theodore Y. Ts'o wrote:
> 
> It might be that the simplest way to solve things is to merge the f2fs
> dev branch up to 79c66e75720c.  This will have the net effect of
> including the five patches listed above onto the fscrypt git tree.  So
> long you don't plan to rebase or otherwise change these five patches,
> it should avoid any merge conflicts.

I've set up a git branch which has the f2fs dev branch, 4.20-rc4, the
fsverity patches, and part of Chandan's patch series here:

git://git.kernel.org/pub/scm/linux/kernel/git/tytso/fscrypt.git test-working

There is a minor conflict when I did a trial merge with f2fs.git's dev
branch, but it's pretty obvious to how to resolve it.

Jaegeuk, Eric, Chandan, please take a look and let me know what you
think.

	    	 		       	  - Ted
Jaegeuk Kim Nov. 26, 2018, 11:52 p.m. UTC | #6
Hi Ted,

On 11/26, Theodore Y. Ts'o wrote:
> On Sun, Nov 25, 2018 at 11:00:38PM -0500, Theodore Y. Ts'o wrote:
> > 
> > It might be that the simplest way to solve things is to merge the f2fs
> > dev branch up to 79c66e75720c.  This will have the net effect of
> > including the five patches listed above onto the fscrypt git tree.  So
> > long you don't plan to rebase or otherwise change these five patches,
> > it should avoid any merge conflicts.
> 
> I've set up a git branch which has the f2fs dev branch, 4.20-rc4, the
> fsverity patches, and part of Chandan's patch series here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/tytso/fscrypt.git test-working
> 
> There is a minor conflict when I did a trial merge with f2fs.git's dev
> branch, but it's pretty obvious to how to resolve it.
> 
> Jaegeuk, Eric, Chandan, please take a look and let me know what you
> think.

I was about to rebase f2fs-dev branch to catch up -rc4, so could you please
update test-working with the rebased one? Then, I'll test Eric & Chandan's
patches in the test-working branch locally. If there is no issue, I'm okay
to push all of their work via fscrypt.git, if you prefer.

Afterward, merging f2fs patches till "f2fs: clean up f2fs_sb_has_##feature_name"
into the test-working branch would be fine as well.

Thanks,

> 
> 	    	 		       	  - Ted
Chandan Rajendra Nov. 29, 2018, 10:38 a.m. UTC | #7
On Monday, November 26, 2018 11:04:35 PM IST Theodore Y. Ts'o wrote:
> On Sun, Nov 25, 2018 at 11:00:38PM -0500, Theodore Y. Ts'o wrote:
> > 
> > It might be that the simplest way to solve things is to merge the f2fs
> > dev branch up to 79c66e75720c.  This will have the net effect of
> > including the five patches listed above onto the fscrypt git tree.  So
> > long you don't plan to rebase or otherwise change these five patches,
> > it should avoid any merge conflicts.
> 
> I've set up a git branch which has the f2fs dev branch, 4.20-rc4, the
> fsverity patches, and part of Chandan's patch series here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/tytso/fscrypt.git test-working
> 
> There is a minor conflict when I did a trial merge with f2fs.git's dev
> branch, but it's pretty obvious to how to resolve it.
> 
> Jaegeuk, Eric, Chandan, please take a look and let me know what you
> think.

Ted,

I have addressed the review comments provided by Eric. Hence three out of
the four patches in the test-working branch have new changes. I also got
UBIFS to use CONFIG_FS_ENCRYPTION instead of the per-filesystem config
symbol.

I am currently executing fstests to verify the changes.


Eric,

When executing generic/900, I noticed that sometimes xfs_io would get stuck
for an indefinite period. /proc/<pid of xfs_io>/stack showed that the task was
stuck in tty_read() inside the kernel. The following change fixed it,

diff --git a/tests/generic/900 b/tests/generic/900
index 290889ce..0831eed4 100755
--- a/tests/generic/900
+++ b/tests/generic/900
@@ -83,7 +83,7 @@ _fsv_create_enable_file $fsv_file >> $seqres.full
 echo "* reading"
 $XFS_IO_PROG -r $fsv_file -c ''
 echo "* xfs_io writing, should be O_RDWR"
-$XFS_IO_PROG $fsv_file |& _filter_scratch
+$XFS_IO_PROG -c '' $fsv_file 2>&1 | _filter_scratch
 echo "* bash >>, should be O_APPEND"
 bash -c "echo >> $fsv_file" |& _filter_scratch
 echo "* bash >, should be O_WRONLY|O_CREAT|O_TRUNC"

xfs_io gets into interactive mode when invoked without a "-c cmd" string.

However, I am not able recreate the scenario once again without the above
changes applied. I am not sure about what changed.
Eric Biggers Nov. 29, 2018, 7:05 p.m. UTC | #8
Hi Chandan,

On Thu, Nov 29, 2018 at 04:08:31PM +0530, Chandan Rajendra wrote:
> On Monday, November 26, 2018 11:04:35 PM IST Theodore Y. Ts'o wrote:
> > On Sun, Nov 25, 2018 at 11:00:38PM -0500, Theodore Y. Ts'o wrote:
> > > 
> > > It might be that the simplest way to solve things is to merge the f2fs
> > > dev branch up to 79c66e75720c.  This will have the net effect of
> > > including the five patches listed above onto the fscrypt git tree.  So
> > > long you don't plan to rebase or otherwise change these five patches,
> > > it should avoid any merge conflicts.
> > 
> > I've set up a git branch which has the f2fs dev branch, 4.20-rc4, the
> > fsverity patches, and part of Chandan's patch series here:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/tytso/fscrypt.git test-working
> > 
> > There is a minor conflict when I did a trial merge with f2fs.git's dev
> > branch, but it's pretty obvious to how to resolve it.
> > 
> > Jaegeuk, Eric, Chandan, please take a look and let me know what you
> > think.
> 
> Ted,
> 
> I have addressed the review comments provided by Eric. Hence three out of
> the four patches in the test-working branch have new changes. I also got
> UBIFS to use CONFIG_FS_ENCRYPTION instead of the per-filesystem config
> symbol.
> 
> I am currently executing fstests to verify the changes.
> 
> 
> Eric,
> 
> When executing generic/900, I noticed that sometimes xfs_io would get stuck
> for an indefinite period. /proc/<pid of xfs_io>/stack showed that the task was
> stuck in tty_read() inside the kernel. The following change fixed it,
> 
> diff --git a/tests/generic/900 b/tests/generic/900
> index 290889ce..0831eed4 100755
> --- a/tests/generic/900
> +++ b/tests/generic/900
> @@ -83,7 +83,7 @@ _fsv_create_enable_file $fsv_file >> $seqres.full
>  echo "* reading"
>  $XFS_IO_PROG -r $fsv_file -c ''
>  echo "* xfs_io writing, should be O_RDWR"
> -$XFS_IO_PROG $fsv_file |& _filter_scratch
> +$XFS_IO_PROG -c '' $fsv_file 2>&1 | _filter_scratch
>  echo "* bash >>, should be O_APPEND"
>  bash -c "echo >> $fsv_file" |& _filter_scratch
>  echo "* bash >, should be O_WRONLY|O_CREAT|O_TRUNC"
> 
> xfs_io gets into interactive mode when invoked without a "-c cmd" string.
> 
> However, I am not able recreate the scenario once again without the above
> changes applied. I am not sure about what changed. 
> 

The test is opening a verity file for read+write access, which should fail.  But
it's incorrectly succeeding, hence the test is right to not pass.  Did you add
the missing call to ext4_set_inode_flags() in ext4_set_verity() as I suggested?

(But I'll make the suggested change to the test too, so it fails cleanly in this
case rather than hangs reading from stdin.)

- Eric
Chandan Rajendra Nov. 30, 2018, 5:27 a.m. UTC | #9
On Friday, November 30, 2018 12:35:13 AM IST Eric Biggers wrote:
> Hi Chandan,
> 
> On Thu, Nov 29, 2018 at 04:08:31PM +0530, Chandan Rajendra wrote:
> > On Monday, November 26, 2018 11:04:35 PM IST Theodore Y. Ts'o wrote:
> > > On Sun, Nov 25, 2018 at 11:00:38PM -0500, Theodore Y. Ts'o wrote:
> > > > 
> > > > It might be that the simplest way to solve things is to merge the f2fs
> > > > dev branch up to 79c66e75720c.  This will have the net effect of
> > > > including the five patches listed above onto the fscrypt git tree.  So
> > > > long you don't plan to rebase or otherwise change these five patches,
> > > > it should avoid any merge conflicts.
> > > 
> > > I've set up a git branch which has the f2fs dev branch, 4.20-rc4, the
> > > fsverity patches, and part of Chandan's patch series here:
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/tytso/fscrypt.git test-working
> > > 
> > > There is a minor conflict when I did a trial merge with f2fs.git's dev
> > > branch, but it's pretty obvious to how to resolve it.
> > > 
> > > Jaegeuk, Eric, Chandan, please take a look and let me know what you
> > > think.
> > 
> > Ted,
> > 
> > I have addressed the review comments provided by Eric. Hence three out of
> > the four patches in the test-working branch have new changes. I also got
> > UBIFS to use CONFIG_FS_ENCRYPTION instead of the per-filesystem config
> > symbol.
> > 
> > I am currently executing fstests to verify the changes.
> > 
> > 
> > Eric,
> > 
> > When executing generic/900, I noticed that sometimes xfs_io would get stuck
> > for an indefinite period. /proc/<pid of xfs_io>/stack showed that the task was
> > stuck in tty_read() inside the kernel. The following change fixed it,
> > 
> > diff --git a/tests/generic/900 b/tests/generic/900
> > index 290889ce..0831eed4 100755
> > --- a/tests/generic/900
> > +++ b/tests/generic/900
> > @@ -83,7 +83,7 @@ _fsv_create_enable_file $fsv_file >> $seqres.full
> >  echo "* reading"
> >  $XFS_IO_PROG -r $fsv_file -c ''
> >  echo "* xfs_io writing, should be O_RDWR"
> > -$XFS_IO_PROG $fsv_file |& _filter_scratch
> > +$XFS_IO_PROG -c '' $fsv_file 2>&1 | _filter_scratch
> >  echo "* bash >>, should be O_APPEND"
> >  bash -c "echo >> $fsv_file" |& _filter_scratch
> >  echo "* bash >, should be O_WRONLY|O_CREAT|O_TRUNC"
> > 
> > xfs_io gets into interactive mode when invoked without a "-c cmd" string.
> > 
> > However, I am not able recreate the scenario once again without the above
> > changes applied. I am not sure about what changed. 
> > 
> 
> The test is opening a verity file for read+write access, which should fail.  But
> it's incorrectly succeeding, hence the test is right to not pass.  Did you add
> the missing call to ext4_set_inode_flags() in ext4_set_verity() as I
> suggested?
> 
> (But I'll make the suggested change to the test too, so it fails cleanly in this
> case rather than hangs reading from stdin.)

Yes, I did make the suggested changes. But the test would some times hang
indefinitely because of xfs_io waiting on input from stdin.

With the new changes made to ext4_set_verity(), I see that the fsck fails
consistency check. But the failure is seen even without my patches applied. I
have planned to debug the failure after I post the next version of the
patchset.
Eric Biggers Nov. 30, 2018, 5:44 p.m. UTC | #10
On Fri, Nov 30, 2018 at 10:57:58AM +0530, Chandan Rajendra wrote:
> On Friday, November 30, 2018 12:35:13 AM IST Eric Biggers wrote:
> > Hi Chandan,
> > 
> > On Thu, Nov 29, 2018 at 04:08:31PM +0530, Chandan Rajendra wrote:
> > > On Monday, November 26, 2018 11:04:35 PM IST Theodore Y. Ts'o wrote:
> > > > On Sun, Nov 25, 2018 at 11:00:38PM -0500, Theodore Y. Ts'o wrote:
> > > > > 
> > > > > It might be that the simplest way to solve things is to merge the f2fs
> > > > > dev branch up to 79c66e75720c.  This will have the net effect of
> > > > > including the five patches listed above onto the fscrypt git tree.  So
> > > > > long you don't plan to rebase or otherwise change these five patches,
> > > > > it should avoid any merge conflicts.
> > > > 
> > > > I've set up a git branch which has the f2fs dev branch, 4.20-rc4, the
> > > > fsverity patches, and part of Chandan's patch series here:
> > > > 
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/tytso/fscrypt.git test-working
> > > > 
> > > > There is a minor conflict when I did a trial merge with f2fs.git's dev
> > > > branch, but it's pretty obvious to how to resolve it.
> > > > 
> > > > Jaegeuk, Eric, Chandan, please take a look and let me know what you
> > > > think.
> > > 
> > > Ted,
> > > 
> > > I have addressed the review comments provided by Eric. Hence three out of
> > > the four patches in the test-working branch have new changes. I also got
> > > UBIFS to use CONFIG_FS_ENCRYPTION instead of the per-filesystem config
> > > symbol.
> > > 
> > > I am currently executing fstests to verify the changes.
> > > 
> > > 
> > > Eric,
> > > 
> > > When executing generic/900, I noticed that sometimes xfs_io would get stuck
> > > for an indefinite period. /proc/<pid of xfs_io>/stack showed that the task was
> > > stuck in tty_read() inside the kernel. The following change fixed it,
> > > 
> > > diff --git a/tests/generic/900 b/tests/generic/900
> > > index 290889ce..0831eed4 100755
> > > --- a/tests/generic/900
> > > +++ b/tests/generic/900
> > > @@ -83,7 +83,7 @@ _fsv_create_enable_file $fsv_file >> $seqres.full
> > >  echo "* reading"
> > >  $XFS_IO_PROG -r $fsv_file -c ''
> > >  echo "* xfs_io writing, should be O_RDWR"
> > > -$XFS_IO_PROG $fsv_file |& _filter_scratch
> > > +$XFS_IO_PROG -c '' $fsv_file 2>&1 | _filter_scratch
> > >  echo "* bash >>, should be O_APPEND"
> > >  bash -c "echo >> $fsv_file" |& _filter_scratch
> > >  echo "* bash >, should be O_WRONLY|O_CREAT|O_TRUNC"
> > > 
> > > xfs_io gets into interactive mode when invoked without a "-c cmd" string.
> > > 
> > > However, I am not able recreate the scenario once again without the above
> > > changes applied. I am not sure about what changed. 
> > > 
> > 
> > The test is opening a verity file for read+write access, which should fail.  But
> > it's incorrectly succeeding, hence the test is right to not pass.  Did you add
> > the missing call to ext4_set_inode_flags() in ext4_set_verity() as I
> > suggested?
> > 
> > (But I'll make the suggested change to the test too, so it fails cleanly in this
> > case rather than hangs reading from stdin.)
> 
> Yes, I did make the suggested changes. But the test would some times hang
> indefinitely because of xfs_io waiting on input from stdin.
> 
> With the new changes made to ext4_set_verity(), I see that the fsck fails
> consistency check. But the failure is seen even without my patches applied. I
> have planned to debug the failure after I post the next version of the
> patchset.
> 

You're testing with e2fsprogs "1.44.4-2" or later, right?  Note that the
original "v1.44.4" had some bugs with verity support, which were fixed shortly
after.  Specifically, e2fsprogs needs to be commit 3baafde6a8ae7 or later.

- Eric

Patch
diff mbox series

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 09d9fc1676a7..844ec573263e 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1491,7 +1491,7 @@  int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	}
 
 	if (size) {
-		if (f2fs_encrypted_inode(inode))
+		if (IS_ENCRYPTED(inode))
 			flags |= FIEMAP_EXTENT_DATA_ENCRYPTED;
 
 		ret = fiemap_fill_next_extent(fieinfo, logical,
@@ -1764,7 +1764,7 @@  static inline bool check_inplace_update_policy(struct inode *inode,
 	if (policy & (0x1 << F2FS_IPU_ASYNC) &&
 			fio && fio->op == REQ_OP_WRITE &&
 			!(fio->op_flags & REQ_SYNC) &&
-			!f2fs_encrypted_inode(inode))
+			!IS_ENCRYPTED(inode))
 		return true;
 
 	/* this is only set during fdatasync */
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index bacc667950b6..cf9e2564388d 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -385,7 +385,7 @@  struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir,
 		if (err)
 			goto put_error;
 
-		if ((f2fs_encrypted_inode(dir) || dummy_encrypt) &&
+		if ((IS_ENCRYPTED(dir) || dummy_encrypt) &&
 					f2fs_may_encrypt(inode)) {
 			err = fscrypt_inherit_context(dir, inode, page, false);
 			if (err)
@@ -399,7 +399,7 @@  struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir,
 
 	if (new_name) {
 		init_dent_inode(new_name, page);
-		if (f2fs_encrypted_inode(dir))
+		if (IS_ENCRYPTED(dir))
 			file_set_enc_name(inode);
 	}
 
@@ -808,7 +808,7 @@  int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
 		de_name.name = d->filename[bit_pos];
 		de_name.len = le16_to_cpu(de->name_len);
 
-		if (f2fs_encrypted_inode(d->inode)) {
+		if (IS_ENCRYPTED(d->inode)) {
 			int save_len = fstr->len;
 
 			err = fscrypt_fname_disk_to_usr(d->inode,
@@ -852,7 +852,7 @@  static int f2fs_readdir(struct file *file, struct dir_context *ctx)
 	struct fscrypt_str fstr = FSTR_INIT(NULL, 0);
 	int err = 0;
 
-	if (f2fs_encrypted_inode(inode)) {
+	if (IS_ENCRYPTED(inode)) {
 		err = fscrypt_get_encryption_info(inode);
 		if (err && err != -ENOKEY)
 			goto out;
@@ -914,7 +914,7 @@  static int f2fs_readdir(struct file *file, struct dir_context *ctx)
 
 static int f2fs_dir_open(struct inode *inode, struct file *filp)
 {
-	if (f2fs_encrypted_inode(inode))
+	if (IS_ENCRYPTED(inode))
 		return fscrypt_get_encryption_info(inode) ? -EACCES : 0;
 	return 0;
 }
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 87794b2a45ff..6c7ad15000b9 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -585,7 +585,7 @@  static int truncate_partial_data_page(struct inode *inode, u64 from,
 	zero_user(page, offset, PAGE_SIZE - offset);
 
 	/* An encrypted inode should have a key and truncate the last page. */
-	f2fs_bug_on(F2FS_I_SB(inode), cache_only && f2fs_encrypted_inode(inode));
+	f2fs_bug_on(F2FS_I_SB(inode), cache_only && IS_ENCRYPTED(inode));
 	if (!cache_only)
 		set_page_dirty(page);
 	f2fs_put_page(page, 1);
@@ -730,7 +730,7 @@  int f2fs_getattr(const struct path *path, struct kstat *stat,
 		stat->attributes |= STATX_ATTR_APPEND;
 	if (flags & F2FS_COMPR_FL)
 		stat->attributes |= STATX_ATTR_COMPRESSED;
-	if (f2fs_encrypted_inode(inode))
+	if (IS_ENCRYPTED(inode))
 		stat->attributes |= STATX_ATTR_ENCRYPTED;
 	if (flags & F2FS_IMMUTABLE_FL)
 		stat->attributes |= STATX_ATTR_IMMUTABLE;
@@ -1587,7 +1587,7 @@  static long f2fs_fallocate(struct file *file, int mode,
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 
-	if (f2fs_encrypted_inode(inode) &&
+	if (IS_ENCRYPTED(inode) &&
 		(mode & (FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_INSERT_RANGE)))
 		return -EOPNOTSUPP;
 
@@ -1671,7 +1671,7 @@  static int f2fs_ioc_getflags(struct file *filp, unsigned long arg)
 	struct f2fs_inode_info *fi = F2FS_I(inode);
 	unsigned int flags = fi->i_flags;
 
-	if (f2fs_encrypted_inode(inode))
+	if (IS_ENCRYPTED(inode))
 		flags |= F2FS_ENCRYPT_FL;
 	if (f2fs_has_inline_data(inode) || f2fs_has_inline_dentry(inode))
 		flags |= F2FS_INLINE_DATA_FL;
@@ -2430,7 +2430,7 @@  static int f2fs_move_file_range(struct file *file_in, loff_t pos_in,
 	if (!S_ISREG(src->i_mode) || !S_ISREG(dst->i_mode))
 		return -EINVAL;
 
-	if (f2fs_encrypted_inode(src) || f2fs_encrypted_inode(dst))
+	if (IS_ENCRYPTED(src) || IS_ENCRYPTED(dst))
 		return -EOPNOTSUPP;
 
 	if (src == dst) {
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 99299ede7429..6ae37e0cf6e3 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -75,7 +75,7 @@  static struct inode *f2fs_new_inode(struct inode *dir, umode_t mode)
 	set_inode_flag(inode, FI_NEW_INODE);
 
 	/* If the directory encrypted, then we should encrypt the inode. */
-	if ((f2fs_encrypted_inode(dir) || DUMMY_ENCRYPTION_ENABLED(sbi)) &&
+	if ((IS_ENCRYPTED(dir) || DUMMY_ENCRYPTION_ENABLED(sbi)) &&
 				f2fs_may_encrypt(inode))
 		f2fs_set_encrypted_inode(inode);
 
@@ -476,7 +476,7 @@  static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 		if (err)
 			goto out_iput;
 	}
-	if (f2fs_encrypted_inode(dir) &&
+	if (IS_ENCRYPTED(dir) &&
 	    (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) &&
 	    !fscrypt_has_permitted_context(dir, inode)) {
 		f2fs_msg(inode->i_sb, KERN_WARNING,
@@ -803,7 +803,7 @@  static int f2fs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 	if (unlikely(f2fs_cp_error(sbi)))
 		return -EIO;
 
-	if (f2fs_encrypted_inode(dir) || DUMMY_ENCRYPTION_ENABLED(sbi)) {
+	if (IS_ENCRYPTED(dir) || DUMMY_ENCRYPTION_ENABLED(sbi)) {
 		int err = fscrypt_get_encryption_info(dir);
 		if (err)
 			return err;