eCryptfs: Request for review

eCryptfs: Request for review

[Michael Halcrow]

[-- Attachment #1: Type: text/plain, Size: 2797 bytes --]

We are preparing to send eCryptfs to the LKML for inclusion in the -mm
tree, and we would like to solicit feedback from those in the
community who have an interest in Linux filesystems and cryptographic
applications. We are mainly interested at this point in comments that
might help us with VFS-related issues.

eCryptfs can be obtained from its SourceForge CVS repository:

http://sourceforge.net/projects/ecryptfs

cvs -d:pserver:anonymous@cvs.sourceforge.net:/cvsroot/ecryptfs login
cvs -z3 -d:pserver:anonymous@cvs.sourceforge.net:/cvsroot/ecryptfs co -P .

The code to perform the filesystem stacking is derived from Erez
Zadok's Cryptfs, which is one of the filesystems instantiated through
the FiST framework:

http://filesystems.org/

I presented eCryptfs at the 2004 and the 2005 Ottawa Linux
Symposium. The paper from this year's symposium starts on page 209 of
the first half of the proceedings:

http://www.linuxsymposium.org/2005/linuxsymposium_procv1.pdf

I like to describe it as a sort of ``PGPFS''. It is stacked on top of
other filesystems. It aims to combine the flexibility of GnuPG
encryption with the transparency of a kernel service. Cryptographic
contexts (e.g., symmetric cipher identifier and encrypted session
keys) are stored in the first page of data in the file. This allows
the underlying encrypted files to be copied between domains with
unmodified userspace applications, and as long as the recipient has
the necessary credentials, he can access the contents of the files
transparently through eCryptfs.

The first release of eCryptfs (0.1) will support only mount-wide
passphrase mode. Some of the more advanced features, such as dynamic
PKI modules (allowing integration w/ GnuPG keyrings, TPM, and so on),
have been implemented and tested to some extent, but they are
cumbersome to deploy without more mature policy support. We have
disabled public key operation modes for the 0.1 release (also in
anticipation of better policy support in the future releases), but
more advanced users and developers are encouraged to experiment with
that code to their hearts' content.

eCryptfs is still a little rough around the edges (some behavior is
due to current needs for debugging), but it is pretty close to its
final form for the 0.1 release. There are known corner cases where it
breaks down right now, and we are chasing those bugs at the
moment. Please take a look at it and provide whatever feedback you
can.

Thanks,
Mike
.___________________________________________________________________.
                         Michael A. Halcrow                          
       Security Software Engineer, IBM Linux Technology Center       
GnuPG Fingerprint: 419C 5B1E 948A FA73 A54C  20F5 DB40 8531 6DCA 8769

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 481 bytes --]

Re: eCryptfs: Request for review

[Greg KH]

On Tue, Oct 18, 2005 at 02:38:11PM -0500, Michael Halcrow wrote:
> We are preparing to send eCryptfs to the LKML for inclusion in the -mm
> tree, and we would like to solicit feedback from those in the
> community who have an interest in Linux filesystems and cryptographic
> applications. We are mainly interested at this point in comments that
> might help us with VFS-related issues.

Then please post patches in the proper form.  Pointing to a cvs server
just doesn't cut it :)

thanks,

greg k-h

Re: eCryptfs: Request for review

[Charles P. Wright]

On Tue, 2005-10-18 at 14:38 -0500, Michael Halcrow wrote:
> I like to describe it as a sort of ``PGPFS''. It is stacked on top of
> other filesystems. It aims to combine the flexibility of GnuPG
> encryption with the transparency of a kernel service. Cryptographic
> contexts (e.g., symmetric cipher identifier and encrypted session
> keys) are stored in the first page of data in the file. This allows
> the underlying encrypted files to be copied between domains with
> unmodified userspace applications, and as long as the recipient has
> the necessary credentials, he can access the contents of the files
> transparently through eCryptfs.
> 
> The first release of eCryptfs (0.1) will support only mount-wide
> passphrase mode. Some of the more advanced features, such as dynamic
> PKI modules (allowing integration w/ GnuPG keyrings, TPM, and so on),
> have been implemented and tested to some extent, but they are
> cumbersome to deploy without more mature policy support. We have
> disabled public key operation modes for the 0.1 release (also in
> anticipation of better policy support in the future releases), but
> more advanced users and developers are encouraged to experiment with
> that code to their hearts' content.
> 
> eCryptfs is still a little rough around the edges (some behavior is
> due to current needs for debugging), but it is pretty close to its
> final form for the 0.1 release. There are known corner cases where it
> breaks down right now, and we are chasing those bugs at the
> moment. Please take a look at it and provide whatever feedback you
> can.
> 
> Thanks,
> Mike
Mike,

This is just a brief glance, and by no means complete, but here are a
few things that I saw.

In dentry.c you shouldn't implement d_delete.  This will cause problems
with certain lower-level file systems.

In crypto.c:ecryptfs_dump_hex you have variables intermixed with code,
which won't work on some older compilers (AFAIK gcc 2.95 will barf).

In ecryptfs_llseek,  If you are adding pages for extra information, you
need to convert the offset to a lower-level offset, and then convert the
return back to what user space expects it to be.  You should test fsx on
your file system, to sort these types of things out.

You should probably rename ecryptfs_write to ecryptfs_dir_write, or just
use generic_read_dir and NULL for directory read and write,
respectively.

In inode.c:
Instead of get_parent you can use dget_parent.  The core kernel just
seems to have lock_parent as two lines in the function that need it
instead of the function these days.

The rest of the VFS locking functions should also be converted from 2.4
to 2.6 primitives (this is a bug that you inherited from the FiST
templates).  For example, instead of using double_down, you should use
lock_rename.

For main.c, you can replace the nasty mount option string processing
with lib/parser.c.

In mmap.c, how do you handle encryption along the writepage path?

Charles

Re: eCryptfs: Request for review

[Michael Thompson]

On 10/19/05, Charles P. Wright <cwright@cs.sunysb.edu> wrote:
> This is just a brief glance, and by no means complete, but here are a
> few things that I saw.
>
> In dentry.c you shouldn't implement d_delete.  This will cause problems
> with certain lower-level file systems.
Not entirely sure I follow the reasoning why here. Obviously, if it
needs to be, it can be removed, but all this function is doing is a
bunch of sanity checks, then then calling d_delete on the lower
filesystems if it exists. Just trying to get a better understand of
the code you see :)

> In crypto.c:ecryptfs_dump_hex you have variables intermixed with code,
> which won't work on some older compilers (AFAIK gcc 2.95 will barf).
Noted. Changed.

> In ecryptfs_llseek,  If you are adding pages for extra information, you
> need to convert the offset to a lower-level offset, and then convert the
> return back to what user space expects it to be.  You should test fsx on
> your file system, to sort these types of things out.
I believe that this is working, we have run fsx "succesfully" for
10000 iterations. However, its not a total success because we are
bugging out in fs/buffer.c:1822 BUG_ON(PageWriteback(page));
Will look a bit more closely though.

> You should probably rename ecryptfs_write to ecryptfs_dir_write, or just
> use generic_read_dir and NULL for directory read and write,
> respectively.
Changed. Will look into using the generics, not entirely sure what
impact that will have without further investigation.

> In inode.c:
> Instead of get_parent you can use dget_parent.  The core kernel just
> seems to have lock_parent as two lines in the function that need it
> instead of the function these days.
>
> The rest of the VFS locking functions should also be converted from 2.4
> to 2.6 primitives (this is a bug that you inherited from the FiST
> templates).  For example, instead of using double_down, you should use
> lock_rename.
Thanks for those, adding them to our ever growing TODO list :)

> For main.c, you can replace the nasty mount option string processing
> with lib/parser.c.
Will look into doing that :)

Mike

Re: eCryptfs: Request for review

[Charles P. Wright]

On Wed, 2005-10-19 at 14:00 -0500, Michael Thompson wrote:
> On 10/19/05, Charles P. Wright <cwright@cs.sunysb.edu> wrote:
> > This is just a brief glance, and by no means complete, but here are a
> > few things that I saw.
> >
> > In dentry.c you shouldn't implement d_delete.  This will cause problems
> > with certain lower-level file systems.
> Not entirely sure I follow the reasoning why here. Obviously, if it
> needs to be, it can be removed, but all this function is doing is a
> bunch of sanity checks, then then calling d_delete on the lower
> filesystems if it exists. Just trying to get a better understand of
> the code you see :)
The reference count of an eCryptfs dentry may not be the same as the
reference count of the lower-level dentry.  If the eCryptfs dentry hits
zero, but the lower-level dentry does not, you are going to remove the
lower-level dentry from the name space prematurely.

This manifested itself on Unionfs running over squashfs.

> > In ecryptfs_llseek,  If you are adding pages for extra information, you
> > need to convert the offset to a lower-level offset, and then convert the
> > return back to what user space expects it to be.  You should test fsx on
> > your file system, to sort these types of things out.
> I believe that this is working, we have run fsx "succesfully" for
> 10000 iterations. However, its not a total success because we are
> bugging out in fs/buffer.c:1822 BUG_ON(PageWriteback(page));
> Will look a bit more closely though.
Actually nevermind, you are using generic_file_llseek on your own file
instead of passing the operation down.  That should be fine and dandy
for files, but I don't know if you are going to have problems with
directories.

Charles

Re: eCryptfs: Request for review

[Michael Thompson]

On 10/19/05, Charles P. Wright <cwright@cs.sunysb.edu> wrote:
> The reference count of an eCryptfs dentry may not be the same as the
> reference count of the lower-level dentry.  If the eCryptfs dentry hits
> zero, but the lower-level dentry does not, you are going to remove the
> lower-level dentry from the name space prematurely.
>
> This manifested itself on Unionfs running over squashfs.
I C, if you will. Now that I know, consider it removed :)

> > > In ecryptfs_llseek,  If you are adding pages for extra information, you
> > > need to convert the offset to a lower-level offset, and then convert the
> > > return back to what user space expects it to be.  You should test fsx on
> > > your file system, to sort these types of things out.
> > I believe that this is working, we have run fsx "succesfully" for
> > 10000 iterations. However, its not a total success because we are
> > bugging out in fs/buffer.c:1822 BUG_ON(PageWriteback(page));
> > Will look a bit more closely though.
> Actually nevermind, you are using generic_file_llseek on your own file
> instead of passing the operation down.  That should be fine and dandy
> for files, but I don't know if you are going to have problems with
> directories.
Guess I'll have to test a bit more then... *marks on the TODO*.

Would you like to take a stab at why we are hitting the
fs/buffer.c:1822 BUG_ON(PageWriteback(page)); problem?
I'm pretty much at a loss, although, I will be honest, I haven't
looked into it very much
(it's scary).

Thanks for the help,
Mike

Re: eCryptfs: Request for review

[Erez Zadok]

In message <afcef88a0510191255t47047d1q93b4a7428609b324@mail.gmail.com>, Michael Thompson writes:

> Would you like to take a stab at why we are hitting the
> fs/buffer.c:1822 BUG_ON(PageWriteback(page)); problem?
> I'm pretty much at a loss, although, I will be honest, I haven't
> looked into it very much
> (it's scary).

That bug is reminiscent of a problem we've had in one of our file systems in
->writepage.  We had a problem where our ->writepage was indirectly
producing more dirty data to write, and the page-flushing system didn't like
that.  ->writepage's semantics have to be followed precisely (i.e., "write
this page and ONLY this page out NOW, no if's or but's").

Erez.

Re: eCryptfs: Request for review

[Badari Pulavarty]

On Wed, 2005-10-19 at 14:55 -0500, Michael Thompson wrote:
> On 10/19/05, Charles P. Wright <cwright@cs.sunysb.edu> wrote:
> > The reference count of an eCryptfs dentry may not be the same as the
> > reference count of the lower-level dentry.  If the eCryptfs dentry hits
> > zero, but the lower-level dentry does not, you are going to remove the
> > lower-level dentry from the name space prematurely.
> >
> > This manifested itself on Unionfs running over squashfs.
> I C, if you will. Now that I know, consider it removed :)
> 
> > > > In ecryptfs_llseek,  If you are adding pages for extra information, you
> > > > need to convert the offset to a lower-level offset, and then convert the
> > > > return back to what user space expects it to be.  You should test fsx on
> > > > your file system, to sort these types of things out.
> > > I believe that this is working, we have run fsx "succesfully" for
> > > 10000 iterations. However, its not a total success because we are
> > > bugging out in fs/buffer.c:1822 BUG_ON(PageWriteback(page));
> > > Will look a bit more closely though.
> > Actually nevermind, you are using generic_file_llseek on your own file
> > instead of passing the operation down.  That should be fine and dandy
> > for files, but I don't know if you are going to have problems with
> > directories.
> Guess I'll have to test a bit more then... *marks on the TODO*.
> 
> Would you like to take a stab at why we are hitting the
> fs/buffer.c:1822 BUG_ON(PageWriteback(page)); problem?
> I'm pretty much at a loss, although, I will be honest, I haven't
> looked into it very much

It is telling you that there is already IO in progress on that page.
Either you ended up in  __block_write_full_page() twice or called
__block_write_full_page() before finishing up end_page_writeback().

Thanks,
Badari

Re: eCryptfs: Request for review

[Michael Thompson]

On 10/19/05, Badari Pulavarty <pbadari@gmail.com> wrote:
> It is telling you that there is already IO in progress on that page.
> Either you ended up in  __block_write_full_page() twice or called
> __block_write_full_page() before finishing up end_page_writeback().

OK, I have narrowed it down to the following: we are in
ecryptfs_writepage, and do the following:
lower_page = grab_cache_page(lower_inode->i_mapping, 0);
where lower_page is the page in the lower filesysetm we want to write to.

We consistantly panic when, after we do the grab_cache_page(), we have
a locked page which is set for PageWriteback....
PageWriteback(lower_page) != 0.

What I would like to know is how I should proceed when I am in this
condition. I am not sure if it is acceptable to simply return from the
function, or if I need to set some flags, or wait on the lock to free
up. Unfortunately, I haven't been digging around the VFS long enough
to know what makes sense in this case.

_Any_ guidance would be extremely helpful.

Cheers,
Mike

Re: eCryptfs: Request for review

[Shaya Potter]

On Fri, 2005-10-21 at 16:44 -0500, Michael Thompson wrote:
> On 10/19/05, Badari Pulavarty <pbadari@gmail.com> wrote:
> > It is telling you that there is already IO in progress on that page.
> > Either you ended up in  __block_write_full_page() twice or called
> > __block_write_full_page() before finishing up end_page_writeback().
> 
> OK, I have narrowed it down to the following: we are in
> ecryptfs_writepage, and do the following:
> lower_page = grab_cache_page(lower_inode->i_mapping, 0);
> where lower_page is the page in the lower filesysetm we want to write to.
> 
> We consistantly panic when, after we do the grab_cache_page(), we have
> a locked page which is set for PageWriteback....
> PageWriteback(lower_page) != 0.
> 
> What I would like to know is how I should proceed when I am in this
> condition. I am not sure if it is acceptable to simply return from the
> function, or if I need to set some flags, or wait on the lock to free
> up. Unfortunately, I haven't been digging around the VFS long enough
> to know what makes sense in this case.

If the lower page can be in a writeback state while the upper page
isn't, don't you have other problems (i.e. things writing to the lower
file outside the knowledge of ecryptfs's stacked framework)?

Re: eCryptfs: Request for review

[Badari Pulavarty]

On Fri, 2005-10-21 at 16:44 -0500, Michael Thompson wrote:
> On 10/19/05, Badari Pulavarty <pbadari@gmail.com> wrote:
> > It is telling you that there is already IO in progress on that page.
> > Either you ended up in  __block_write_full_page() twice or called
> > __block_write_full_page() before finishing up end_page_writeback().
> 
> OK, I have narrowed it down to the following: we are in
> ecryptfs_writepage, and do the following:
> lower_page = grab_cache_page(lower_inode->i_mapping, 0);
> where lower_page is the page in the lower filesysetm we want to write to.
> 
> We consistantly panic when, after we do the grab_cache_page(), we have
> a locked page which is set for PageWriteback....
> PageWriteback(lower_page) != 0.
> 
> What I would like to know is how I should proceed when I am in this
> condition. I am not sure if it is acceptable to simply return from the
> function, or if I need to set some flags, or wait on the lock to free
> up. Unfortunately, I haven't been digging around the VFS long enough
> to know what makes sense in this case.

I have not looked at your code fully  - forgive me if doesn't make
any sense to you.

Why is "lower_page" is  under writeback currently ? Is it that,
file got modified underneath eCryptfs ?

Or may be, you need to do wait_on_page_writeback(lowerpage) in
your ecryptfs_commit_write() after grabbing lower page and
before calling prepare-write on lower page.

Makes any sense ? My head hurts thinking about it :)

Thanks,
Badari

Re: eCryptfs: Request for review

[Michael Thompson]

On 10/21/05, Badari Pulavarty <pbadari@gmail.com> wrote:
> Makes any sense ? My head hurts thinking about it :)
Me too!

I believe I have resolved the problem. When we were grabbing the lower
page, we would always grab with the same index, which is not what we
intended. After this change, we work properly under fsx at 1 million
iterations.

Thanks for the feedback, it got me looking in the right places :)
Mike

Re: eCryptfs: Request for review

[Michael Thompson]

Hello again,

In preperation of our submission to the LKML, I have begun running
sparse on the code, and have discovered that due to the nature of our
stacked filesystem, we have encountered the following sparse message:

fs/ecryptfs/inode.c:843:60: warning: incorrect type in argument 2
(different address spaces)
fs/ecryptfs/inode.c:843:60:    expected char [noderef] *<noident><asn:1>
fs/ecryptfs/inode.c:843:60:    got char *[assigned] lower_buf

This is in the function ecryptfs_readlink, which follows the inode
operations definition of:
int (*readlink) (struct dentry *, char __user *,int);

The reason we are getting these messages is that we are calling the
readlink operation of the lower filesystem, but passing it a buffer
which we allocate. Spare is picking up on the fact that the lower fs's
readlink expects a buffer of type (char __user*), but we provide
(char*).

Is it acceptable to do the following:
char *buffer;
...
lower_fs_inode->i_op->readlink(lower_dentry, (char __user*)lower_buf, bufsiz);

Not sure, as this will stop sparse from complaining, but I am not sure
what ramifications it will have for the handling of memory in the
lower filesystem as it is not a pointer from user space but from
kernel space.  In short, is it OK to force a cast to effectively trick
the system? or is there a better approach? Even general guidelines
will help as this problem is appears in multiple places through the
code and will probably exist in other stacked filesystems.

Thanks,
Mike

Re: eCryptfs: Request for review

[Anton Altaparmakov]

On Wed, 26 Oct 2005, Michael Thompson wrote:
> Hello again,
> 
> In preperation of our submission to the LKML, I have begun running
> sparse on the code, and have discovered that due to the nature of our
> stacked filesystem, we have encountered the following sparse message:
> 
> fs/ecryptfs/inode.c:843:60: warning: incorrect type in argument 2
> (different address spaces)
> fs/ecryptfs/inode.c:843:60:    expected char [noderef] *<noident><asn:1>
> fs/ecryptfs/inode.c:843:60:    got char *[assigned] lower_buf
> 
> This is in the function ecryptfs_readlink, which follows the inode
> operations definition of:
> int (*readlink) (struct dentry *, char __user *,int);
> 
> The reason we are getting these messages is that we are calling the
> readlink operation of the lower filesystem, but passing it a buffer
> which we allocate. Spare is picking up on the fact that the lower fs's
> readlink expects a buffer of type (char __user*), but we provide
> (char*).
> 
> Is it acceptable to do the following:
> char *buffer;
> ...
> lower_fs_inode->i_op->readlink(lower_dentry, (char __user*)lower_buf, bufsiz);

Yes, that is absolutely fine to do.

But I do hope you are doing the necessary get_fs/set_fs magic if you are 
passing kernel pointers instead of user space ones...  I haven't seen your 
code so apologies if I am stating the obvious.  If you OTOH don't know 
what I am talking about you better learn it fast, e.g. look at 
drivers/block/loop.c::__do_lo_send_write() which does it.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: eCryptfs: Request for review

[Charles P. Wright]

On Wed, 2005-10-26 at 21:13 +0100, Anton Altaparmakov wrote:
> On Wed, 26 Oct 2005, Michael Thompson wrote:
> > Hello again,
> > 
> > In preperation of our submission to the LKML, I have begun running
> > sparse on the code, and have discovered that due to the nature of our
> > stacked filesystem, we have encountered the following sparse message:
> > 
> > fs/ecryptfs/inode.c:843:60: warning: incorrect type in argument 2
> > (different address spaces)
> > fs/ecryptfs/inode.c:843:60:    expected char [noderef] *<noident><asn:1>
> > fs/ecryptfs/inode.c:843:60:    got char *[assigned] lower_buf
> > 
> > This is in the function ecryptfs_readlink, which follows the inode
> > operations definition of:
> > int (*readlink) (struct dentry *, char __user *,int);
> > 
> > The reason we are getting these messages is that we are calling the
> > readlink operation of the lower filesystem, but passing it a buffer
> > which we allocate. Spare is picking up on the fact that the lower fs's
> > readlink expects a buffer of type (char __user*), but we provide
> > (char*).
> > 
> > Is it acceptable to do the following:
> > char *buffer;
> > ...
> > lower_fs_inode->i_op->readlink(lower_dentry, (char __user*)lower_buf, bufsiz);
> 
> Yes, that is absolutely fine to do.
> 
> But I do hope you are doing the necessary get_fs/set_fs magic if you are 
> passing kernel pointers instead of user space ones...
Mike,

If you haven't changed the FiST templates too much they already handle
this.

Charles

Re: eCryptfs: Request for review

[Charles P. Wright]

On Wed, 2005-10-19 at 14:55 -0500, Michael Thompson wrote:
> > > > In ecryptfs_llseek,  If you are adding pages for extra information, you
> > > > need to convert the offset to a lower-level offset, and then convert the
> > > > return back to what user space expects it to be.  You should test fsx on
> > > > your file system, to sort these types of things out.
> > > I believe that this is working, we have run fsx "succesfully" for
> > > 10000 iterations. However, its not a total success because we are
> > > bugging out in fs/buffer.c:1822 BUG_ON(PageWriteback(page));
> > > Will look a bit more closely though.
> > Actually nevermind, you are using generic_file_llseek on your own file
> > instead of passing the operation down.  That should be fine and dandy
> > for files, but I don't know if you are going to have problems with
> > directories.
> Guess I'll have to test a bit more then... *marks on the TODO*.
You should check to see which file systems implement llseek for
directories, that should give you a hint as to where you need to do some
testing.

> Would you like to take a stab at why we are hitting the
> fs/buffer.c:1822 BUG_ON(PageWriteback(page)); problem?
> I'm pretty much at a loss, although, I will be honest, I haven't
> looked into it very much
> (it's scary).
I can't be of much help here.

Charles

Re: eCryptfs: Request for review

[Michael Thompson]

On 10/19/05, Charles P. Wright <cwright@cs.sunysb.edu> wrote:
> In inode.c:
> Instead of get_parent you can use dget_parent.  The core kernel just
> seems to have lock_parent as two lines in the function that need it
> instead of the function these days.
What function would that be? We've looked around to try to find out
what replaced it, but have had no luck :(

> For main.c, you can replace the nasty mount option string processing
> with lib/parser.c.
This is now taken care of, and yes, it is a lot nice :)

Thanks much,
Mike

Re: eCryptfs: Request for review

[Charles P. Wright]

On Wed, 2005-10-26 at 18:29 -0500, Michael Thompson wrote:
> On 10/19/05, Charles P. Wright <cwright@cs.sunysb.edu> wrote:
> > In inode.c:
> > Instead of get_parent you can use dget_parent.  The core kernel just
> > seems to have lock_parent as two lines in the function that need it
> > instead of the function these days.
> What function would that be? We've looked around to try to find out
> what replaced it, but have had no luck :(
The lock_parent function wasn't actually used in 2.4, so it seems to
have been removed in 2.6.  AFAIK, there is no replacement.  The
functions that need to lock a parent directory just do a dget (or
equivalent), and then down the semaphore.

Some of them use helpers like lookup_create that do the downing for
them, but those won't work in the context of a stackable f/s.

Charles