[PATCH] eCryptfs: fix possible fault in ecryptfs_sync_page

[PATCH] eCryptfs: fix possible fault in ecryptfs_sync_page

[Ryusuke Konishi]

This will avoid a possible fault in ecryptfs_sync_page().

In the function, eCryptfs calls sync_page() method of a lower 
filesystem without checking its existence.  However, there 
are many filesystems that don't have this method including
network filesystems such as NFS, AFS, and so forth.
They may fail when an eCryptfs page is waiting for lock.

Signed-off-by: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>
---
 fs/ecryptfs/mmap.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index e4ab7bc..fd3f94d 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -834,7 +834,8 @@ static void ecryptfs_sync_page(struct page *page)
 		ecryptfs_printk(KERN_DEBUG, "find_lock_page failed\n");
 		return;
 	}
-	lower_page->mapping->a_ops->sync_page(lower_page);
+	if (lower_page->mapping->a_ops->sync_page)
+		lower_page->mapping->a_ops->sync_page(lower_page);
 	ecryptfs_printk(KERN_DEBUG, "Unlocking page with index = [0x%.16x]\n",
 			lower_page->index);
 	unlock_page(lower_page);

Re: [Ecryptfs-devel] [PATCH] eCryptfs: fix possible fault in ecryptfs_sync_page

[Michael Halcrow]

On Thu, Aug 23, 2007 at 04:42:59PM +0900, Ryusuke Konishi wrote:
> This will avoid a possible fault in ecryptfs_sync_page().

Acked-by: Michael Halcrow <mhalcrow@us.ibm.com>

Note that there are other outstanding issues with eCryptfs on NFS. For
instance, prepare_write()/commit_write() have gone away in -mm,
leading to an oops when eCryptfs tries to call them directly, which is
well deserved, since eCryptfs really should not be doing that. Unionfs
has just two places where it calls vfs_read() and vfs_write()
respectively; eCryptfs is a bit more complex, with multiple write
paths that end up writing encrypted data and updating metadata in the
header. I am currently trying to find a way to convert everything over
to vfs_read() and vfs_write() in eCryptfs, in a way that does not
ultimately result in a kernel hang.

> In the function, eCryptfs calls sync_page() method of a lower 
> filesystem without checking its existence.  However, there 
> are many filesystems that don't have this method including
> network filesystems such as NFS, AFS, and so forth.
> They may fail when an eCryptfs page is waiting for lock.
> 
> Signed-off-by: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>
> ---
>  fs/ecryptfs/mmap.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
> index e4ab7bc..fd3f94d 100644
> --- a/fs/ecryptfs/mmap.c
> +++ b/fs/ecryptfs/mmap.c
> @@ -834,7 +834,8 @@ static void ecryptfs_sync_page(struct page *page)
>  		ecryptfs_printk(KERN_DEBUG, "find_lock_page failed\n");
>  		return;
>  	}
> -	lower_page->mapping->a_ops->sync_page(lower_page);
> +	if (lower_page->mapping->a_ops->sync_page)
> +		lower_page->mapping->a_ops->sync_page(lower_page);
>  	ecryptfs_printk(KERN_DEBUG, "Unlocking page with index = [0x%.16x]\n",
>  			lower_page->index);
>  	unlock_page(lower_page);
> 
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >>  http://get.splunk.com/
> _______________________________________________
> eCryptfs-devel mailing list
> eCryptfs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ecryptfs-devel

Re: [Ecryptfs-devel] [PATCH] eCryptfs: fix possible fault in ecryptfs_sync_page

[Erez Zadok]

In message <20070823143745.GM22689@halcrow.austin.ibm.com>, Michael Halcrow writes:
> On Thu, Aug 23, 2007 at 04:42:59PM +0900, Ryusuke Konishi wrote:
> > This will avoid a possible fault in ecryptfs_sync_page().
> 
> Acked-by: Michael Halcrow <mhalcrow@us.ibm.com>
> 
> Note that there are other outstanding issues with eCryptfs on NFS. For
> instance, prepare_write()/commit_write() have gone away in -mm,
> leading to an oops when eCryptfs tries to call them directly, which is
> well deserved, since eCryptfs really should not be doing that. Unionfs
> has just two places where it calls vfs_read() and vfs_write()
> respectively; eCryptfs is a bit more complex, with multiple write
> paths that end up writing encrypted data and updating metadata in the
> header. I am currently trying to find a way to convert everything over
> to vfs_read() and vfs_write() in eCryptfs, in a way that does not
> ultimately result in a kernel hang.

Mike, check the latest unionfs 2.1.2 code in
http://unionfs.filesystems.org/.  In the past couple of months I've already
gotten rid of direct calls to prepare/commit_write in favor of
vfs_read/write.  There were a number of reasons for that mostly having to do
with races, deadlocks/hangs, and memory-pressure conditions.  I also have a
number of additional mmap related fixes in fs/unionfs/mmap.c that you should
probably look at as they're applicable to stacking in general.

Lastly, to stack on nfs, at the very least you'll probably need my lower_nd
fixes from unionfs as well, or something like it.  As of 2.6.23-rc3, you
need to stack on the nameidata structure even for nfs2/nfs3.  My patches
take care of that for now (until such a day as the VFS will have a true,
stackable, small intent structure that can be passed to a lower f/s).

Cheers,
Erez.

Re: [Ecryptfs-devel] [PATCH] eCryptfs: fix possible fault in ecryptfs_sync_page

[Ryusuke Konishi]

On Thu, 23 Aug 2007, Michael Halcrow write:
>Note that there are other outstanding issues with eCryptfs on NFS. For
>instance, prepare_write()/commit_write() have gone away in -mm,
>leading to an oops when eCryptfs tries to call them directly

Yes, I certainly encountered the problem during testing eCryptfs on -mm,
and found Nick Piggin's new aops patchset lacks changes for eCryptfs.

>which is 
>well deserved, since eCryptfs really should not be doing that. Unionfs
>has just two places where it calls vfs_read() and vfs_write()
>respectively; eCryptfs is a bit more complex, with multiple write
>paths that end up writing encrypted data and updating metadata in the
>header. I am currently trying to find a way to convert everything over
>to vfs_read() and vfs_write() in eCryptfs, in a way that does not
>ultimately result in a kernel hang.

OK, I understand the reason and your plan.  Thanks for letting me know!
As you say, I honestly felt that it's a difficult task to write
stackable filesystems using low level operations.
It's good idea to replace them with the vfs functions.

So, how long does it take for the conversion, do you think?
Though I'm currenty focussing on eCryptfs in mainline,
I'd like to shift my focus to the new one if it's preferable.

Regards,
Ryusuke Konishi