cleanup truncate handling in ecryptfs

cleanup truncate handling in ecryptfs

[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.

Diffstat:
 inode.c |  228 +++++++++++++++++++++++++++-------------------------------------
 1 file changed, 98 insertions(+), 130 deletions(-)

[PATCH 1/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 8ab014db3e03..cf20873a9cc4 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 and 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 2/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 cf20873a9cc4..46dc867a8860 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -891,25 +891,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(
@@ -922,8 +920,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 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 46dc867a8860..57df35a22e9c 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 57df35a22e9c..7a3da72eb3c6 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: sanitize struct iattr handling 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.

Switch to only passing a lower ia which must have ia_size set to the
expected lower size, which cleans up the logic in truncate_upper and
ecryptfs_truncate.

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

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 7a3da72eb3c6..a7dc25fae8ee 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -709,7 +709,6 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
 /**
  * truncate_upper
  * @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
@@ -722,8 +721,7 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
  *
  * Returns zero on success; non-zero otherwise
  */
-static int truncate_upper(struct dentry *dentry, struct iattr *ia,
-			  struct iattr *lower_ia)
+static int truncate_upper(struct dentry *dentry, struct iattr *lower_ia)
 {
 	struct inode *inode = d_inode(dentry);
 	struct ecryptfs_crypt_stat *crypt_stat;
@@ -733,7 +731,7 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 	size_t num_zeros;
 	int rc;
 
-	if (unlikely((ia->ia_size == i_size))) {
+	if (unlikely(lower_ia->ia_size == i_size)) {
 		lower_ia->ia_valid &= ~ATTR_SIZE;
 		return 0;
 	}
@@ -742,7 +740,7 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 	if (rc)
 		return rc;
 
-	if (ia->ia_size > i_size) {
+	if (lower_ia->ia_size > i_size) {
 		char zero[] = { 0x00 };
 
 		/*
@@ -751,16 +749,14 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 		 * 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);
+		rc = ecryptfs_write(inode, zero, lower_ia->ia_size - 1, 1);
 		lower_ia->ia_valid &= ~ATTR_SIZE;
 		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;
-		lower_ia->ia_valid |= ATTR_SIZE;
+		truncate_setsize(inode, lower_ia->ia_size);
 		goto out;
 	}
 
@@ -769,17 +765,17 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 	 * 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);
+	num_zeros = PAGE_SIZE - (lower_ia->ia_size & ~PAGE_MASK);
 	if (num_zeros) {
 		rc = ecryptfs_write(inode, page_address(ZERO_PAGE(0)),
-				ia->ia_size, num_zeros);
+				lower_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);
 			goto out;
 		}
 	}
-	truncate_setsize(inode, ia->ia_size);
+	truncate_setsize(inode, lower_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",
@@ -794,13 +790,12 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 	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) {
+		upper_size_to_lower_size(crypt_stat, lower_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 {
+	else
 		lower_ia->ia_valid &= ~ATTR_SIZE;
-	}
+
 out:
 	ecryptfs_put_lower_file(inode);
 	return rc;
@@ -840,15 +835,17 @@ static int ecryptfs_inode_newsize_ok(struct inode *inode, loff_t offset)
  */
 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 };
+	struct iattr lower_ia = {
+		.ia_valid	= ATTR_SIZE,
+		.ia_size	= new_length,
+	};
 	int rc;
 
 	rc = ecryptfs_inode_newsize_ok(d_inode(dentry), new_length);
 	if (rc)
 		return rc;
 
-	rc = truncate_upper(dentry, &ia, &lower_ia);
+	rc = truncate_upper(dentry, &lower_ia);
 	if (!rc && lower_ia.ia_valid & ATTR_SIZE) {
 		struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
 
@@ -943,7 +940,7 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 		if (rc)
 			goto out;
 
-		rc = truncate_upper(dentry, ia, &lower_ia);
+		rc = truncate_upper(dentry, &lower_ia);
 		if (rc < 0)
 			goto out;
 	}
-- 
2.47.3

[PATCH 6/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 | 53 +++++++++++++++------------------------------
 1 file changed, 17 insertions(+), 36 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index a7dc25fae8ee..c87ee3c6ecba 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -736,6 +736,23 @@ static int truncate_upper(struct dentry *dentry, struct iattr *lower_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, lower_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;
@@ -754,7 +771,6 @@ static int truncate_upper(struct dentry *dentry, struct iattr *lower_ia)
 		goto out;
 	}
 
-	crypt_stat = &ecryptfs_inode_to_private(d_inode(dentry))->crypt_stat;
 	if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
 		truncate_setsize(inode, lower_ia->ia_size);
 		goto out;
@@ -787,42 +803,15 @@ static int truncate_upper(struct dentry *dentry, struct iattr *lower_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, lower_ia->ia_size);
 	if (lower_size_after_truncate < lower_size_before_truncate)
 		lower_ia->ia_size = lower_size_after_truncate;
 	else
 		lower_ia->ia_valid &= ~ATTR_SIZE;
-
 out:
 	ecryptfs_put_lower_file(inode);
 	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
@@ -841,10 +830,6 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length)
 	};
 	int rc;
 
-	rc = ecryptfs_inode_newsize_ok(d_inode(dentry), new_length);
-	if (rc)
-		return rc;
-
 	rc = truncate_upper(dentry, &lower_ia);
 	if (!rc && lower_ia.ia_valid & ATTR_SIZE) {
 		struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
@@ -936,10 +921,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, &lower_ia);
 		if (rc < 0)
 			goto out;
-- 
2.47.3

[PATCH 7/7] ecryptfs: call notify_change from truncate_upper

[Christoph Hellwig]

Keep all the truncation logic in one place by also moving the call to
notify_change into truncate_upper.  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 | 61 +++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 33 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index c87ee3c6ecba..256beed0e47d 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -707,7 +707,7 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
 }
 
 /**
- * truncate_upper
+ * __ecryptfs_truncate
  * @dentry: The ecryptfs layer dentry
  * @lower_ia: Address of the lower inode's attributes
  *
@@ -721,9 +721,10 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
  *
  * Returns zero on success; non-zero otherwise
  */
-static int truncate_upper(struct dentry *dentry, struct iattr *lower_ia)
+static int __ecryptfs_truncate(struct dentry *dentry, struct iattr *lower_ia)
 {
 	struct inode *inode = d_inode(dentry);
+	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
 	struct ecryptfs_crypt_stat *crypt_stat;
 	loff_t i_size = i_size_read(inode);
 	loff_t lower_size_before_truncate;
@@ -731,10 +732,8 @@ static int truncate_upper(struct dentry *dentry, struct iattr *lower_ia)
 	size_t num_zeros;
 	int rc;
 
-	if (unlikely(lower_ia->ia_size == i_size)) {
-		lower_ia->ia_valid &= ~ATTR_SIZE;
+	if (unlikely(lower_ia->ia_size == i_size))
 		return 0;
-	}
 
 	crypt_stat = &ecryptfs_inode_to_private(inode)->crypt_stat;
 	lower_size_before_truncate =
@@ -767,13 +766,12 @@ static int truncate_upper(struct dentry *dentry, struct iattr *lower_ia)
 		 * new and of the file.
 		 */
 		rc = ecryptfs_write(inode, zero, lower_ia->ia_size - 1, 1);
-		lower_ia->ia_valid &= ~ATTR_SIZE;
 		goto out;
 	}
 
 	if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
 		truncate_setsize(inode, lower_ia->ia_size);
-		goto out;
+		goto set_size;
 	}
 
 	/*
@@ -803,10 +801,14 @@ static int truncate_upper(struct dentry *dentry, struct iattr *lower_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;
-	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:
+	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;
@@ -828,18 +830,8 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length)
 		.ia_valid	= ATTR_SIZE,
 		.ia_size	= new_length,
 	};
-	int rc;
-
-	rc = truncate_upper(dentry, &lower_ia);
-	if (!rc && lower_ia.ia_valid & ATTR_SIZE) {
-		struct dentry *lower_dentry = ecryptfs_dentry_to_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));
-	}
-	return rc;
+	return __ecryptfs_truncate(dentry, &lower_ia);
 }
 
 static int
@@ -867,7 +859,6 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 			    struct dentry *dentry, struct iattr *ia)
 {
 	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;
@@ -918,13 +909,6 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 		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 = truncate_upper(dentry, &lower_ia);
-		if (rc < 0)
-			goto out;
-	}
 
 	/*
 	 * mode change is for clearing setuid/setgid bits. Allow lower fs
@@ -933,9 +917,20 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 	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);
-	inode_unlock(d_inode(lower_dentry));
+	if (ia->ia_valid & ATTR_SIZE) {
+		if (ia->ia_valid & ATTR_FILE)
+			lower_ia.ia_file = ecryptfs_file_to_lower(ia->ia_file);
+		rc = __ecryptfs_truncate(dentry, &lower_ia);
+		if (rc < 0)
+			goto out;
+	} else {
+		struct dentry *lower_dentry = ecryptfs_dentry_to_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 1/7] ecryptfs: streamline truncate_upper

[Tyler Hicks]

On 2026-03-31 17:37:22, 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

I think that first sentence was supposed to say, "Use a few strategic
gotos to reduce indentation and keep the main flow outside of branches."
Could you confirm?

> 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 8ab014db3e03..cf20873a9cc4 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 and of the file.

Since we're touching this comment, we might as well fix the typo:

 s/new and of the file/new end of the file/

I can do both of these small changes when merging, if we don't have a
better reason to respin this series.

Tyler

> +		 */
> +		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: [PATCH 2/7] ecryptfs: cleanup ecryptfs_setattr

[Tyler Hicks]

On 2026-03-31 17:37:23, Christoph Hellwig wrote:
> Initialize variables at declaration time where applicable and reformat
> conditionals to match the kernel coding style.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This one looks good to me.

Tyler

> ---
>  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 cf20873a9cc4..46dc867a8860 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -891,25 +891,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(
> @@ -922,8 +920,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
> 

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

[Tyler Hicks]

On 2026-03-31 17:37:24, Christoph Hellwig wrote:
> Use the existing pre-zeroed memory instead of allocating a new chunk.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Nice improvement!

Tyler

> ---
>  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 46dc867a8860..57df35a22e9c 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
> 

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

[Tyler Hicks]

On 2026-03-31 17:37:25, Christoph Hellwig wrote:
> 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>

This one looks good to me, as well.

Tyler

> ---
>  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 57df35a22e9c..7a3da72eb3c6 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
> 

Re: [PATCH 5/7] ecryptfs: sanitize struct iattr handling in truncate_upper

[Tyler Hicks]

On 2026-03-31 17:37:26, Christoph Hellwig wrote:
> 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.
> 
> Switch to only passing a lower ia which must have ia_size set to the
> expected lower size, which cleans up the logic in truncate_upper and
> ecryptfs_truncate.

This one isn't making sense to me. It is shoving the upper inode size
into the lower_ia->ia_size, which are two different values for encrypted
files. I find that it makes truncate_upper() more confusing to read.

I'm wondering if the following function signature would make more sense
so that we can make better sense of which inode size we're talking about:

 static int truncate_upper(struct dentry *dentry, size_t upper_size,
			   struct iattr *lower_ia)

Tyler

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/ecryptfs/inode.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 7a3da72eb3c6..a7dc25fae8ee 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -709,7 +709,6 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
>  /**
>   * truncate_upper
>   * @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
> @@ -722,8 +721,7 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
>   *
>   * Returns zero on success; non-zero otherwise
>   */
> -static int truncate_upper(struct dentry *dentry, struct iattr *ia,
> -			  struct iattr *lower_ia)
> +static int truncate_upper(struct dentry *dentry, struct iattr *lower_ia)
>  {
>  	struct inode *inode = d_inode(dentry);
>  	struct ecryptfs_crypt_stat *crypt_stat;
> @@ -733,7 +731,7 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
>  	size_t num_zeros;
>  	int rc;
>  
> -	if (unlikely((ia->ia_size == i_size))) {
> +	if (unlikely(lower_ia->ia_size == i_size)) {
>  		lower_ia->ia_valid &= ~ATTR_SIZE;
>  		return 0;
>  	}
> @@ -742,7 +740,7 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
>  	if (rc)
>  		return rc;
>  
> -	if (ia->ia_size > i_size) {
> +	if (lower_ia->ia_size > i_size) {
>  		char zero[] = { 0x00 };
>  
>  		/*
> @@ -751,16 +749,14 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
>  		 * 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);
> +		rc = ecryptfs_write(inode, zero, lower_ia->ia_size - 1, 1);
>  		lower_ia->ia_valid &= ~ATTR_SIZE;
>  		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;
> -		lower_ia->ia_valid |= ATTR_SIZE;
> +		truncate_setsize(inode, lower_ia->ia_size);
>  		goto out;
>  	}
>  
> @@ -769,17 +765,17 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
>  	 * 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);
> +	num_zeros = PAGE_SIZE - (lower_ia->ia_size & ~PAGE_MASK);
>  	if (num_zeros) {
>  		rc = ecryptfs_write(inode, page_address(ZERO_PAGE(0)),
> -				ia->ia_size, num_zeros);
> +				lower_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);
>  			goto out;
>  		}
>  	}
> -	truncate_setsize(inode, ia->ia_size);
> +	truncate_setsize(inode, lower_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",
> @@ -794,13 +790,12 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
>  	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) {
> +		upper_size_to_lower_size(crypt_stat, lower_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 {
> +	else
>  		lower_ia->ia_valid &= ~ATTR_SIZE;
> -	}
> +
>  out:
>  	ecryptfs_put_lower_file(inode);
>  	return rc;
> @@ -840,15 +835,17 @@ static int ecryptfs_inode_newsize_ok(struct inode *inode, loff_t offset)
>   */
>  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 };
> +	struct iattr lower_ia = {
> +		.ia_valid	= ATTR_SIZE,
> +		.ia_size	= new_length,
> +	};
>  	int rc;
>  
>  	rc = ecryptfs_inode_newsize_ok(d_inode(dentry), new_length);
>  	if (rc)
>  		return rc;
>  
> -	rc = truncate_upper(dentry, &ia, &lower_ia);
> +	rc = truncate_upper(dentry, &lower_ia);
>  	if (!rc && lower_ia.ia_valid & ATTR_SIZE) {
>  		struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
>  
> @@ -943,7 +940,7 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
>  		if (rc)
>  			goto out;
>  
> -		rc = truncate_upper(dentry, ia, &lower_ia);
> +		rc = truncate_upper(dentry, &lower_ia);
>  		if (rc < 0)
>  			goto out;
>  	}
> -- 
> 2.47.3
> 

Re: [PATCH 6/7] ecryptfs: merge ecryptfs_inode_newsize_ok into truncate_upper

[Tyler Hicks]

On 2026-03-31 17:37:27, Christoph Hellwig wrote:
> 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>

This one looks good to me.

Tyler

> ---
>  fs/ecryptfs/inode.c | 53 +++++++++++++++------------------------------
>  1 file changed, 17 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index a7dc25fae8ee..c87ee3c6ecba 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -736,6 +736,23 @@ static int truncate_upper(struct dentry *dentry, struct iattr *lower_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, lower_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;
> @@ -754,7 +771,6 @@ static int truncate_upper(struct dentry *dentry, struct iattr *lower_ia)
>  		goto out;
>  	}
>  
> -	crypt_stat = &ecryptfs_inode_to_private(d_inode(dentry))->crypt_stat;
>  	if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
>  		truncate_setsize(inode, lower_ia->ia_size);
>  		goto out;
> @@ -787,42 +803,15 @@ static int truncate_upper(struct dentry *dentry, struct iattr *lower_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, lower_ia->ia_size);
>  	if (lower_size_after_truncate < lower_size_before_truncate)
>  		lower_ia->ia_size = lower_size_after_truncate;
>  	else
>  		lower_ia->ia_valid &= ~ATTR_SIZE;
> -
>  out:
>  	ecryptfs_put_lower_file(inode);
>  	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
> @@ -841,10 +830,6 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length)
>  	};
>  	int rc;
>  
> -	rc = ecryptfs_inode_newsize_ok(d_inode(dentry), new_length);
> -	if (rc)
> -		return rc;
> -
>  	rc = truncate_upper(dentry, &lower_ia);
>  	if (!rc && lower_ia.ia_valid & ATTR_SIZE) {
>  		struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
> @@ -936,10 +921,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, &lower_ia);
>  		if (rc < 0)
>  			goto out;
> -- 
> 2.47.3
> 

Re: [PATCH 5/7] ecryptfs: sanitize struct iattr handling in truncate_upper

[Tyler Hicks]

On 2026-04-06 00:58:23, Tyler Hicks wrote:
> On 2026-03-31 17:37:26, Christoph Hellwig wrote:
> > 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.
> > 
> > Switch to only passing a lower ia which must have ia_size set to the
> > expected lower size, which cleans up the logic in truncate_upper and
> > ecryptfs_truncate.
> 
> This one isn't making sense to me. It is shoving the upper inode size
> into the lower_ia->ia_size, which are two different values for encrypted
> files. I find that it makes truncate_upper() more confusing to read.
> 
> I'm wondering if the following function signature would make more sense
> so that we can make better sense of which inode size we're talking about:
> 
>  static int truncate_upper(struct dentry *dentry, size_t upper_size,
> 			   struct iattr *lower_ia)

Err... that was a bad suggestion. upper_size should be a loff_t but, now
that I'm at the end of the patch series review, I see that's essentially
the signature of ecryptfs_truncate() but I'm still not understanding why
we're sticking the upper inode size into the lower_ia.

Tyler

> 
> Tyler
> 
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/ecryptfs/inode.c | 39 ++++++++++++++++++---------------------
> >  1 file changed, 18 insertions(+), 21 deletions(-)
> > 
> > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> > index 7a3da72eb3c6..a7dc25fae8ee 100644
> > --- a/fs/ecryptfs/inode.c
> > +++ b/fs/ecryptfs/inode.c
> > @@ -709,7 +709,6 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
> >  /**
> >   * truncate_upper
> >   * @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
> > @@ -722,8 +721,7 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
> >   *
> >   * Returns zero on success; non-zero otherwise
> >   */
> > -static int truncate_upper(struct dentry *dentry, struct iattr *ia,
> > -			  struct iattr *lower_ia)
> > +static int truncate_upper(struct dentry *dentry, struct iattr *lower_ia)
> >  {
> >  	struct inode *inode = d_inode(dentry);
> >  	struct ecryptfs_crypt_stat *crypt_stat;
> > @@ -733,7 +731,7 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
> >  	size_t num_zeros;
> >  	int rc;
> >  
> > -	if (unlikely((ia->ia_size == i_size))) {
> > +	if (unlikely(lower_ia->ia_size == i_size)) {
> >  		lower_ia->ia_valid &= ~ATTR_SIZE;
> >  		return 0;
> >  	}
> > @@ -742,7 +740,7 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
> >  	if (rc)
> >  		return rc;
> >  
> > -	if (ia->ia_size > i_size) {
> > +	if (lower_ia->ia_size > i_size) {
> >  		char zero[] = { 0x00 };
> >  
> >  		/*
> > @@ -751,16 +749,14 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
> >  		 * 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);
> > +		rc = ecryptfs_write(inode, zero, lower_ia->ia_size - 1, 1);
> >  		lower_ia->ia_valid &= ~ATTR_SIZE;
> >  		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;
> > -		lower_ia->ia_valid |= ATTR_SIZE;
> > +		truncate_setsize(inode, lower_ia->ia_size);
> >  		goto out;
> >  	}
> >  
> > @@ -769,17 +765,17 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
> >  	 * 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);
> > +	num_zeros = PAGE_SIZE - (lower_ia->ia_size & ~PAGE_MASK);
> >  	if (num_zeros) {
> >  		rc = ecryptfs_write(inode, page_address(ZERO_PAGE(0)),
> > -				ia->ia_size, num_zeros);
> > +				lower_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);
> >  			goto out;
> >  		}
> >  	}
> > -	truncate_setsize(inode, ia->ia_size);
> > +	truncate_setsize(inode, lower_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",
> > @@ -794,13 +790,12 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
> >  	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) {
> > +		upper_size_to_lower_size(crypt_stat, lower_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 {
> > +	else
> >  		lower_ia->ia_valid &= ~ATTR_SIZE;
> > -	}
> > +
> >  out:
> >  	ecryptfs_put_lower_file(inode);
> >  	return rc;
> > @@ -840,15 +835,17 @@ static int ecryptfs_inode_newsize_ok(struct inode *inode, loff_t offset)
> >   */
> >  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 };
> > +	struct iattr lower_ia = {
> > +		.ia_valid	= ATTR_SIZE,
> > +		.ia_size	= new_length,
> > +	};
> >  	int rc;
> >  
> >  	rc = ecryptfs_inode_newsize_ok(d_inode(dentry), new_length);
> >  	if (rc)
> >  		return rc;
> >  
> > -	rc = truncate_upper(dentry, &ia, &lower_ia);
> > +	rc = truncate_upper(dentry, &lower_ia);
> >  	if (!rc && lower_ia.ia_valid & ATTR_SIZE) {
> >  		struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
> >  
> > @@ -943,7 +940,7 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
> >  		if (rc)
> >  			goto out;
> >  
> > -		rc = truncate_upper(dentry, ia, &lower_ia);
> > +		rc = truncate_upper(dentry, &lower_ia);
> >  		if (rc < 0)
> >  			goto out;
> >  	}
> > -- 
> > 2.47.3
> > 
> 

Re: [PATCH 5/7] ecryptfs: sanitize struct iattr handling in truncate_upper

[Christoph Hellwig]

On Mon, Apr 06, 2026 at 01:22:53AM -0500, Tyler Hicks wrote:
> Err... that was a bad suggestion. upper_size should be a loff_t but, now
> that I'm at the end of the patch series review, I see that's essentially
> the signature of ecryptfs_truncate() but I'm still not understanding why
> we're sticking the upper inode size into the lower_ia.

Because that's what the existing setattr path already does by doing a
memcpy of the upper ia.  We can explicitly pass it as a separate
argument, but I'm not really sure that clarifies things as all other
information gets passed in lower_ia.  I could add a big comment explaining
all this if it helps?  It took me some time to figure out, so I might as
well share that with the world.

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

[Christoph Hellwig]

On Mon, Apr 06, 2026 at 12:52:05AM -0500, Tyler Hicks wrote:
> On 2026-03-31 17:37:22, 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
> 
> I think that first sentence was supposed to say, "Use a few strategic
> gotos to reduce indentation and keep the main flow outside of branches."
> Could you confirm?

Yes.

> > +		/*
> > +		 * 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.
> 
> Since we're touching this comment, we might as well fix the typo:
> 
>  s/new and of the file/new end of the file/
> 
> I can do both of these small changes when merging, if we don't have a
> better reason to respin this series.

I'll fix it up when I resend, and it looks like we're need another version
anyway.

Re: [PATCH 7/7] ecryptfs: call notify_change from truncate_upper

[Tyler Hicks]

On 2026-03-31 17:37:28, Christoph Hellwig wrote:
> Keep all the truncation logic in one place by also moving the call to
> notify_change into truncate_upper.  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 | 61 +++++++++++++++++++++------------------------
>  1 file changed, 28 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index c87ee3c6ecba..256beed0e47d 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -707,7 +707,7 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
>  }
>  
>  /**
> - * truncate_upper
> + * __ecryptfs_truncate
>   * @dentry: The ecryptfs layer dentry
>   * @lower_ia: Address of the lower inode's attributes
>   *
> @@ -721,9 +721,10 @@ upper_size_to_lower_size(struct ecryptfs_crypt_stat *crypt_stat,
>   *
>   * Returns zero on success; non-zero otherwise
>   */
> -static int truncate_upper(struct dentry *dentry, struct iattr *lower_ia)
> +static int __ecryptfs_truncate(struct dentry *dentry, struct iattr *lower_ia)

Could you update the function documentation, as well? This sentence is
no longer true:

 * ... 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.                         

The rest looks good.

Tyler

>  {
>  	struct inode *inode = d_inode(dentry);
> +	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
>  	struct ecryptfs_crypt_stat *crypt_stat;
>  	loff_t i_size = i_size_read(inode);
>  	loff_t lower_size_before_truncate;
> @@ -731,10 +732,8 @@ static int truncate_upper(struct dentry *dentry, struct iattr *lower_ia)
>  	size_t num_zeros;
>  	int rc;
>  
> -	if (unlikely(lower_ia->ia_size == i_size)) {
> -		lower_ia->ia_valid &= ~ATTR_SIZE;
> +	if (unlikely(lower_ia->ia_size == i_size))
>  		return 0;
> -	}
>  
>  	crypt_stat = &ecryptfs_inode_to_private(inode)->crypt_stat;
>  	lower_size_before_truncate =
> @@ -767,13 +766,12 @@ static int truncate_upper(struct dentry *dentry, struct iattr *lower_ia)
>  		 * new and of the file.
>  		 */
>  		rc = ecryptfs_write(inode, zero, lower_ia->ia_size - 1, 1);
> -		lower_ia->ia_valid &= ~ATTR_SIZE;
>  		goto out;
>  	}
>  
>  	if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
>  		truncate_setsize(inode, lower_ia->ia_size);
> -		goto out;
> +		goto set_size;
>  	}
>  
>  	/*
> @@ -803,10 +801,14 @@ static int truncate_upper(struct dentry *dentry, struct iattr *lower_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;
> -	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:
> +	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;
> @@ -828,18 +830,8 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length)
>  		.ia_valid	= ATTR_SIZE,
>  		.ia_size	= new_length,
>  	};
> -	int rc;
> -
> -	rc = truncate_upper(dentry, &lower_ia);
> -	if (!rc && lower_ia.ia_valid & ATTR_SIZE) {
> -		struct dentry *lower_dentry = ecryptfs_dentry_to_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));
> -	}
> -	return rc;
> +	return __ecryptfs_truncate(dentry, &lower_ia);
>  }
>  
>  static int
> @@ -867,7 +859,6 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
>  			    struct dentry *dentry, struct iattr *ia)
>  {
>  	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;
> @@ -918,13 +909,6 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
>  		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 = truncate_upper(dentry, &lower_ia);
> -		if (rc < 0)
> -			goto out;
> -	}
>  
>  	/*
>  	 * mode change is for clearing setuid/setgid bits. Allow lower fs
> @@ -933,9 +917,20 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
>  	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);
> -	inode_unlock(d_inode(lower_dentry));
> +	if (ia->ia_valid & ATTR_SIZE) {
> +		if (ia->ia_valid & ATTR_FILE)
> +			lower_ia.ia_file = ecryptfs_file_to_lower(ia->ia_file);
> +		rc = __ecryptfs_truncate(dentry, &lower_ia);
> +		if (rc < 0)
> +			goto out;
> +	} else {
> +		struct dentry *lower_dentry = ecryptfs_dentry_to_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 5/7] ecryptfs: sanitize struct iattr handling in truncate_upper

[Tyler Hicks]

On 2026-04-06 08:27:13, Christoph Hellwig wrote:
> On Mon, Apr 06, 2026 at 01:22:53AM -0500, Tyler Hicks wrote:
> > Err... that was a bad suggestion. upper_size should be a loff_t but, now
> > that I'm at the end of the patch series review, I see that's essentially
> > the signature of ecryptfs_truncate() but I'm still not understanding why
> > we're sticking the upper inode size into the lower_ia.
> 
> Because that's what the existing setattr path already does by doing a
> memcpy of the upper ia.  We can explicitly pass it as a separate
> argument, but I'm not really sure that clarifies things as all other
> information gets passed in lower_ia.  I could add a big comment explaining
> all this if it helps?  It took me some time to figure out, so I might as
> well share that with the world.

I think the intent of the current code is to copy all of the existing
iattrs, from upper to lower, but treat lower_ia->ia_size as
"uninitialized" and calculate/initialize it accordingly within
truncate_upper(). I don't think the passed in value of lower_ia->ia_size
was being used anywhere.

I would prefer passing the new upper size as a separate argument.

The end result of this patch series is very nice. Thanks for working on
it!

Tyler

[PATCH 2/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 69a5dfaddc5c..695573850569 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -891,25 +891,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(
@@ -922,8 +920,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