lkml.org 
[lkml]   [2012]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/3] eCryptfs: Check inode changes in setattr
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@nudt.edu.cn>
> Yunchuan Wen <wenyunchuan@kylinos.com.cn>
>
> ---
>
> 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@canonical.com>
> >To:ecryptfs@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
> >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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-01-24 07:35    [from the cache]
©2003-2011 Jasper Spaans