Re: [PATCH 2/3] eCryptfs: Check inode changes in setattr

[Tyler Hicks]

On 2012-01-21 15:57:58, Li Wang wrote:
> Hi Tyler,
>   Please consider the following two things,

Hello - Thanks for the review!

> 1. While invoking inode_newsize_ok/inode_change_ok, it just make sure the new file size seen from
> eCryptfs will not exceed the whatever kinds of file size limit, what about the new size does not
> exceed the limit, plus ecryptfs_lower_header_size will. Therefore the safest way is to check the
> new size seen from lower file system, which is ecryptfs_lower_header_size bigger.  
> 2. The senmatics of sb->s_maxbytes, is the maximum file size allowed by the file system 
> repsented by sb. For eCryptfs, it should be lower_sb->s_maxbytes - ecryptfs_lower_header_size, 
> rather than equal to lower_sb->s_maxbytes. However, the ecryptfs_lower_header_size is different
> file by file, not a file system wide constant. It is, kind of nasty and we cannot trust it. 
> Combined with the reason 1, we prefer to execute an extra new size check on lower inode
> after inode_change_ok on ecryptfs inode. For ecryptfs_truncate, directly perform new size check
> on lower inode. 
>   Please check the patch below.

I generally agree with this description, but have some comments below
regarding implementation details.

> 
> Cheers,
> Li Wang
> 
> Signed-off-by: Li Wang <liwang@xxxxxxxxxxx>
>                Yunchuan Wen <wenyunchuan@xxxxxxxxxxxxxx>
> 
> ---
> 
> diff -prNu a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> --- a/fs/ecryptfs/inode.c	2012-01-05 07:55:44.000000000 +0800
> +++ b/fs/ecryptfs/inode.c	2012-01-21 15:55:21.000000000 +0800
> @@ -841,18 +841,6 @@ static int truncate_upper(struct dentry 
>  		size_t num_zeros = (PAGE_CACHE_SIZE
>  				    - (ia->ia_size & ~PAGE_CACHE_MASK));
>  
> -
> -		/*
> -		 * XXX(truncate) this should really happen at the begginning
> -		 * of ->setattr.  But the code is too messy to that as part
> -		 * of a larger patch.  ecryptfs is also totally missing out
> -		 * on the inode_change_ok check at the beginning of
> -		 * ->setattr while would include this.
> -		 */
> -		rc = inode_newsize_ok(inode, ia->ia_size);
> -		if (rc)
> -			goto out;
> -
>  		if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
>  			truncate_setsize(inode, ia->ia_size);
>  			lower_ia->ia_size = ia->ia_size;
> @@ -916,8 +904,14 @@ int ecryptfs_truncate(struct dentry *den
>  {
>  	struct iattr ia = { .ia_valid = ATTR_SIZE, .ia_size = new_length };
>  	struct iattr lower_ia = { .ia_valid = 0 };
> +	struct ecryptfs_crypt_stat *crypt_stat;
>  	int rc;
> -
> +	
> +	crypt_stat = &ecryptfs_inode_to_private(dentry->d_inode)->crypt_stat;
> +	rc = inode_newsize_ok(ecryptfs_inode_to_lower(dentry->d_inode), new_length + ecryptfs_lower_header_size(crypt_stat));

A few issues here..

1) This is not taking into account the padding added to the last
encryption extent. It can range between 0 and
(ECRYPTFS_DEFAULT_EXTENT_SIZE - 1) bytes.

2) To call inode_newsize_ok() on the lower inode, we'd need to be
holding its i_mutex.

3) I'm not comfortable calling inode_newsize_ok() directly on the lower
inode. I suppose that some filesystems may need a chance to get i_size up to
date (that's what eCryptfs is potentially doing at the start of
->setattr() when reading the metadata). Since
inode_change_ok()/inode_newsize_ok() is not called by the VFS, that
implies to me that it is not safe for us to just blindly call into with
another filesystem's inodes.

So, I say that we do something along these lines:

inode_newsize_ok(ecryptfs_inode, upper_size_to_lower_size(ia->ia_size));

It isn't ideal, but I'd rather not open code our own version of
inode_newsize_ok().

> +	if (rc)
> +		return rc;
> +	
>  	rc = truncate_upper(dentry, &ia, &lower_ia);
>  	if (!rc && lower_ia.ia_valid & ATTR_SIZE) {
>  		struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
> @@ -997,6 +991,15 @@ static int ecryptfs_setattr(struct dentr
>  		}
>  	}
>  	mutex_unlock(&crypt_stat->cs_mutex);
> +	
> +	rc = inode_change_ok(inode, ia);
> +	if (rc)
> +		goto out;
> +	if (ia->ia_valid & ATTR_SIZE)
> +		rc = inode_newsize_ok(lower_inode, ia->ia_size + ecryptfs_lower_header_size(crypt_stat));

I think that all of the points above apply here, as well.

I'll try to get a patch out in response to this email.

Tyler

> +	if (rc)
> +		goto out;
> +	
>  	if (S_ISREG(inode->i_mode)) {
>  		rc = filemap_write_and_wait(inode->i_mapping);
>  		if (rc)
> 
> 
> 
> 
> 
> ---------- Origin message ----------
> >Fromï"Tyler Hicks" <tyhicks@xxxxxxxxxxxxx>
> >Toïecryptfs@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx
> >Subjectï[PATCH 2/3] eCryptfs: Check inode changes in setattr
> >Dateï2012-01-21 06:35:06
> 
> Most filesystems call inode_change_ok() very early in ->setattr(), but
> eCryptfs didn't call it at all. It allowed the lower filesystem to make
> the call in its ->setattr() function. Then, eCryptfs would copy the
> appropriate inode attributes from the lower inode to the eCryptfs inode.
> 
> This patch changes that and actually calls inode_change_ok() on the
> eCryptfs inode, fairly early in ecryptfs_setattr(). Ideally, the call
> would happen earlier in ecryptfs_setattr(), but there is some possible
> inode initialization that must happen first.
> 
> Since the call was already being made on the lower inode, the change in
> functionality should be minimal, except for the case of a file extending
> truncate call. In that case, inode_newsize_ok() was never being
> called on the eCryptfs inode. Rather than inode_newsize_ok() catching
> errors early on, eCryptfs would encrypt zeroed pages and write them to
> the lower filesystem until the lower filesystem's write path caught the
> error in generic_write_checks().
> 
> In summary this change prevents eCryptfs truncate operations (and the
> resulting page encryptions), which would exceed the lower filesystem

Attachment: signature.asc
Description: Digital signature