| Message ID | 1329403106-17875-2-git-send-email-colin.king@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 16.02.2012 15:38, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > When truncating inodes in the lower filesystem, eCryptfs directly > invoked vmtruncate(). As Christoph Hellwig pointed out, vmtruncate() is > a filesystem helper function, but filesystems may need to do more than > just a call to vmtruncate(). > > This patch moves the lower inode truncation out of ecryptfs_truncate() > and renames the function to truncate_upper(). truncate_upper() updates > an iattr for the lower inode to indicate if the lower inode needs to be > truncated upon return. ecryptfs_setattr() then calls notify_change(), > using the updated iattr for the lower inode, to complete the truncation. > > For eCryptfs functions needing to truncate, ecryptfs_truncate() is > reintroduced as a simple way to truncate the upper inode to a specified > size and then truncate the lower inode accordingly. > > https://bugs.launchpad.net/bugs/451368 > > This is a backport of commit 5f3ef64f4da1c587cdcfaaac72311225b7df094c > > Reported-by: Christoph Hellwig <hch@lst.de> > Acked-by: Dustin Kirkland <kirkland@canonical.com> > Cc: ecryptfs-devel@lists.launchpad.net > Cc: linux-fsdevel@vger.kernel.org > Signed-off-by: Tyler Hicks <tyhicks@linux.vnet.ibm.com> > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > fs/ecryptfs/inode.c | 99 ++++++++++++++++++++++++++++++++++---------------- > 1 files changed, 67 insertions(+), 32 deletions(-) > > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c > index 88ba4d4..7ce5471 100644 > --- a/fs/ecryptfs/inode.c > +++ b/fs/ecryptfs/inode.c > @@ -758,18 +758,23 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat, > } > > /** > - * ecryptfs_truncate > + * truncate_upper > * @dentry: The ecryptfs layer dentry > - * @new_length: The length to expand the file to > + * @ia: Address of the ecryptfs inode's attributes > + * @lower_ia: Address of the lower inode's attributes > * > * Function to handle truncations modifying the size of the file. Note > * that the file sizes are interpolated. When expanding, we are simply > - * writing strings of 0's out. When truncating, we need to modify the > - * underlying file size according to the page index interpolations. > + * writing strings of 0's out. When truncating, we truncate the upper > + * inode and update the lower_ia according to the page index > + * interpolations. If ATTR_SIZE is set in lower_ia->ia_valid upon return, > + * the caller must use lower_ia in a call to notify_change() to perform > + * the truncation of the lower inode. > * > * Returns zero on success; non-zero otherwise > */ > -int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) > +static int truncate_upper(struct dentry *dentry, struct iattr *ia, > + struct iattr *lower_ia) > { > int rc = 0; > struct inode *inode = dentry->d_inode; > @@ -780,8 +785,10 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) > loff_t lower_size_before_truncate; > loff_t lower_size_after_truncate; > > - if (unlikely((new_length == i_size))) > + if (unlikely((ia->ia_size == i_size))) { > + lower_ia->ia_valid &= ~ATTR_SIZE; > goto out; > + } > crypt_stat = &ecryptfs_inode_to_private(dentry->d_inode)->crypt_stat; > /* Set up a fake ecryptfs file, this is used to interface with > * the file in the underlying filesystem so that the > @@ -801,28 +808,30 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) > &fake_ecryptfs_file, > ecryptfs_inode_to_private(dentry->d_inode)->lower_file); > /* Switch on growing or shrinking file */ > - if (new_length > i_size) { > + if (ia->ia_size > i_size) { > char zero[] = { 0x00 }; > > + lower_ia->ia_valid &= ~ATTR_SIZE; > /* Write a single 0 at the last position of the file; > * this triggers code that will fill in 0's throughout > * the intermediate portion of the previous end of the > * file and the new and of the file */ > rc = ecryptfs_write(&fake_ecryptfs_file, zero, > - (new_length - 1), 1); > - } else { /* new_length < i_size_read(inode) */ > - /* We're chopping off all the pages down do the page > - * in which new_length is located. Fill in the end of > - * that page from (new_length & ~PAGE_CACHE_MASK) to > + (ia->ia_size - 1), 1); > + } else { /* ia->ia_size < i_size_read(inode) */ > + /* We're chopping off all the pages down to the page > + * in which ia->ia_size is located. Fill in the end of > + * that page from (ia->ia_size & ~PAGE_CACHE_MASK) to > * PAGE_CACHE_SIZE with zeros. */ > size_t num_zeros = (PAGE_CACHE_SIZE > - - (new_length & ~PAGE_CACHE_MASK)); > + - (ia->ia_size & ~PAGE_CACHE_MASK)); > > if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) { > - rc = vmtruncate(inode, new_length); > + rc = vmtruncate(inode, ia->ia_size); > if (rc) > goto out_free; > - rc = vmtruncate(lower_dentry->d_inode, new_length); > + lower_ia->ia_size = ia->ia_size; > + lower_ia->ia_valid |= ATTR_SIZE; > goto out_free; > } > if (num_zeros) { > @@ -834,7 +843,7 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) > goto out_free; > } > rc = ecryptfs_write(&fake_ecryptfs_file, zeros_virt, > - new_length, num_zeros); > + ia->ia_size, num_zeros); > kfree(zeros_virt); > if (rc) { > printk(KERN_ERR "Error attempting to zero out " > @@ -843,7 +852,7 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) > goto out_free; > } > } > - vmtruncate(inode, new_length); > + vmtruncate(inode, ia->ia_size); > rc = ecryptfs_write_inode_size_to_metadata(inode); > if (rc) { > printk(KERN_ERR "Problem with " > @@ -856,10 +865,12 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) > lower_size_before_truncate = > upper_size_to_lower_size(crypt_stat, i_size); > lower_size_after_truncate = > - upper_size_to_lower_size(crypt_stat, new_length); > - if (lower_size_after_truncate < lower_size_before_truncate) > - vmtruncate(lower_dentry->d_inode, > - lower_size_after_truncate); > + upper_size_to_lower_size(crypt_stat, ia->ia_size); > + if (lower_size_after_truncate < lower_size_before_truncate) { > + lower_ia->ia_size = lower_size_after_truncate; > + lower_ia->ia_valid |= ATTR_SIZE; > + } else > + lower_ia->ia_valid &= ~ATTR_SIZE; > } > out_free: > if (ecryptfs_file_to_private(&fake_ecryptfs_file)) > @@ -869,6 +880,33 @@ out: > return rc; > } > > +/** > + * ecryptfs_truncate > + * @dentry: The ecryptfs layer dentry > + * @new_length: The length to expand the file to > + * > + * Simple function that handles the truncation of an eCryptfs inode and > + * its corresponding lower inode. > + * > + * Returns zero on success; non-zero otherwise > + */ > +int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) > +{ > + struct iattr ia = { .ia_valid = ATTR_SIZE, .ia_size = new_length }; > + struct iattr lower_ia = { .ia_valid = 0 }; > + int 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); > + > + mutex_lock(&lower_dentry->d_inode->i_mutex); > + rc = notify_change(lower_dentry, &lower_ia); > + mutex_unlock(&lower_dentry->d_inode->i_mutex); > + } > + return rc; > +} > + > static int > ecryptfs_permission(struct inode *inode, int mask) > { > @@ -891,6 +929,7 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia) > { > int rc = 0; > struct dentry *lower_dentry; > + struct iattr lower_ia; > struct inode *inode; > struct inode *lower_inode; > struct ecryptfs_crypt_stat *crypt_stat; > @@ -929,15 +968,11 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia) > } > } > mutex_unlock(&crypt_stat->cs_mutex); > + memcpy(&lower_ia, ia, sizeof(lower_ia)); > + if (ia->ia_valid & ATTR_FILE) > + lower_ia.ia_file = ecryptfs_file_to_lower(ia->ia_file); > if (ia->ia_valid & ATTR_SIZE) { > - ecryptfs_printk(KERN_DEBUG, > - "ia->ia_valid = [0x%x] ATTR_SIZE" " = [0x%x]\n", > - ia->ia_valid, ATTR_SIZE); > - rc = ecryptfs_truncate(dentry, ia->ia_size); > - /* ecryptfs_truncate handles resizing of the lower file */ > - ia->ia_valid &= ~ATTR_SIZE; > - ecryptfs_printk(KERN_DEBUG, "ia->ia_valid = [%x]\n", > - ia->ia_valid); > + rc = truncate_upper(dentry, ia, &lower_ia); > if (rc < 0) > goto out; > } > @@ -946,11 +981,11 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia) > * mode change is for clearing setuid/setgid bits. Allow lower fs > * to interpret this in its own way. > */ > - if (ia->ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID)) > - ia->ia_valid &= ~ATTR_MODE; > + if (lower_ia.ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID)) > + lower_ia.ia_valid &= ~ATTR_MODE; > > mutex_lock(&lower_dentry->d_inode->i_mutex); > - rc = notify_change(lower_dentry, ia); > + rc = notify_change(lower_dentry, &lower_ia); > mutex_unlock(&lower_dentry->d_inode->i_mutex); > out: > fsstack_copy_attr_all(inode, lower_inode, NULL); In upstream since 2.6.33, looking similar as the backport and trusting Colin's testing... Acked-by: Stefan Bader <stefan.bader@canonical.com>
Colin - in your soak testing prior to the application of this patch, were you able to detect corrupted or leaked inodes ? If so, can you distill your tests and attach the reproducer to the bug report ? rtg
On 16/02/12 15:03, Tim Gardner wrote: > Colin - in your soak testing prior to the application of this patch, > were you able to detect corrupted or leaked inodes ? If so, can you > distill your tests and attach the reproducer to the bug report ? That's a good question. As yet, I have not yet been able to figure out a simple way to detect this and spot any theoretical corruption or leakage. > > rtg
On 02/16/2012 08:08 AM, Colin Ian King wrote: > On 16/02/12 15:03, Tim Gardner wrote: >> Colin - in your soak testing prior to the application of this patch, >> were you able to detect corrupted or leaked inodes ? If so, can you >> distill your tests and attach the reproducer to the bug report ? > > That's a good question. As yet, I have not yet been able to figure out a > simple way to detect this and spot any theoretical corruption or leakage. >> >> rtg > I've got this mammoth Emerald with lots of CPUs and spindles. Lemme run some soak testing with the patch applied. rtg
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c index 88ba4d4..7ce5471 100644 --- a/fs/ecryptfs/inode.c +++ b/fs/ecryptfs/inode.c @@ -758,18 +758,23 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat, } /** - * ecryptfs_truncate + * truncate_upper * @dentry: The ecryptfs layer dentry - * @new_length: The length to expand the file to + * @ia: Address of the ecryptfs inode's attributes + * @lower_ia: Address of the lower inode's attributes * * Function to handle truncations modifying the size of the file. Note * that the file sizes are interpolated. When expanding, we are simply - * writing strings of 0's out. When truncating, we need to modify the - * underlying file size according to the page index interpolations. + * writing strings of 0's out. When truncating, we truncate the upper + * inode and update the lower_ia according to the page index + * interpolations. If ATTR_SIZE is set in lower_ia->ia_valid upon return, + * the caller must use lower_ia in a call to notify_change() to perform + * the truncation of the lower inode. * * Returns zero on success; non-zero otherwise */ -int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) +static int truncate_upper(struct dentry *dentry, struct iattr *ia, + struct iattr *lower_ia) { int rc = 0; struct inode *inode = dentry->d_inode; @@ -780,8 +785,10 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) loff_t lower_size_before_truncate; loff_t lower_size_after_truncate; - if (unlikely((new_length == i_size))) + if (unlikely((ia->ia_size == i_size))) { + lower_ia->ia_valid &= ~ATTR_SIZE; goto out; + } crypt_stat = &ecryptfs_inode_to_private(dentry->d_inode)->crypt_stat; /* Set up a fake ecryptfs file, this is used to interface with * the file in the underlying filesystem so that the @@ -801,28 +808,30 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) &fake_ecryptfs_file, ecryptfs_inode_to_private(dentry->d_inode)->lower_file); /* Switch on growing or shrinking file */ - if (new_length > i_size) { + if (ia->ia_size > i_size) { char zero[] = { 0x00 }; + lower_ia->ia_valid &= ~ATTR_SIZE; /* Write a single 0 at the last position of the file; * this triggers code that will fill in 0's throughout * the intermediate portion of the previous end of the * file and the new and of the file */ rc = ecryptfs_write(&fake_ecryptfs_file, zero, - (new_length - 1), 1); - } else { /* new_length < i_size_read(inode) */ - /* We're chopping off all the pages down do the page - * in which new_length is located. Fill in the end of - * that page from (new_length & ~PAGE_CACHE_MASK) to + (ia->ia_size - 1), 1); + } else { /* ia->ia_size < i_size_read(inode) */ + /* We're chopping off all the pages down to the page + * in which ia->ia_size is located. Fill in the end of + * that page from (ia->ia_size & ~PAGE_CACHE_MASK) to * PAGE_CACHE_SIZE with zeros. */ size_t num_zeros = (PAGE_CACHE_SIZE - - (new_length & ~PAGE_CACHE_MASK)); + - (ia->ia_size & ~PAGE_CACHE_MASK)); if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) { - rc = vmtruncate(inode, new_length); + rc = vmtruncate(inode, ia->ia_size); if (rc) goto out_free; - rc = vmtruncate(lower_dentry->d_inode, new_length); + lower_ia->ia_size = ia->ia_size; + lower_ia->ia_valid |= ATTR_SIZE; goto out_free; } if (num_zeros) { @@ -834,7 +843,7 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) goto out_free; } rc = ecryptfs_write(&fake_ecryptfs_file, zeros_virt, - new_length, num_zeros); + ia->ia_size, num_zeros); kfree(zeros_virt); if (rc) { printk(KERN_ERR "Error attempting to zero out " @@ -843,7 +852,7 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) goto out_free; } } - vmtruncate(inode, new_length); + vmtruncate(inode, ia->ia_size); rc = ecryptfs_write_inode_size_to_metadata(inode); if (rc) { printk(KERN_ERR "Problem with " @@ -856,10 +865,12 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) lower_size_before_truncate = upper_size_to_lower_size(crypt_stat, i_size); lower_size_after_truncate = - upper_size_to_lower_size(crypt_stat, new_length); - if (lower_size_after_truncate < lower_size_before_truncate) - vmtruncate(lower_dentry->d_inode, - lower_size_after_truncate); + upper_size_to_lower_size(crypt_stat, ia->ia_size); + if (lower_size_after_truncate < lower_size_before_truncate) { + lower_ia->ia_size = lower_size_after_truncate; + lower_ia->ia_valid |= ATTR_SIZE; + } else + lower_ia->ia_valid &= ~ATTR_SIZE; } out_free: if (ecryptfs_file_to_private(&fake_ecryptfs_file)) @@ -869,6 +880,33 @@ out: return rc; } +/** + * ecryptfs_truncate + * @dentry: The ecryptfs layer dentry + * @new_length: The length to expand the file to + * + * Simple function that handles the truncation of an eCryptfs inode and + * its corresponding lower inode. + * + * Returns zero on success; non-zero otherwise + */ +int ecryptfs_truncate(struct dentry *dentry, loff_t new_length) +{ + struct iattr ia = { .ia_valid = ATTR_SIZE, .ia_size = new_length }; + struct iattr lower_ia = { .ia_valid = 0 }; + int 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); + + mutex_lock(&lower_dentry->d_inode->i_mutex); + rc = notify_change(lower_dentry, &lower_ia); + mutex_unlock(&lower_dentry->d_inode->i_mutex); + } + return rc; +} + static int ecryptfs_permission(struct inode *inode, int mask) { @@ -891,6 +929,7 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia) { int rc = 0; struct dentry *lower_dentry; + struct iattr lower_ia; struct inode *inode; struct inode *lower_inode; struct ecryptfs_crypt_stat *crypt_stat; @@ -929,15 +968,11 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia) } } mutex_unlock(&crypt_stat->cs_mutex); + memcpy(&lower_ia, ia, sizeof(lower_ia)); + if (ia->ia_valid & ATTR_FILE) + lower_ia.ia_file = ecryptfs_file_to_lower(ia->ia_file); if (ia->ia_valid & ATTR_SIZE) { - ecryptfs_printk(KERN_DEBUG, - "ia->ia_valid = [0x%x] ATTR_SIZE" " = [0x%x]\n", - ia->ia_valid, ATTR_SIZE); - rc = ecryptfs_truncate(dentry, ia->ia_size); - /* ecryptfs_truncate handles resizing of the lower file */ - ia->ia_valid &= ~ATTR_SIZE; - ecryptfs_printk(KERN_DEBUG, "ia->ia_valid = [%x]\n", - ia->ia_valid); + rc = truncate_upper(dentry, ia, &lower_ia); if (rc < 0) goto out; } @@ -946,11 +981,11 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia) * mode change is for clearing setuid/setgid bits. Allow lower fs * to interpret this in its own way. */ - if (ia->ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID)) - ia->ia_valid &= ~ATTR_MODE; + if (lower_ia.ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID)) + lower_ia.ia_valid &= ~ATTR_MODE; mutex_lock(&lower_dentry->d_inode->i_mutex); - rc = notify_change(lower_dentry, ia); + rc = notify_change(lower_dentry, &lower_ia); mutex_unlock(&lower_dentry->d_inode->i_mutex); out: fsstack_copy_attr_all(inode, lower_inode, NULL);