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