ecryptfs_privileged_open(): when kthread-ecryptfs is required ?

ecryptfs_privileged_open(): when kthread-ecryptfs is required ?

[Yann Droneaud]

Hi,

While trying to investigate why using ecryptfs is painfully sluggish,
I've find a piece of code that, I believed at first, wouldn't scale.

After adding some printk() there, I've found that it's not at all 
called in my use case.
So I wonder: what's making  ecryptfs_privileged_open()[1] submit work 
to ecryptfs-kthread thread running ecryptfs_threadfn()[2] ?

[1]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/ecryptfs/kthread.c#n155

[2]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/ecryptfs/kthread.c#n56

BTW, I have a patch that remove this kernel thread as I think
credentials can be given to dentry_open(), so instead of running an
helper kthread, some privileged credentials can be given to
dentry_open() in the case ecryptfs_privileged_open() really need to 
open a file read/write. But as I'm not able to verify it's working as 
expected, I'm holding such trival patch.

Regards.

-- 
Yann Droneaud
OPTEYA

Re: ecryptfs_privileged_open(): when kthread-ecryptfs is required ?

[Tyler Hicks]

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

On 2014-05-23 16:24:05, Yann Droneaud wrote:
> Hi,
> 
> While trying to investigate why using ecryptfs is painfully sluggish,

When is it painfully sluggish for you?

The two biggest pain points, off of the top of my head, are:

 1) Extending a file to a much larger size is slow because pages of
    zeros are encrypted in the process

 2) Utils such as find and ls are slow when dealing with large numbers
    of files because, during lookups, the eCryptfs metadata has to be
    read from the front of each lower file in order to fill the eCryptfs
    inode

> I've find a piece of code that, I believed at first, wouldn't scale.
> 
> After adding some printk() there, I've found that it's not at all 
> called in my use case.
> So I wonder: what's making  ecryptfs_privileged_open()[1] submit work 
> to ecryptfs-kthread thread running ecryptfs_threadfn()[2] ?
> 
> [1]
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/ecryptfs/kthread.c#n155
> 
> [2]
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/ecryptfs/kthread.c#n56

This change (746f1e55) happened before my time of maintaining eCryptfs,
but we can piece together the reasoning...

Assume that an eCryptfs mount, /upper, is mounted on /lower and keep in
mind that there is only ever one lower file opened for all open eCryptfs
files that correspond to the lower inode. So, if two processes have open
file descriptors for /upper/foo, then /lower/foo is only opened once by
the eCryptfs and a pointer to the lower file struct is attached to the
eCryptfs inode's private data.

Since there's only ever one lower file opened for a given lower inode,
it must be opened as O_RDWR to satisfy all future open()s of the
eCryptfs file, as one of those future eCryptfs open()s may be O_RDWR.

The last paragraph of the 746f1e55 commit message, indicates that the
kthread was intended to work around DAC issues when opening the lower
file. That may have been the case back then, but it isn't the case now.
Today, dentry_open() succeeds even when the lower inode isn't writeable
by the current process.

Following the dentry_open() code path, I see that security_file_open()
is called from do_dentry_open(). That means that even though the kthread
is no longer needed from a DAC standpoint, it is still needed from the
LSM standpoint. I'll use AppArmor for a simple demonstration:

 1) Create a profile, "test", that grants access to all files but then
    revokes write permissions to /lower/foo
  
    $ echo "profile test { file, deny file /lower/foo w, }" | \
       sudo apparmor_parser -rq

 2) Create an eCryptfs the file
    - eCryptfs will create the lower file

    $ touch /upper/foo

 3) Open the eCryptfs file with O_RDONLY
    - It will succeed

    $ cat /upper/foo

 4) Open the eCryptfs file with O_RDONLY, while confined
    - It should succeed due to falling back to ecryptfs-kthread

    $ aa-exec -p test -- cat /upper/foo

 5) Remove the ecryptfs-kthread code, recompile the eCryptfs module,
    load the new module and set up the eCryptfs mount again

 6) Repeat step #4
    - It will fail this time since there's no ecryptfs-kthread

    $ aa-exec -p test -- cat /upper/foo
    cat: /upper/foo: Permission denied

 7) Note the eCryptfs error messages in the syslog

    [ 3395.362511] Error opening lower file for lower_dentry [0xffff88007461c288] and lower_mnt [0xffff88007a105420]; rc = [-13]
    [ 3395.375967] ecryptfs_open: Error attempting to initialize the lower file for the dentry with name [foo]; rc = [-13]

> BTW, I have a patch that remove this kernel thread as I think
> credentials can be given to dentry_open(), so instead of running an
> helper kthread, some privileged credentials can be given to
> dentry_open() in the case ecryptfs_privileged_open() really need to 
> open a file read/write. But as I'm not able to verify it's working as 
> expected, I'm holding such trival patch.

This made me think of a patch set from years ago that, unfortunately,
went unreviewed:

  https://lkml.org/lkml/2011/4/29/104

I think Roberto probably had the right idea. By default, all lower
inodes are opened using a cred that was set up at eCryptfs mount time.
Optionally, an eCryptfs mount option can be given to specify the LSM
security context that should be used when opening the lower inodes.

Would you have any interest in reviving that patch set?

If so, note that although I used AppArmor in my example above, you'd
need to use SELinux or SMACK to test the ecryptfs_security_ctx mount
option since AppArmor doesn't have support for
set_security_override_from_ctx().

Tyler

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