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    [W:0.030 / U:37.480 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site