cleanup truncate handling in ecryptfs v3

cleanup truncate handling in ecryptfs v3

[Christoph Hellwig]

Hi Tyler,

this series cleans up the truncate handling in ecryptfs.  I did
it as preparation for some changes into size changing truncate
VFS interfaces I'm looking into in the moment.  The changes have
passed the regression test suite in the userspace ecryptfs
repository and against the ecryptfs next branch.

Changes since v2:
 - change the calling convention to only pass the upper ia to
   __ecryptfs_truncate

Changes since v1:
 - fix a pre-existing spelling error in a comment
 - pass an explicit new_size argument to the main truncate routines
 - fix a comment that kept obsolete information

Diffstat:
 inode.c |  257 ++++++++++++++++++++++++++++------------------------------------
 1 file changed, 115 insertions(+), 142 deletions(-)

[PATCH 1/7] ecryptfs: cleanup ecryptfs_setattr

[Christoph Hellwig]

Initialize variables at declaration time where applicable and reformat
conditionals to match the kernel coding style.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ecryptfs/inode.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 8ab014db3e03..81cf42d01ec5 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -886,25 +886,23 @@ ecryptfs_permission(struct mnt_idmap *idmap, struct inode *inode,
 static int ecryptfs_setattr(struct mnt_idmap *idmap,
 			    struct dentry *dentry, struct iattr *ia)
 {
-	int rc = 0;
-	struct dentry *lower_dentry;
+	struct inode *inode = d_inode(dentry);
+	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
+	struct inode *lower_inode = ecryptfs_inode_to_lower(inode);
 	struct iattr lower_ia;
-	struct inode *inode;
-	struct inode *lower_inode;
 	struct ecryptfs_crypt_stat *crypt_stat;
+	int rc;
 
 	crypt_stat = &ecryptfs_inode_to_private(d_inode(dentry))->crypt_stat;
 	if (!(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED))
 		ecryptfs_init_crypt_stat(crypt_stat);
-	inode = d_inode(dentry);
-	lower_inode = ecryptfs_inode_to_lower(inode);
-	lower_dentry = ecryptfs_dentry_to_lower(dentry);
+
 	mutex_lock(&crypt_stat->cs_mutex);
 	if (d_is_dir(dentry))
 		crypt_stat->flags &= ~(ECRYPTFS_ENCRYPTED);
-	else if (d_is_reg(dentry)
-		 && (!(crypt_stat->flags & ECRYPTFS_POLICY_APPLIED)
-		     || !(crypt_stat->flags & ECRYPTFS_KEY_VALID))) {
+	else if (d_is_reg(dentry) &&
+		 (!(crypt_stat->flags & ECRYPTFS_POLICY_APPLIED) ||
+		  !(crypt_stat->flags & ECRYPTFS_KEY_VALID))) {
 		struct ecryptfs_mount_crypt_stat *mount_crypt_stat;
 
 		mount_crypt_stat = &ecryptfs_superblock_to_private(
@@ -917,8 +915,8 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 		rc = ecryptfs_read_metadata(dentry);
 		ecryptfs_put_lower_file(inode);
 		if (rc) {
-			if (!(mount_crypt_stat->flags
-			      & ECRYPTFS_PLAINTEXT_PASSTHROUGH_ENABLED)) {
+			if (!(mount_crypt_stat->flags &
+			      ECRYPTFS_PLAINTEXT_PASSTHROUGH_ENABLED)) {
 				rc = -EIO;
 				printk(KERN_WARNING "Either the lower file "
 				       "is not in a valid eCryptfs format, "
-- 
2.47.3

[PATCH 2/7] ecryptfs: streamline truncate_upper

[Christoph Hellwig]

Use a few strategic gotos to keep reduce indentation and keep the
main reduce size flow outside of branches.  Switch all touched comments
to normal kernel style and avoid breaks in printed strings for all
the code touched.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ecryptfs/inode.c | 115 +++++++++++++++++++++++---------------------
 1 file changed, 60 insertions(+), 55 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 81cf42d01ec5..695573850569 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -725,83 +725,88 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
 static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 			  struct iattr *lower_ia)
 {
-	int rc = 0;
 	struct inode *inode = d_inode(dentry);
 	struct ecryptfs_crypt_stat *crypt_stat;
 	loff_t i_size = i_size_read(inode);
 	loff_t lower_size_before_truncate;
 	loff_t lower_size_after_truncate;
+	size_t num_zeros;
+	int rc;
 
 	if (unlikely((ia->ia_size == i_size))) {
 		lower_ia->ia_valid &= ~ATTR_SIZE;
 		return 0;
 	}
+
 	rc = ecryptfs_get_lower_file(dentry, inode);
 	if (rc)
 		return rc;
-	crypt_stat = &ecryptfs_inode_to_private(d_inode(dentry))->crypt_stat;
-	/* Switch on growing or shrinking file */
+
 	if (ia->ia_size > i_size) {
 		char zero[] = { 0x00 };
 
+		/*
+		 * 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 end of the file.
+		 */
+		rc = ecryptfs_write(inode, zero, ia->ia_size - 1, 1);
 		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(inode, zero,
-				    (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_MASK) to
-		 * PAGE_SIZE with zeros. */
-		size_t num_zeros = (PAGE_SIZE
-				    - (ia->ia_size & ~PAGE_MASK));
-
-		if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
-			truncate_setsize(inode, ia->ia_size);
-			lower_ia->ia_size = ia->ia_size;
-			lower_ia->ia_valid |= ATTR_SIZE;
-			goto out;
-		}
-		if (num_zeros) {
-			char *zeros_virt;
+		goto out;
+	}
 
-			zeros_virt = kzalloc(num_zeros, GFP_KERNEL);
-			if (!zeros_virt) {
-				rc = -ENOMEM;
-				goto out;
-			}
-			rc = ecryptfs_write(inode, zeros_virt,
-					    ia->ia_size, num_zeros);
-			kfree(zeros_virt);
-			if (rc) {
-				printk(KERN_ERR "Error attempting to zero out "
-				       "the remainder of the end page on "
-				       "reducing truncate; rc = [%d]\n", rc);
-				goto out;
-			}
-		}
+	crypt_stat = &ecryptfs_inode_to_private(d_inode(dentry))->crypt_stat;
+	if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
 		truncate_setsize(inode, ia->ia_size);
-		rc = ecryptfs_write_inode_size_to_metadata(inode);
+		lower_ia->ia_size = ia->ia_size;
+		lower_ia->ia_valid |= ATTR_SIZE;
+		goto out;
+	}
+
+	/*
+	 * 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_MASK) to PAGE_SIZE with zeros.
+	 */
+	num_zeros = PAGE_SIZE - (ia->ia_size & ~PAGE_MASK);
+	if (num_zeros) {
+		char *zeros_virt;
+
+		zeros_virt = kzalloc(num_zeros, GFP_KERNEL);
+		if (!zeros_virt) {
+			rc = -ENOMEM;
+			goto out;
+		}
+		rc = ecryptfs_write(inode, zeros_virt, ia->ia_size, num_zeros);
+		kfree(zeros_virt);
 		if (rc) {
-			printk(KERN_ERR	"Problem with "
-			       "ecryptfs_write_inode_size_to_metadata; "
-			       "rc = [%d]\n", rc);
+			pr_err("Error attempting to zero out the remainder of the end page on reducing truncate; rc = [%d]\n",
+				rc);
 			goto out;
 		}
-		/* We are reducing the size of the ecryptfs file, and need to
-		 * know if we need to reduce the size of the lower file. */
-		lower_size_before_truncate =
-		    upper_size_to_lower_size(crypt_stat, i_size);
-		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;
+	}
+	truncate_setsize(inode, ia->ia_size);
+	rc = ecryptfs_write_inode_size_to_metadata(inode);
+	if (rc) {
+		pr_err("Problem with ecryptfs_write_inode_size_to_metadata; rc = [%d]\n",
+			rc);
+		goto out;
+	}
+
+	/*
+	 * We are reducing the size of the ecryptfs file, and need to know if we
+	 * need to reduce the size of the lower file.
+	 */
+	lower_size_before_truncate =
+		upper_size_to_lower_size(crypt_stat, i_size);
+	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:
 	ecryptfs_put_lower_file(inode);
-- 
2.47.3

[PATCH 3/7] ecryptfs: use ZERO_PAGE instead of allocating zeroed memory in truncate_upper

[Christoph Hellwig]

Use the existing pre-zeroed memory instead of allocating a new chunk.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ecryptfs/inode.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 695573850569..daa63b7dd015 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -771,15 +771,8 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 	 */
 	num_zeros = PAGE_SIZE - (ia->ia_size & ~PAGE_MASK);
 	if (num_zeros) {
-		char *zeros_virt;
-
-		zeros_virt = kzalloc(num_zeros, GFP_KERNEL);
-		if (!zeros_virt) {
-			rc = -ENOMEM;
-			goto out;
-		}
-		rc = ecryptfs_write(inode, zeros_virt, ia->ia_size, num_zeros);
-		kfree(zeros_virt);
+		rc = ecryptfs_write(inode, page_address(ZERO_PAGE(0)),
+				ia->ia_size, num_zeros);
 		if (rc) {
 			pr_err("Error attempting to zero out the remainder of the end page on reducing truncate; rc = [%d]\n",
 				rc);
-- 
2.47.3

[PATCH 4/7] ecryptfs: combine the two ATTR_SIZE blocks in ecryptfs_setattr

[Christoph Hellwig]

Simplify the logic in ecryptfs_setattr by combining the two ATTR_SIZE
blocks.  This initializes lower_ia before the size check, which is
obviously correct as the size check doesn't look at it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ecryptfs/inode.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index daa63b7dd015..ec6aae5af1f8 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -934,16 +934,15 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 	rc = setattr_prepare(&nop_mnt_idmap, dentry, ia);
 	if (rc)
 		goto out;
-	if (ia->ia_valid & ATTR_SIZE) {
-		rc = ecryptfs_inode_newsize_ok(inode, ia->ia_size);
-		if (rc)
-			goto out;
-	}
 
 	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) {
+		rc = ecryptfs_inode_newsize_ok(inode, ia->ia_size);
+		if (rc)
+			goto out;
+
 		rc = truncate_upper(dentry, ia, &lower_ia);
 		if (rc < 0)
 			goto out;
-- 
2.47.3

[PATCH 5/7] ecryptfs: merge ecryptfs_inode_newsize_ok into truncate_upper

[Christoph Hellwig]

Both callers of ecryptfs_inode_newsize_ok call truncate_upper right
after.  Merge ecryptfs_inode_newsize_ok into truncate_upper to simplify
the logic.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ecryptfs/inode.c | 52 +++++++++++++++------------------------------
 1 file changed, 17 insertions(+), 35 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index ec6aae5af1f8..4ec3e76f0562 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -738,6 +738,23 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 		return 0;
 	}
 
+	crypt_stat = &ecryptfs_inode_to_private(inode)->crypt_stat;
+	lower_size_before_truncate =
+		upper_size_to_lower_size(crypt_stat, i_size);
+	lower_size_after_truncate =
+		upper_size_to_lower_size(crypt_stat, ia->ia_size);
+	if (lower_size_after_truncate > lower_size_before_truncate) {
+		/*
+		 * 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.
+		 */
+		rc = inode_newsize_ok(inode, lower_size_after_truncate);
+		if (rc)
+			return rc;
+	}
+
 	rc = ecryptfs_get_lower_file(dentry, inode);
 	if (rc)
 		return rc;
@@ -756,7 +773,6 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 		goto out;
 	}
 
-	crypt_stat = &ecryptfs_inode_to_private(d_inode(dentry))->crypt_stat;
 	if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
 		truncate_setsize(inode, ia->ia_size);
 		lower_ia->ia_size = ia->ia_size;
@@ -791,10 +807,6 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 	 * We are reducing the size of the ecryptfs file, and need to know if we
 	 * need to reduce the size of the lower file.
 	 */
-	lower_size_before_truncate =
-		upper_size_to_lower_size(crypt_stat, i_size);
-	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;
@@ -806,28 +818,6 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 	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
@@ -844,10 +834,6 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length)
 	struct iattr lower_ia = { .ia_valid = 0 };
 	int rc;
 
-	rc = ecryptfs_inode_newsize_ok(d_inode(dentry), 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);
@@ -939,10 +925,6 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 	if (ia->ia_valid & ATTR_FILE)
 		lower_ia.ia_file = ecryptfs_file_to_lower(ia->ia_file);
 	if (ia->ia_valid & ATTR_SIZE) {
-		rc = ecryptfs_inode_newsize_ok(inode, ia->ia_size);
-		if (rc)
-			goto out;
-
 		rc = truncate_upper(dentry, ia, &lower_ia);
 		if (rc < 0)
 			goto out;
-- 
2.47.3

[PATCH 6/7] ecryptfs: factor out a ecryptfs_iattr_to_lower helper

[Christoph Hellwig]

Prepare for using the code to create a lower struct iattr in multiple
places.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ecryptfs/inode.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 4ec3e76f0562..a06b84033ff3 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -677,6 +677,20 @@ static const char *ecryptfs_get_link(struct dentry *dentry,
 	return buf;
 }
 
+static void ecryptfs_iattr_to_lower(struct iattr *lower_ia,
+		const struct iattr *ia)
+{
+	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 the mode change is for clearing setuid/setgid bits, allow the lower
+	 * file system to interpret this in its own way.
+	 */
+	if (lower_ia->ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID))
+		lower_ia->ia_valid &= ~ATTR_MODE;
+}
+
 /**
  * upper_size_to_lower_size
  * @crypt_stat: Crypt_stat associated with file
@@ -921,21 +935,13 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 	if (rc)
 		goto out;
 
-	memcpy(&lower_ia, ia, sizeof(lower_ia));
-	if (ia->ia_valid & ATTR_FILE)
-		lower_ia.ia_file = ecryptfs_file_to_lower(ia->ia_file);
+	ecryptfs_iattr_to_lower(&lower_ia, ia);
 	if (ia->ia_valid & ATTR_SIZE) {
 		rc = truncate_upper(dentry, ia, &lower_ia);
 		if (rc < 0)
 			goto out;
 	}
 
-	/*
-	 * mode change is for clearing setuid/setgid bits. Allow lower fs
-	 * to interpret this in its own way.
-	 */
-	if (lower_ia.ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID))
-		lower_ia.ia_valid &= ~ATTR_MODE;
 
 	inode_lock(d_inode(lower_dentry));
 	rc = notify_change(&nop_mnt_idmap, lower_dentry, &lower_ia, NULL);
-- 
2.47.3

[PATCH 7/7] ecryptfs: keep the lower iattr contained in truncate_upper

[Christoph Hellwig]

Currently the two callers of truncate_upper handle passing information
very differently.  ecryptfs_truncate passes a zeroed lower_ia and expects
truncate_upper to fill it in from the upper ia created just for that,
while ecryptfs_setattr passes a fully initialized lower_ia copied from
the upper one.  Both of them then call notify_change on the lower_ia.

Switch to only passing the upper ia, and derive the lower ia from it
inside truncate_upper, and call notify_change inside the function itself.
Because the old name is misleading now, rename the resulting function to
__ecryptfs_truncate as it deals with both the lower and upper inodes.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ecryptfs/inode.c | 82 ++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 46 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index a06b84033ff3..546c1fe692c0 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -721,36 +721,33 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
 }
 
 /**
- * truncate_upper
+ * __ecryptfs_truncate
  * @dentry: The ecryptfs layer dentry
  * @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 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.
+ * 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 truncate the upper inode and update the lower_ia
+ * according to the page index interpolations.
  *
  * Returns zero on success; non-zero otherwise
  */
-static int truncate_upper(struct dentry *dentry, struct iattr *ia,
-			  struct iattr *lower_ia)
+static int __ecryptfs_truncate(struct dentry *dentry, const struct iattr *ia)
 {
+	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
 	struct inode *inode = d_inode(dentry);
 	struct ecryptfs_crypt_stat *crypt_stat;
 	loff_t i_size = i_size_read(inode);
 	loff_t lower_size_before_truncate;
 	loff_t lower_size_after_truncate;
+	struct iattr lower_ia;
 	size_t num_zeros;
 	int rc;
 
-	if (unlikely((ia->ia_size == i_size))) {
-		lower_ia->ia_valid &= ~ATTR_SIZE;
+	ecryptfs_iattr_to_lower(&lower_ia, ia);
+
+	if (unlikely((ia->ia_size == i_size)))
 		return 0;
-	}
 
 	crypt_stat = &ecryptfs_inode_to_private(inode)->crypt_stat;
 	lower_size_before_truncate =
@@ -783,15 +780,13 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 		 * new end of the file.
 		 */
 		rc = ecryptfs_write(inode, zero, ia->ia_size - 1, 1);
-		lower_ia->ia_valid &= ~ATTR_SIZE;
 		goto out;
 	}
 
 	if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
 		truncate_setsize(inode, ia->ia_size);
-		lower_ia->ia_size = ia->ia_size;
-		lower_ia->ia_valid |= ATTR_SIZE;
-		goto out;
+		lower_ia.ia_size = ia->ia_size;
+		goto set_size;
 	}
 
 	/*
@@ -821,12 +816,15 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 	 * We are reducing the size of the ecryptfs file, and need to know if we
 	 * need to reduce the size of the lower file.
 	 */
-	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;
-	}
+	if (lower_size_after_truncate >= lower_size_before_truncate)
+		goto out;
+
+	lower_ia.ia_size = lower_size_after_truncate;
+set_size:
+	lower_ia.ia_valid |= ATTR_SIZE;
+	inode_lock(d_inode(lower_dentry));
+	rc = notify_change(&nop_mnt_idmap, lower_dentry, &lower_ia, NULL);
+	inode_unlock(d_inode(lower_dentry));
 out:
 	ecryptfs_put_lower_file(inode);
 	return rc;
@@ -844,20 +842,12 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
  */
 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);
+	const struct iattr ia = {
+		.ia_valid	= ATTR_SIZE,
+		.ia_size	= new_length,
+	};
 
-		inode_lock(d_inode(lower_dentry));
-		rc = notify_change(&nop_mnt_idmap, lower_dentry,
-				   &lower_ia, NULL);
-		inode_unlock(d_inode(lower_dentry));
-	}
-	return rc;
+	return __ecryptfs_truncate(dentry, &ia);
 }
 
 static int
@@ -887,7 +877,6 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 	struct inode *inode = d_inode(dentry);
 	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
 	struct inode *lower_inode = ecryptfs_inode_to_lower(inode);
-	struct iattr lower_ia;
 	struct ecryptfs_crypt_stat *crypt_stat;
 	int rc;
 
@@ -935,17 +924,18 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 	if (rc)
 		goto out;
 
-	ecryptfs_iattr_to_lower(&lower_ia, ia);
 	if (ia->ia_valid & ATTR_SIZE) {
-		rc = truncate_upper(dentry, ia, &lower_ia);
-		if (rc < 0)
-			goto out;
-	}
+		rc = __ecryptfs_truncate(dentry, ia);
+	} else {
+		struct iattr lower_ia;
 
+		ecryptfs_iattr_to_lower(&lower_ia, ia);
 
-	inode_lock(d_inode(lower_dentry));
-	rc = notify_change(&nop_mnt_idmap, lower_dentry, &lower_ia, NULL);
-	inode_unlock(d_inode(lower_dentry));
+		inode_lock(d_inode(lower_dentry));
+		rc = notify_change(&nop_mnt_idmap, lower_dentry, &lower_ia,
+				NULL);
+		inode_unlock(d_inode(lower_dentry));
+	}
 out:
 	fsstack_copy_attr_all(inode, lower_inode);
 	return rc;
-- 
2.47.3

Re: [PATCH 2/7] ecryptfs: streamline truncate_upper

[Tyler Hicks]

On 2026-04-08 08:06:37, Christoph Hellwig wrote:
> Use a few strategic gotos to keep reduce indentation and keep the
> main reduce size flow outside of branches.  Switch all touched comments
> to normal kernel style and avoid breaks in printed strings for all
> the code touched.

That garbled first sentence is still here. I'll touch it up according to
our email exchange on v1.

Tyler

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/ecryptfs/inode.c | 115 +++++++++++++++++++++++---------------------
>  1 file changed, 60 insertions(+), 55 deletions(-)
> 
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 81cf42d01ec5..695573850569 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -725,83 +725,88 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
>  static int truncate_upper(struct dentry *dentry, struct iattr *ia,
>  			  struct iattr *lower_ia)
>  {
> -	int rc = 0;
>  	struct inode *inode = d_inode(dentry);
>  	struct ecryptfs_crypt_stat *crypt_stat;
>  	loff_t i_size = i_size_read(inode);
>  	loff_t lower_size_before_truncate;
>  	loff_t lower_size_after_truncate;
> +	size_t num_zeros;
> +	int rc;
>  
>  	if (unlikely((ia->ia_size == i_size))) {
>  		lower_ia->ia_valid &= ~ATTR_SIZE;
>  		return 0;
>  	}
> +
>  	rc = ecryptfs_get_lower_file(dentry, inode);
>  	if (rc)
>  		return rc;
> -	crypt_stat = &ecryptfs_inode_to_private(d_inode(dentry))->crypt_stat;
> -	/* Switch on growing or shrinking file */
> +
>  	if (ia->ia_size > i_size) {
>  		char zero[] = { 0x00 };
>  
> +		/*
> +		 * 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 end of the file.
> +		 */
> +		rc = ecryptfs_write(inode, zero, ia->ia_size - 1, 1);
>  		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(inode, zero,
> -				    (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_MASK) to
> -		 * PAGE_SIZE with zeros. */
> -		size_t num_zeros = (PAGE_SIZE
> -				    - (ia->ia_size & ~PAGE_MASK));
> -
> -		if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
> -			truncate_setsize(inode, ia->ia_size);
> -			lower_ia->ia_size = ia->ia_size;
> -			lower_ia->ia_valid |= ATTR_SIZE;
> -			goto out;
> -		}
> -		if (num_zeros) {
> -			char *zeros_virt;
> +		goto out;
> +	}
>  
> -			zeros_virt = kzalloc(num_zeros, GFP_KERNEL);
> -			if (!zeros_virt) {
> -				rc = -ENOMEM;
> -				goto out;
> -			}
> -			rc = ecryptfs_write(inode, zeros_virt,
> -					    ia->ia_size, num_zeros);
> -			kfree(zeros_virt);
> -			if (rc) {
> -				printk(KERN_ERR "Error attempting to zero out "
> -				       "the remainder of the end page on "
> -				       "reducing truncate; rc = [%d]\n", rc);
> -				goto out;
> -			}
> -		}
> +	crypt_stat = &ecryptfs_inode_to_private(d_inode(dentry))->crypt_stat;
> +	if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
>  		truncate_setsize(inode, ia->ia_size);
> -		rc = ecryptfs_write_inode_size_to_metadata(inode);
> +		lower_ia->ia_size = ia->ia_size;
> +		lower_ia->ia_valid |= ATTR_SIZE;
> +		goto out;
> +	}
> +
> +	/*
> +	 * 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_MASK) to PAGE_SIZE with zeros.
> +	 */
> +	num_zeros = PAGE_SIZE - (ia->ia_size & ~PAGE_MASK);
> +	if (num_zeros) {
> +		char *zeros_virt;
> +
> +		zeros_virt = kzalloc(num_zeros, GFP_KERNEL);
> +		if (!zeros_virt) {
> +			rc = -ENOMEM;
> +			goto out;
> +		}
> +		rc = ecryptfs_write(inode, zeros_virt, ia->ia_size, num_zeros);
> +		kfree(zeros_virt);
>  		if (rc) {
> -			printk(KERN_ERR	"Problem with "
> -			       "ecryptfs_write_inode_size_to_metadata; "
> -			       "rc = [%d]\n", rc);
> +			pr_err("Error attempting to zero out the remainder of the end page on reducing truncate; rc = [%d]\n",
> +				rc);
>  			goto out;
>  		}
> -		/* We are reducing the size of the ecryptfs file, and need to
> -		 * know if we need to reduce the size of the lower file. */
> -		lower_size_before_truncate =
> -		    upper_size_to_lower_size(crypt_stat, i_size);
> -		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;
> +	}
> +	truncate_setsize(inode, ia->ia_size);
> +	rc = ecryptfs_write_inode_size_to_metadata(inode);
> +	if (rc) {
> +		pr_err("Problem with ecryptfs_write_inode_size_to_metadata; rc = [%d]\n",
> +			rc);
> +		goto out;
> +	}
> +
> +	/*
> +	 * We are reducing the size of the ecryptfs file, and need to know if we
> +	 * need to reduce the size of the lower file.
> +	 */
> +	lower_size_before_truncate =
> +		upper_size_to_lower_size(crypt_stat, i_size);
> +	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:
>  	ecryptfs_put_lower_file(inode);
> -- 
> 2.47.3
> 

Re: cleanup truncate handling in ecryptfs v3

[Tyler Hicks]

On Wed, 08 Apr 2026 08:06:35 +0200, Christoph Hellwig wrote:
> this series cleans up the truncate handling in ecryptfs.  I did
> it as preparation for some changes into size changing truncate
> VFS interfaces I'm looking into in the moment.  The changes have
> passed the regression test suite in the userspace ecryptfs
> repository and against the ecryptfs next branch.
> 
> Changes since v2:
>  - change the calling convention to only pass the upper ia to
>    __ecryptfs_truncate
> 
> [...]

Thank you! This has been applied to the next branch of the tyhicks/ecryptfs.git tree.

You can find a direct link below but please be aware that the commit hash is
unstable and, therefore, the URL may not be valid in the future.

[1/7] ecryptfs: cleanup ecryptfs_setattr
      https://git.kernel.org/tyhicks/ecryptfs/c/8f61364322a07ff6c35691b575d6fbda8e71e29d
[2/7] ecryptfs: streamline truncate_upper
      https://git.kernel.org/tyhicks/ecryptfs/c/b109187378615e683d8d8a24f4bc246bd3fb7b26
[3/7] ecryptfs: use ZERO_PAGE instead of allocating zeroed memory in truncate_upper
      https://git.kernel.org/tyhicks/ecryptfs/c/b19fe74e0fc970cef90bb78ddb473ae0356bce94
[4/7] ecryptfs: combine the two ATTR_SIZE blocks in ecryptfs_setattr
      https://git.kernel.org/tyhicks/ecryptfs/c/472dea1d2235439c0c25850d53deffc517cc8c61
[5/7] ecryptfs: merge ecryptfs_inode_newsize_ok into truncate_upper
      https://git.kernel.org/tyhicks/ecryptfs/c/081447ecfc255cb63b6e392cd01d9f684d4df5b8
[6/7] ecryptfs: factor out a ecryptfs_iattr_to_lower helper
      https://git.kernel.org/tyhicks/ecryptfs/c/5d1f0e8cd9482ddb5318f765f7ca508ce707cf83
[7/7] ecryptfs: keep the lower iattr contained in truncate_upper
      https://git.kernel.org/tyhicks/ecryptfs/c/e836ec1819b0cc50e0b45a53b0bdce6c596f0207

Tyler