* [PATCH 0/2] ecryptfs: Improved metadata read failure logging @ 2012-01-11 17:00 Tim Gardner 2012-01-11 17:00 ` [PATCH 1/2] ecryptfs: Improve metatdata " Tim Gardner 2012-01-11 17:00 ` [PATCH 2/2] ecryptfs: Print inode on metadata error Tim Gardner 0 siblings, 2 replies; 11+ messages in thread From: Tim Gardner @ 2012-01-11 17:00 UTC (permalink / raw) To: linux-fsdevel, tyler.hicks This patch set improves error logging when metadata read failures are encountered. This is usually the result of lower file system corruption or from an older inode race bug where 0 length FNEK files were created. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] ecryptfs: Improve metatdata read failure logging 2012-01-11 17:00 [PATCH 0/2] ecryptfs: Improved metadata read failure logging Tim Gardner @ 2012-01-11 17:00 ` Tim Gardner 2012-01-11 20:36 ` Kees Cook 2012-01-12 12:00 ` Tyler Hicks 2012-01-11 17:00 ` [PATCH 2/2] ecryptfs: Print inode on metadata error Tim Gardner 1 sibling, 2 replies; 11+ messages in thread From: Tim Gardner @ 2012-01-11 17:00 UTC (permalink / raw) To: linux-fsdevel, tyler.hicks; +Cc: Tim Gardner, linux-kernel, stable 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(). Removes unnecessary variable initialization. Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org Cc: Tyler Hicks <tyler.hicks@canonical.com> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- 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; + } + + rc = ecryptfs_read_headers_virt(page_virt, crypt_stat, + ecryptfs_dentry, + ECRYPTFS_VALIDATE_HEADER_SIZE); if (rc) { memset(page_virt, 0, PAGE_CACHE_SIZE); rc = ecryptfs_read_xattr_region(page_virt, ecryptfs_inode); -- 1.7.8.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ecryptfs: Improve metatdata read failure logging 2012-01-11 17:00 ` [PATCH 1/2] ecryptfs: Improve metatdata " Tim Gardner @ 2012-01-11 20:36 ` Kees Cook 2012-01-12 12:00 ` Tyler Hicks 1 sibling, 0 replies; 11+ messages in thread From: Kees Cook @ 2012-01-11 20:36 UTC (permalink / raw) To: Tim Gardner; +Cc: linux-fsdevel, tyler.hicks, linux-kernel, stable On Wed, Jan 11, 2012 at 06:00:41PM +0100, 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(). > > Removes unnecessary variable initialization. > > Cc: linux-kernel@vger.kernel.org > Cc: stable@vger.kernel.org > Cc: Tyler Hicks <tyler.hicks@canonical.com> > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> I always like more verbose error reporting. :) Reviewed-by: Kees Cook <keescook@chromium.org> > + printk(KERN_ERR "%s: Could not read %u bytes\n", > + __func__, crypt_stat->extent_size); I wonder if eCryptfs in general might want to consider using the "pr_fmt" and "pr_*" macros to do printk handling: #define pr_fmt(fmt) __func__ ": " fmt ... pr_err("Could not read %u bytes\n", crypt_stat->extent_size); -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ecryptfs: Improve metatdata read failure logging 2012-01-11 17:00 ` [PATCH 1/2] ecryptfs: Improve metatdata " Tim Gardner 2012-01-11 20:36 ` Kees Cook @ 2012-01-12 12:00 ` Tyler Hicks 2012-01-12 16:45 ` Tim Gardner 1 sibling, 1 reply; 11+ messages in thread From: Tyler Hicks @ 2012-01-12 12:00 UTC (permalink / raw) To: Tim Gardner; +Cc: linux-fsdevel, linux-kernel, stable, ecryptfs [-- Attachment #1: Type: text/plain, Size: 3041 bytes --] 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@vger.kernel.org > Cc: stable@vger.kernel.org > Cc: Tyler Hicks <tyler.hicks@canonical.com> > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > 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 > + > + rc = ecryptfs_read_headers_virt(page_virt, crypt_stat, > + ecryptfs_dentry, > + ECRYPTFS_VALIDATE_HEADER_SIZE); > if (rc) { > memset(page_virt, 0, PAGE_CACHE_SIZE); > rc = ecryptfs_read_xattr_region(page_virt, ecryptfs_inode); > -- > 1.7.8.3 > [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ecryptfs: Improve metatdata read failure logging 2012-01-12 12:00 ` Tyler Hicks @ 2012-01-12 16:45 ` Tim Gardner 0 siblings, 0 replies; 11+ messages in thread From: Tim Gardner @ 2012-01-12 16:45 UTC (permalink / raw) To: Tyler Hicks; +Cc: linux-fsdevel, linux-kernel, stable, ecryptfs [-- Attachment #1: Type: text/plain, Size: 3222 bytes --] 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@vger.kernel.org >> Cc: stable@vger.kernel.org >> Cc: Tyler Hicks<tyler.hicks@canonical.com> >> Signed-off-by: Tim Gardner<tim.gardner@canonical.com> >> --- >> 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@canonical.com [-- Attachment #2: 0001-ecryptfs-Improve-metadata-read-failure-logging.patch --] [-- Type: text/x-patch, Size: 2571 bytes --] From 48f75c409ddc00caec88162f53c3155196b17854 Mon Sep 17 00:00:00 2001 From: Tim Gardner <tim.gardner@canonical.com> 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@vger.kernel.org Cc: Tyler Hicks <tyler.hicks@canonical.com> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ecryptfs: Improve metatdata read failure logging @ 2012-01-12 16:45 ` Tim Gardner 0 siblings, 0 replies; 11+ messages in thread From: Tim Gardner @ 2012-01-12 16:45 UTC (permalink / raw) To: Tyler Hicks; +Cc: linux-fsdevel, linux-kernel, stable, ecryptfs [-- Attachment #1: Type: text/plain, Size: 3222 bytes --] 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@vger.kernel.org >> Cc: stable@vger.kernel.org >> Cc: Tyler Hicks<tyler.hicks@canonical.com> >> Signed-off-by: Tim Gardner<tim.gardner@canonical.com> >> --- >> 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@canonical.com [-- Attachment #2: 0001-ecryptfs-Improve-metadata-read-failure-logging.patch --] [-- Type: text/x-patch, Size: 2572 bytes --] >From 48f75c409ddc00caec88162f53c3155196b17854 Mon Sep 17 00:00:00 2001 From: Tim Gardner <tim.gardner@canonical.com> 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@vger.kernel.org Cc: Tyler Hicks <tyler.hicks@canonical.com> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ecryptfs: Improve metatdata read failure logging 2012-01-12 16:45 ` Tim Gardner (?) @ 2012-01-20 19:50 ` Tyler Hicks 2012-01-20 20:17 ` Tim Gardner -1 siblings, 1 reply; 11+ messages in thread From: Tyler Hicks @ 2012-01-20 19:50 UTC (permalink / raw) To: tim.gardner; +Cc: linux-fsdevel, linux-kernel, stable, ecryptfs [-- Attachment #1: Type: text/plain, Size: 6757 bytes --] On 2012-01-12 17:45:04, Tim Gardner wrote: > 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@vger.kernel.org > >>Cc: stable@vger.kernel.org > >>Cc: Tyler Hicks<tyler.hicks@canonical.com> > >>Signed-off-by: Tim Gardner<tim.gardner@canonical.com> > >>--- > >> 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. Hey Tim - Sorry for the hiccup in response time. Comments below. > > rtg > -- > Tim Gardner tim.gardner@canonical.com > From 48f75c409ddc00caec88162f53c3155196b17854 Mon Sep 17 00:00:00 2001 > From: Tim Gardner <tim.gardner@canonical.com> > 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@vger.kernel.org > Cc: Tyler Hicks <tyler.hicks@canonical.com> > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > 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; I doubt there is any issue with not initializing these variables in the older, stable kernel releases, but I don't want to take the time to verify that. So, would you mind if I split this variable initialization part into a separate patch not for stable? I can handle that on my end, if that's alright with you. The printk's below are safe and stable-worthy, IMO, because end-users should really benefit from the changes. Tyler > 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 > [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ecryptfs: Improve metatdata read failure logging 2012-01-20 19:50 ` Tyler Hicks @ 2012-01-20 20:17 ` Tim Gardner 0 siblings, 0 replies; 11+ messages in thread From: Tim Gardner @ 2012-01-20 20:17 UTC (permalink / raw) To: Tyler Hicks; +Cc: linux-fsdevel, linux-kernel, stable, ecryptfs On 01/20/2012 12:50 PM, Tyler Hicks wrote: > On 2012-01-12 17:45:04, Tim Gardner wrote: >> 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@vger.kernel.org >>>> Cc: stable@vger.kernel.org >>>> Cc: Tyler Hicks<tyler.hicks@canonical.com> >>>> Signed-off-by: Tim Gardner<tim.gardner@canonical.com> >>>> --- >>>> 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. > > Hey Tim - Sorry for the hiccup in response time. Comments below. > >> >> rtg >> -- >> Tim Gardner tim.gardner@canonical.com > >> From 48f75c409ddc00caec88162f53c3155196b17854 Mon Sep 17 00:00:00 2001 >> From: Tim Gardner<tim.gardner@canonical.com> >> 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@vger.kernel.org >> Cc: Tyler Hicks<tyler.hicks@canonical.com> >> Signed-off-by: Tim Gardner<tim.gardner@canonical.com> >> --- >> 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; > > I doubt there is any issue with not initializing these variables in > the older, stable kernel releases, but I don't want to take the time to > verify that. > > So, would you mind if I split this variable initialization part into a > separate patch not for stable? I can handle that on my end, if that's > alright with you. > > The printk's below are safe and stable-worthy, IMO, because end-users > should really benefit from the changes. > > Tyler > I'm fine with that. rtg -- Tim Gardner tim.gardner@canonical.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] ecryptfs: Print inode on metadata error 2012-01-11 17:00 [PATCH 0/2] ecryptfs: Improved metadata read failure logging Tim Gardner 2012-01-11 17:00 ` [PATCH 1/2] ecryptfs: Improve metatdata " Tim Gardner @ 2012-01-11 17:00 ` Tim Gardner 2012-01-11 20:38 ` Kees Cook 2012-01-12 12:02 ` Tyler Hicks 1 sibling, 2 replies; 11+ messages in thread From: Tim Gardner @ 2012-01-11 17:00 UTC (permalink / raw) To: linux-fsdevel, tyler.hicks; +Cc: Tim Gardner, linux-kernel, stable If a lower file system file is corrupted, then ecryptfs prints a generic and mostly useless log message when attempting to read metadata. Print the inode associated with the file so that a user can do something about it, e.g., find -inum INODE_NUM Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org Cc: Tyler Hicks <tyler.hicks@canonical.com> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- 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 ac063bd..affad0c 100644 --- a/fs/ecryptfs/crypto.c +++ b/fs/ecryptfs/crypto.c @@ -1612,8 +1612,9 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry) rc = ecryptfs_read_lower(page_virt, 0, crypt_stat->extent_size, ecryptfs_inode); if (rc < 0) { - printk(KERN_ERR "%s: Could not read %u bytes\n", - __func__, crypt_stat->extent_size); + printk(KERN_ERR "%s: Could not read %zu bytes, inode %lu\n", + __func__, crypt_stat->extent_size, + ecryptfs_inode->i_ino); goto out; } @@ -1625,7 +1626,8 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry) 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; } @@ -1634,7 +1636,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 @@ -1645,7 +1648,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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] ecryptfs: Print inode on metadata error 2012-01-11 17:00 ` [PATCH 2/2] ecryptfs: Print inode on metadata error Tim Gardner @ 2012-01-11 20:38 ` Kees Cook 2012-01-12 12:02 ` Tyler Hicks 1 sibling, 0 replies; 11+ messages in thread From: Kees Cook @ 2012-01-11 20:38 UTC (permalink / raw) To: Tim Gardner; +Cc: linux-fsdevel, tyler.hicks, linux-kernel On Wed, Jan 11, 2012 at 06:00:42PM +0100, Tim Gardner wrote: > If a lower file system file is corrupted, then ecryptfs prints > a generic and mostly useless log message when attempting to > read metadata. Print the inode associated with the file > so that a user can do something about it, e.g., > > find -inum INODE_NUM > > Cc: linux-kernel@vger.kernel.org > Cc: stable@vger.kernel.org > Cc: Tyler Hicks <tyler.hicks@canonical.com> > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> Reviewed-by: Kees Cook <keescook@chromium.org> -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] ecryptfs: Print inode on metadata error 2012-01-11 17:00 ` [PATCH 2/2] ecryptfs: Print inode on metadata error Tim Gardner 2012-01-11 20:38 ` Kees Cook @ 2012-01-12 12:02 ` Tyler Hicks 1 sibling, 0 replies; 11+ messages in thread From: Tyler Hicks @ 2012-01-12 12:02 UTC (permalink / raw) To: Tim Gardner; +Cc: linux-fsdevel, linux-kernel, stable [-- Attachment #1: Type: text/plain, Size: 2593 bytes --] On 2012-01-11 18:00:42, Tim Gardner wrote: > If a lower file system file is corrupted, then ecryptfs prints > a generic and mostly useless log message when attempting to > read metadata. Print the inode associated with the file > so that a user can do something about it, e.g., > > find -inum INODE_NUM > > Cc: linux-kernel@vger.kernel.org > Cc: stable@vger.kernel.org > Cc: Tyler Hicks <tyler.hicks@canonical.com> > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- This looks good to me. I'll get it applied. Thanks! Tyler > 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 ac063bd..affad0c 100644 > --- a/fs/ecryptfs/crypto.c > +++ b/fs/ecryptfs/crypto.c > @@ -1612,8 +1612,9 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry) > rc = ecryptfs_read_lower(page_virt, 0, crypt_stat->extent_size, > ecryptfs_inode); > if (rc < 0) { > - printk(KERN_ERR "%s: Could not read %u bytes\n", > - __func__, crypt_stat->extent_size); > + printk(KERN_ERR "%s: Could not read %zu bytes, inode %lu\n", > + __func__, crypt_stat->extent_size, > + ecryptfs_inode->i_ino); > goto out; > } > > @@ -1625,7 +1626,8 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry) > 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; > } > @@ -1634,7 +1636,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 > @@ -1645,7 +1648,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 > [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-01-20 20:17 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-01-11 17:00 [PATCH 0/2] ecryptfs: Improved metadata read failure logging Tim Gardner 2012-01-11 17:00 ` [PATCH 1/2] ecryptfs: Improve metatdata " Tim Gardner 2012-01-11 20:36 ` Kees Cook 2012-01-12 12:00 ` Tyler Hicks 2012-01-12 16:45 ` Tim Gardner 2012-01-12 16:45 ` Tim Gardner 2012-01-20 19:50 ` Tyler Hicks 2012-01-20 20:17 ` Tim Gardner 2012-01-11 17:00 ` [PATCH 2/2] ecryptfs: Print inode on metadata error Tim Gardner 2012-01-11 20:38 ` Kees Cook 2012-01-12 12:02 ` Tyler Hicks
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.