Re: [PATCH 1/2] ecryptfs: Improve metatdata read failure logging

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 01/12/2012 01:00 PM, Tyler Hicks wrote:
On 2012-01-11 18:00:41, Tim Gardner wrote:
There are 3 read failure cases in ecryptfs_read_metadata(), but only 2
of them are uniquely noted by kernel log messages. This patch
identifies and logs each read failure case. It also correctly interprets
a negative return value from ecryptfs_read_lower().

I'm not sure that the negative return value was incorrectly interpreted.
See below.


Removes unnecessary variable initialization.

Cc: linux-kernel@xxxxxxxxxxxxxxx
Cc: stable@xxxxxxxxxxxxxxx
Cc: Tyler Hicks<tyler.hicks@xxxxxxxxxxxxx>
Signed-off-by: Tim Gardner<tim.gardner@xxxxxxxxxxxxx>
---
  fs/ecryptfs/crypto.c |   17 +++++++++++------
  1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index d3c8776..ac063bd 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -1590,8 +1590,8 @@ int ecryptfs_read_and_validate_xattr_region(struct dentry *dentry,
   */
  int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry)
  {
-	int rc = 0;
-	char *page_virt = NULL;
+	int rc;
+	char *page_virt;
  	struct inode *ecryptfs_inode = ecryptfs_dentry->d_inode;
  	struct ecryptfs_crypt_stat *crypt_stat =
  	&ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat;
@@ -1611,10 +1611,15 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry)
  	}
  	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<  0) {
+		printk(KERN_ERR "%s: Could not read %u bytes\n",
+			__func__, crypt_stat->extent_size);
+		goto out;
+	}

This may break things a bit for users that use the
ecryptfs_xattr_metadata mount option (which stores the metadata in an
xattr, rather than the header of the file). A failure to read the lower
file here doesn't matter because the metadata is likely stored in an
xattr, which will be found with the ecryptfs_read_xattr_region() call
below.

I've never liked that ecryptfs potentially looks in both the header and
the xattr for metadata, but that's how it has been since the very early
days of eCryptfs. I've wanted to change this but there is the potential
to break things for users who have a mixture of files with metadata in
the header and in the header.

Maybe we just go whole hog for 3.3 and only look in either the header or
xattr for metadata, depending on whether or not ecryptfs_xattr_metadata
is used?

I could live with that, but I would definitely not want something like
that to go to the stable kernel. Any thoughts?

Tyler


I think you're right. Given the way the metadata checks are coded I think its impossible to disambiguate the 2 failure scenarios. However, its not really relevant that we know which one failed, only the inode of the metadata read that _did_ fail is important.

How about this much simpler patch that is also suitable for stable ? Tested on 3.2.

rtg
--
Tim Gardner tim.gardner@xxxxxxxxxxxxx
>From 48f75c409ddc00caec88162f53c3155196b17854 Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@xxxxxxxxxxxxx>
Date: Thu, 12 Jan 2012 16:31:55 +0100
Subject: [PATCH 1/1 V2] ecryptfs: Improve metadata read failure logging

Print inode on metadata read failure. The only real
way of dealing with metadata read failures is to delete
the underlying file system file. Having the inode
allows one to 'find . -inum INODE`.

Cc: stable@xxxxxxxxxxxxxxx
Cc: Tyler Hicks <tyler.hicks@xxxxxxxxxxxxx>
Signed-off-by: Tim Gardner <tim.gardner@xxxxxxxxxxxxx>
---
 fs/ecryptfs/crypto.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index d3c8776..326d6ab 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -1590,8 +1590,8 @@ int ecryptfs_read_and_validate_xattr_region(struct dentry *dentry,
  */
 int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry)
 {
-	int rc = 0;
-	char *page_virt = NULL;
+	int rc;
+	char *page_virt;
 	struct inode *ecryptfs_inode = ecryptfs_dentry->d_inode;
 	struct ecryptfs_crypt_stat *crypt_stat =
 	    &ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat;
@@ -1616,11 +1616,13 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry)
 						ecryptfs_dentry,
 						ECRYPTFS_VALIDATE_HEADER_SIZE);
 	if (rc) {
+		/* metadata is not in the file header, so try xattrs */
 		memset(page_virt, 0, PAGE_CACHE_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\n");
+			       "file header region or xattr region, inode %lu\n",
+				ecryptfs_inode->i_ino);
 			rc = -EINVAL;
 			goto out;
 		}
@@ -1629,7 +1631,8 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry)
 						ECRYPTFS_DONT_VALIDATE_HEADER_SIZE);
 		if (rc) {
 			printk(KERN_DEBUG "Valid eCryptfs headers not found in "
-			       "file xattr region either\n");
+			       "file xattr region either, inode %lu\n",
+				ecryptfs_inode->i_ino);
 			rc = -EINVAL;
 		}
 		if (crypt_stat->mount_crypt_stat->flags
@@ -1640,7 +1643,8 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry)
 			       "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.\n");
+			       "this like an encrypted file, inode %lu\n",
+				ecryptfs_inode->i_ino);
 			rc = -EINVAL;
 		}
 	}
-- 
1.7.8.3


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux