* eCryptfs: Truncate path improvements
@ 2012-01-20 22:35 Tyler Hicks
2012-01-20 22:35 ` [PATCH 1/3] eCryptfs: Make truncate path killable Tyler Hicks
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Tyler Hicks @ 2012-01-20 22:35 UTC (permalink / raw)
To: ecryptfs, linux-kernel, linux-fsdevel
Cc: Linus Torvalds, john.johansen, dustin.kirkland, Cong Wang, Li Wang
As Linus pointed out[1], eCryptfs needs more checks around its truncate path.
The first patch in this series makes it possible for an eCryptfs truncate
operation to be gracefully interrupted by a fatal signal. The second adds
checks for eCryptfs inode changes in setattr. The third is simply removal of
an old, unused function.
Tyler
[1] https://lkml.org/lkml/2012/1/18/356
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] eCryptfs: Make truncate path killable 2012-01-20 22:35 eCryptfs: Truncate path improvements Tyler Hicks @ 2012-01-20 22:35 ` Tyler Hicks 2012-01-20 22:35 ` [PATCH 2/3] eCryptfs: Check inode changes in setattr Tyler Hicks ` (3 subsequent siblings) 4 siblings, 0 replies; 11+ messages in thread From: Tyler Hicks @ 2012-01-20 22:35 UTC (permalink / raw) To: ecryptfs, linux-kernel, linux-fsdevel Cc: Linus Torvalds, john.johansen, dustin.kirkland, Cong Wang, Li Wang ecryptfs_write() handles the truncation of eCryptfs inodes. It grabs a page, zeroes out the appropriate portions, and then encrypts the page before writing it to the lower filesystem. It was unkillable and due to the lack of sparse file support could result in tying up a large portion of system resources, while encrypting pages of zeros, with no way for the truncate operation to be stopped from userspace. This patch adds the ability for ecryptfs_write() to detect a pending fatal signal and return as gracefully as possible. The intent is to leave the lower file in a useable state, while still allowing a user to break out of the encryption loop. If a pending fatal signal is detected, the eCryptfs inode size is updated to reflect the modified inode size and then -EINTR is returned. Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Cc: <stable@vger.kernel.org> --- fs/ecryptfs/read_write.c | 19 ++++++++++++++----- 1 files changed, 14 insertions(+), 5 deletions(-) diff --git a/fs/ecryptfs/read_write.c b/fs/ecryptfs/read_write.c index ec3d936..608c1c3 100644 --- a/fs/ecryptfs/read_write.c +++ b/fs/ecryptfs/read_write.c @@ -132,6 +132,11 @@ int ecryptfs_write(struct inode *ecryptfs_inode, char *data, loff_t offset, size_t num_bytes = (PAGE_CACHE_SIZE - start_offset_in_page); loff_t total_remaining_bytes = ((offset + size) - pos); + if (fatal_signal_pending(current)) { + rc = -EINTR; + break; + } + if (num_bytes > total_remaining_bytes) num_bytes = total_remaining_bytes; if (pos < offset) { @@ -193,15 +198,19 @@ int ecryptfs_write(struct inode *ecryptfs_inode, char *data, loff_t offset, } pos += num_bytes; } - if ((offset + size) > ecryptfs_file_size) { - i_size_write(ecryptfs_inode, (offset + size)); + if (pos > ecryptfs_file_size) { + i_size_write(ecryptfs_inode, pos); if (crypt_stat->flags & ECRYPTFS_ENCRYPTED) { - rc = ecryptfs_write_inode_size_to_metadata( + int rc2; + + rc2 = ecryptfs_write_inode_size_to_metadata( ecryptfs_inode); - if (rc) { + if (rc2) { printk(KERN_ERR "Problem with " "ecryptfs_write_inode_size_to_metadata; " - "rc = [%d]\n", rc); + "rc = [%d]\n", rc2); + if (!rc) + rc = rc2; goto out; } } -- 1.7.8.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] eCryptfs: Check inode changes in setattr 2012-01-20 22:35 eCryptfs: Truncate path improvements Tyler Hicks 2012-01-20 22:35 ` [PATCH 1/3] eCryptfs: Make truncate path killable Tyler Hicks @ 2012-01-20 22:35 ` Tyler Hicks 2012-01-20 22:35 ` [PATCH 3/3] eCryptfs: Remove unused ecryptfs_read() Tyler Hicks ` (2 subsequent siblings) 4 siblings, 0 replies; 11+ messages in thread From: Tyler Hicks @ 2012-01-20 22:35 UTC (permalink / raw) To: ecryptfs, linux-kernel, linux-fsdevel Cc: Linus Torvalds, john.johansen, dustin.kirkland, Cong Wang, Li Wang 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 limits or FSIZE rlimits, from ever starting. Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Cc: <stable@vger.kernel.org> --- fs/ecryptfs/inode.c | 21 +++++++++------------ 1 files changed, 9 insertions(+), 12 deletions(-) diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c index 19a8ca4..e025697 100644 --- a/fs/ecryptfs/inode.c +++ b/fs/ecryptfs/inode.c @@ -822,18 +822,6 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia, 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; @@ -899,6 +887,10 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) struct iattr lower_ia = { .ia_valid = 0 }; int rc; + rc = inode_newsize_ok(dentry->d_inode, new_length); + 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); @@ -978,6 +970,11 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia) } } mutex_unlock(&crypt_stat->cs_mutex); + + rc = inode_change_ok(inode, ia); + if (rc) + goto out; + if (S_ISREG(inode->i_mode)) { rc = filemap_write_and_wait(inode->i_mapping); if (rc) -- 1.7.8.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] eCryptfs: Remove unused ecryptfs_read() 2012-01-20 22:35 eCryptfs: Truncate path improvements Tyler Hicks 2012-01-20 22:35 ` [PATCH 1/3] eCryptfs: Make truncate path killable Tyler Hicks 2012-01-20 22:35 ` [PATCH 2/3] eCryptfs: Check inode changes in setattr Tyler Hicks @ 2012-01-20 22:35 ` Tyler Hicks [not found] ` <527098189.19032@eyou.net> 2012-01-23 16:49 ` eCryptfs: Truncate path improvements Linus Torvalds 4 siblings, 0 replies; 11+ messages in thread From: Tyler Hicks @ 2012-01-20 22:35 UTC (permalink / raw) To: ecryptfs, linux-kernel, linux-fsdevel Cc: Linus Torvalds, john.johansen, dustin.kirkland, Cong Wang, Li Wang ecryptfs_read() has been ifdef'ed out for years now and it was apparently unused before then. It is time to get rid of it for good. Signed-off-by: Tyler Hicks <tyhicks@canonical.com> --- fs/ecryptfs/read_write.c | 73 ---------------------------------------------- 1 files changed, 0 insertions(+), 73 deletions(-) diff --git a/fs/ecryptfs/read_write.c b/fs/ecryptfs/read_write.c index 608c1c3..5c0106f 100644 --- a/fs/ecryptfs/read_write.c +++ b/fs/ecryptfs/read_write.c @@ -282,76 +282,3 @@ int ecryptfs_read_lower_page_segment(struct page *page_for_ecryptfs, flush_dcache_page(page_for_ecryptfs); return rc; } - -#if 0 -/** - * ecryptfs_read - * @data: The virtual address into which to write the data read (and - * possibly decrypted) from the lower file - * @offset: The offset in the decrypted view of the file from which to - * read into @data - * @size: The number of bytes to read into @data - * @ecryptfs_file: The eCryptfs file from which to read - * - * Read an arbitrary amount of data from an arbitrary location in the - * eCryptfs page cache. This is done on an extent-by-extent basis; - * individual extents are decrypted and read from the lower page - * cache (via VFS reads). This function takes care of all the - * address translation to locations in the lower filesystem. - * - * Returns zero on success; non-zero otherwise - */ -int ecryptfs_read(char *data, loff_t offset, size_t size, - struct file *ecryptfs_file) -{ - struct inode *ecryptfs_inode = ecryptfs_file->f_dentry->d_inode; - struct page *ecryptfs_page; - char *ecryptfs_page_virt; - loff_t ecryptfs_file_size = i_size_read(ecryptfs_inode); - loff_t data_offset = 0; - loff_t pos; - int rc = 0; - - if ((offset + size) > ecryptfs_file_size) { - rc = -EINVAL; - printk(KERN_ERR "%s: Attempt to read data past the end of the " - "file; offset = [%lld]; size = [%td]; " - "ecryptfs_file_size = [%lld]\n", - __func__, offset, size, ecryptfs_file_size); - goto out; - } - pos = offset; - while (pos < (offset + size)) { - pgoff_t ecryptfs_page_idx = (pos >> PAGE_CACHE_SHIFT); - size_t start_offset_in_page = (pos & ~PAGE_CACHE_MASK); - size_t num_bytes = (PAGE_CACHE_SIZE - start_offset_in_page); - size_t total_remaining_bytes = ((offset + size) - pos); - - if (num_bytes > total_remaining_bytes) - num_bytes = total_remaining_bytes; - ecryptfs_page = ecryptfs_get_locked_page(ecryptfs_inode, - ecryptfs_page_idx); - if (IS_ERR(ecryptfs_page)) { - rc = PTR_ERR(ecryptfs_page); - printk(KERN_ERR "%s: Error getting page at " - "index [%ld] from eCryptfs inode " - "mapping; rc = [%d]\n", __func__, - ecryptfs_page_idx, rc); - goto out; - } - ecryptfs_page_virt = kmap_atomic(ecryptfs_page, KM_USER0); - memcpy((data + data_offset), - ((char *)ecryptfs_page_virt + start_offset_in_page), - num_bytes); - kunmap_atomic(ecryptfs_page_virt, KM_USER0); - flush_dcache_page(ecryptfs_page); - SetPageUptodate(ecryptfs_page); - unlock_page(ecryptfs_page); - page_cache_release(ecryptfs_page); - pos += num_bytes; - data_offset += num_bytes; - } -out: - return rc; -} -#endif /* 0 */ -- 1.7.8.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
[parent not found: <527098189.19032@eyou.net>]
* Re:[PATCH 2/3] eCryptfs: Check inode changes in setattr [not found] ` <527098189.19032@eyou.net> @ 2012-01-21 7:57 ` Li Wang 2012-01-24 6:32 ` [PATCH " Tyler Hicks 0 siblings, 1 reply; 11+ messages in thread From: Li Wang @ 2012-01-21 7:57 UTC (permalink / raw) To: Linus Torvalds, john.johansen, dustin.kirkland, Cong Wang, ecryptfs, linux-kernel, linux-fsdevel, Tyler Hicks [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="GBK", Size: 4591 bytes --] Hi Tyler, Please consider the following two things, 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. 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)); + 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)); + 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 ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] eCryptfs: Check inode changes in setattr 2012-01-21 7:57 ` Re:[PATCH 2/3] eCryptfs: Check inode changes in setattr Li Wang @ 2012-01-24 6:32 ` Tyler Hicks 2012-01-24 7:37 ` [PATCH 2/3 v2] " Tyler Hicks [not found] ` <527389872.29922@eyou.net> 0 siblings, 2 replies; 11+ messages in thread From: Tyler Hicks @ 2012-01-24 6:32 UTC (permalink / raw) To: Li Wang Cc: Linus Torvalds, john.johansen, dustin.kirkland, Cong Wang, ecryptfs, linux-kernel, linux-fsdevel [-- Attachment #1: Type: text/plain, Size: 6004 bytes --] 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3 v2] eCryptfs: Check inode changes in setattr 2012-01-24 6:32 ` [PATCH " Tyler Hicks @ 2012-01-24 7:37 ` Tyler Hicks [not found] ` <527389872.29922@eyou.net> 1 sibling, 0 replies; 11+ messages in thread From: Tyler Hicks @ 2012-01-24 7:37 UTC (permalink / raw) To: ecryptfs, linux-kernel, linux-fsdevel Cc: Linus Torvalds, john.johansen, dustin.kirkland, Cong Wang, Li Wang 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 are some possible inode initialization steps 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 maximum file size 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(). This patch introduces a new function, called ecryptfs_inode_newsize_ok(), which checks if the new lower file size is within the appropriate limits when the truncate operation will be growing the lower file. In summary this change prevents eCryptfs truncate operations (and the resulting page encryptions), which would exceed the lower filesystem limits or FSIZE rlimits, from ever starting. Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Cc: <stable@vger.kernel.org> --- Linus, this is still in the out-for-comments stage. I'll be preparing a pull request soon. Changes from v1: - Added ecryptfs_inode_newsize_ok() to help with detecting when inode_newsize_ok() needs to be called and to pass the appropriate offset parameter to that function - Minor touchups to the commit message fs/ecryptfs/inode.c | 48 ++++++++++++++++++++++++++++++++++++------------ 1 files changed, 36 insertions(+), 12 deletions(-) diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c index 19a8ca4..19892d7 100644 --- a/fs/ecryptfs/inode.c +++ b/fs/ecryptfs/inode.c @@ -822,18 +822,6 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia, 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; @@ -883,6 +871,28 @@ out: return rc; } +static int ecryptfs_inode_newsize_ok(struct inode *inode, loff_t offset) +{ + struct ecryptfs_crypt_stat *crypt_stat; + loff_t lower_oldsize, lower_newsize; + + crypt_stat = &ecryptfs_inode_to_private(inode)->crypt_stat; + lower_oldsize = upper_size_to_lower_size(crypt_stat, + i_size_read(inode)); + lower_newsize = upper_size_to_lower_size(crypt_stat, offset); + if (lower_newsize > lower_oldsize) { + /* + * The eCryptfs inode and the new *lower* size are mixed here + * because we may not have the lower i_mutex held and/or it may + * not be appropriate to call inode_newsize_ok() with inodes + * from other filesystems. + */ + return inode_newsize_ok(inode, lower_newsize); + } + + return 0; +} + /** * ecryptfs_truncate * @dentry: The ecryptfs layer dentry @@ -899,6 +909,10 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) struct iattr lower_ia = { .ia_valid = 0 }; int rc; + rc = ecryptfs_inode_newsize_ok(dentry->d_inode, new_length); + 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); @@ -978,6 +992,16 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia) } } mutex_unlock(&crypt_stat->cs_mutex); + + rc = inode_change_ok(inode, ia); + if (rc) + goto out; + if (ia->ia_valid & ATTR_SIZE) { + rc = ecryptfs_inode_newsize_ok(inode, ia->ia_size); + if (rc) + goto out; + } + if (S_ISREG(inode->i_mode)) { rc = filemap_write_and_wait(inode->i_mapping); if (rc) -- 1.7.8.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
[parent not found: <527389872.29922@eyou.net>]
* Re:[PATCH 2/3 v2] eCryptfs: Check inode changes in setattr [not found] ` <527389872.29922@eyou.net> @ 2012-01-24 14:14 ` Li Wang 2012-01-25 7:40 ` [PATCH] eCryptfs: move misleading function comments Li Wang 1 sibling, 0 replies; 11+ messages in thread From: Li Wang @ 2012-01-24 14:14 UTC (permalink / raw) To: Linus Torvalds, john.johansen, dustin.kirkland, Cong Wang, ecryptfs, linux-kernel, linux-fsdevel, Tyler Hicks [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="GBK", Size: 1947 bytes --] Hi Tyler, We think that is good, except it is not very elegant to invoke inode_newsize_ok with the ecryptfs (upper) inode and lower size, nevertheless, we donot have other better choices and it is wrapped in ecryptfs_inode_newsize_ok... In a word, we are with you. Cheers, Li Wang ---------- 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 v2] eCryptfs: Check inode changes in setattr >Date£º2012-01-24 15:37:32 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 are some possible inode initialization steps 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 maximum file size 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(). This patch introduces a new function, called ecryptfs_inode_newsize_ok(), which checks if the new lower file size is within the appropriate limits when the truncate operation will be growing the lower file. ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥ ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] eCryptfs: move misleading function comments [not found] ` <527389872.29922@eyou.net> 2012-01-24 14:14 ` Li Wang @ 2012-01-25 7:40 ` Li Wang 1 sibling, 0 replies; 11+ messages in thread From: Li Wang @ 2012-01-25 7:40 UTC (permalink / raw) To: Linus Torvalds, john.johansen, dustin.kirkland, Cong Wang, ecryptfs, linux-kernel, linux-fsdevel, Tyler Hicks [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="GBK", Size: 1528 bytes --] Hi, The data encryption was moved from ecryptfs_write_end into ecryptfs_writepage, this patch moves the corresponding function comments to be consistent with the modification. Signed-off-by: Li Wang <liwang@nudt.edu.cn> --- fs/ecryptfs/mmap.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c index 6a44148..10ec695 100644 --- a/fs/ecryptfs/mmap.c +++ b/fs/ecryptfs/mmap.c @@ -57,6 +57,10 @@ struct page *ecryptfs_get_locked_page(struct inode *inode, loff_t index) * @page: Page that is locked before this call is made * * Returns zero on success; non-zero otherwise + * + * This is where we encrypt the data and pass the encrypted data to + * the lower filesystem. In OpenPGP-compatible mode, we operate on + * entire underlying packets. */ static int ecryptfs_writepage(struct page *page, struct writeback_control *wbc) { @@ -481,10 +485,6 @@ int ecryptfs_write_inode_size_to_metadata(struct inode *ecryptfs_inode) * @copied: The amount of data copied * @page: The eCryptfs page * @fsdata: The fsdata (unused) - * - * This is where we encrypt the data and pass the encrypted data to - * the lower filesystem. In OpenPGP-compatible mode, we operate on - * entire underlying packets. */ static int ecryptfs_write_end(struct file *file, struct address_space *mapping, ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥ ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: eCryptfs: Truncate path improvements 2012-01-20 22:35 eCryptfs: Truncate path improvements Tyler Hicks ` (3 preceding siblings ...) [not found] ` <527098189.19032@eyou.net> @ 2012-01-23 16:49 ` Linus Torvalds 2012-01-23 16:57 ` Tyler Hicks 4 siblings, 1 reply; 11+ messages in thread From: Linus Torvalds @ 2012-01-23 16:49 UTC (permalink / raw) To: Tyler Hicks Cc: ecryptfs, linux-kernel, linux-fsdevel, john.johansen, dustin.kirkland, Cong Wang, Li Wang On Fri, Jan 20, 2012 at 2:35 PM, Tyler Hicks <tyhicks@canonical.com> wrote: > As Linus pointed out[1], eCryptfs needs more checks around its truncate path. > The first patch in this series makes it possible for an eCryptfs truncate > operation to be gracefully interrupted by a fatal signal. The second adds > checks for eCryptfs inode changes in setattr. The third is simply removal of > an old, unused function. Tyler, should I take these patches directly? Or are these just for review and I should wait for a pull request? Linus ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: eCryptfs: Truncate path improvements 2012-01-23 16:49 ` eCryptfs: Truncate path improvements Linus Torvalds @ 2012-01-23 16:57 ` Tyler Hicks 0 siblings, 0 replies; 11+ messages in thread From: Tyler Hicks @ 2012-01-23 16:57 UTC (permalink / raw) To: Linus Torvalds Cc: ecryptfs, linux-kernel, linux-fsdevel, john.johansen, dustin.kirkland, Cong Wang, Li Wang [-- Attachment #1: Type: text/plain, Size: 714 bytes --] On 2012-01-23 08:49:40, Linus Torvalds wrote: > On Fri, Jan 20, 2012 at 2:35 PM, Tyler Hicks <tyhicks@canonical.com> wrote: > > As Linus pointed out[1], eCryptfs needs more checks around its truncate path. > > The first patch in this series makes it possible for an eCryptfs truncate > > operation to be gracefully interrupted by a fatal signal. The second adds > > checks for eCryptfs inode changes in setattr. The third is simply removal of > > an old, unused function. > > Tyler, should I take these patches directly? Or are these just for > review and I should wait for a pull request? Just for review, as I'll send a pull request. I'll try to be more clear about that in the future. Tyler [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-01-25 7:54 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-20 22:35 eCryptfs: Truncate path improvements Tyler Hicks
2012-01-20 22:35 ` [PATCH 1/3] eCryptfs: Make truncate path killable Tyler Hicks
2012-01-20 22:35 ` [PATCH 2/3] eCryptfs: Check inode changes in setattr Tyler Hicks
2012-01-20 22:35 ` [PATCH 3/3] eCryptfs: Remove unused ecryptfs_read() Tyler Hicks
[not found] ` <527098189.19032@eyou.net>
2012-01-21 7:57 ` Re:[PATCH 2/3] eCryptfs: Check inode changes in setattr Li Wang
2012-01-24 6:32 ` [PATCH " Tyler Hicks
2012-01-24 7:37 ` [PATCH 2/3 v2] " Tyler Hicks
[not found] ` <527389872.29922@eyou.net>
2012-01-24 14:14 ` Li Wang
2012-01-25 7:40 ` [PATCH] eCryptfs: move misleading function comments Li Wang
2012-01-23 16:49 ` eCryptfs: Truncate path improvements Linus Torvalds
2012-01-23 16:57 ` Tyler Hicks
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).