Re: [PATCH] ecryptfs: Add missing gotos in ecryptfs_read_metadata

[Al Viro <viro@zeniv.linux.org.uk>]

On Sun, Jan 11, 2026 at 01:36:52AM +0100, Thorsten Blum wrote:
> Add two missing goto statements to exit ecryptfs_read_metadata() when an
> error occurs.
> 
> The first goto is required; otherwise ECRYPTFS_METADATA_IN_XATTR may be
> set when xattr metadata is enabled even though parsing the metadata
> failed. The second goto is not strictly necessary, but it makes the
> error path explicit instead of relying on falling through to 'out'.

Ugh...  IMO the whole thing from the point we'd successfully allocated
the page to the point where we start to clear it ought to be in a separate
helper.  Something like this, perhaps?

diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index 260f8a4938b0..53fec5a3acaf 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -1272,6 +1272,43 @@ int ecryptfs_read_and_validate_xattr_region(struct dentry *dentry,
 	return rc;
 }
 
+static int do_read_metadata(struct dentry *dentry, char *page,
+			    struct ecryptfs_crypt_stat *crypt_stat)
+{
+	struct inode *inode = d_inode(dentry);
+
+	/* try to get it from file header */
+	if (ecryptfs_read_lower(page, 0, crypt_stat->extent_size, inode) >= 0 &&
+	    ecryptfs_read_headers_virt(page, crypt_stat, dentry,
+				       ECRYPTFS_VALIDATE_HEADER_SIZE) == 0)
+		return 0;
+
+	/* metadata is not in the file header, so try xattrs */
+	memset(page, 0, PAGE_SIZE);
+	if (ecryptfs_read_xattr_region(page, inode) < 0 ||
+	    ecryptfs_read_headers_virt(page, crypt_stat, dentry,
+				       ECRYPTFS_DONT_VALIDATE_HEADER_SIZE) != 0) {
+		printk(KERN_DEBUG "Valid eCryptfs headers not found in "
+		       "file xattr region either, inode %lu\n", inode->i_ino);
+		return -EINVAL;
+	}
+
+	/* OK, it's in xattrs; are we allowed to use that? */
+	if (crypt_stat->mount_crypt_stat->flags
+	    & ECRYPTFS_XATTR_METADATA_ENABLED) {
+		crypt_stat->flags |= ECRYPTFS_METADATA_IN_XATTR;
+		return 0;
+	}
+
+	printk(KERN_WARNING "Attempt to access file with "
+	       "crypto metadata only in the extended attribute "
+	       "region, but eCryptfs was mounted without "
+	       "xattr support enabled. eCryptfs will not treat "
+	       "this like an encrypted file, inode %lu\n",
+		inode->i_ino);
+	return -EINVAL;
+}
+
 /*
  * ecryptfs_read_metadata
  *
@@ -1299,54 +1336,14 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry)
 						      mount_crypt_stat);
 	/* Read the first page from the underlying file */
 	page_virt = kmem_cache_alloc(ecryptfs_header_cache, GFP_USER);
-	if (!page_virt) {
-		rc = -ENOMEM;
-		goto out;
-	}
-	rc = ecryptfs_read_lower(page_virt, 0, crypt_stat->extent_size,
-				 ecryptfs_inode);
-	if (rc >= 0)
-		rc = ecryptfs_read_headers_virt(page_virt, crypt_stat,
-						ecryptfs_dentry,
-						ECRYPTFS_VALIDATE_HEADER_SIZE);
-	if (rc) {
-		/* metadata is not in the file header, so try xattrs */
-		memset(page_virt, 0, PAGE_SIZE);
-		rc = ecryptfs_read_xattr_region(page_virt, ecryptfs_inode);
-		if (rc) {
-			printk(KERN_DEBUG "Valid eCryptfs headers not found in "
-			       "file header region or xattr region, inode %lu\n",
-				ecryptfs_inode->i_ino);
-			rc = -EINVAL;
-			goto out;
-		}
-		rc = ecryptfs_read_headers_virt(page_virt, crypt_stat,
-						ecryptfs_dentry,
-						ECRYPTFS_DONT_VALIDATE_HEADER_SIZE);
-		if (rc) {
-			printk(KERN_DEBUG "Valid eCryptfs headers not found in "
-			       "file xattr region either, inode %lu\n",
-				ecryptfs_inode->i_ino);
-			rc = -EINVAL;
-		}
-		if (crypt_stat->mount_crypt_stat->flags
-		    & ECRYPTFS_XATTR_METADATA_ENABLED) {
-			crypt_stat->flags |= ECRYPTFS_METADATA_IN_XATTR;
-		} else {
-			printk(KERN_WARNING "Attempt to access file with "
-			       "crypto metadata only in the extended attribute "
-			       "region, but eCryptfs was mounted without "
-			       "xattr support enabled. eCryptfs will not treat "
-			       "this like an encrypted file, inode %lu\n",
-				ecryptfs_inode->i_ino);
-			rc = -EINVAL;
-		}
-	}
-out:
-	if (page_virt) {
-		memset(page_virt, 0, PAGE_SIZE);
-		kmem_cache_free(ecryptfs_header_cache, page_virt);
-	}
+	if (!page_virt)
+		return -ENOMEM;
+
+	rc = do_read_metadata(ecryptfs_dentry, page_virt, crypt_stat);
+
+	memset(page_virt, 0, PAGE_SIZE);
+	kmem_cache_free(ecryptfs_header_cache, page_virt);
+
 	return rc;
 }