Re: [RFC][PATCH] ecryptfs: Allow only one instance per lower path

Re: [RFC][PATCH] ecryptfs: Allow only one instance per lower path

[Richard Weinberger]

Am 02.08.2015 um 03:03 schrieb Tyler Hicks:
> Thanks for the report and for the patch, Richard!
> 
> On 2015-07-31 12:23:10, Richard Weinberger wrote:
>> Mounting the same lower path multiple times should not result
>> into multiple ecryptfs instances, otherwise ecryptfs gets confused.
>>
>> A command sequence of:
> 
> An important detail that took me a while to realize is that /tmp should
> be tmpfs in order to trigger the warnings below. I was unable to
> reproduce the warnings with ext4 as the lower filesystem.

Hmm, I saw it with UBIFS found that it triggers with tmpfs too.
I gave ext4 a quick try and yes, it behaves differently, I get
a EIO upon the second unlink().

>> $ mount -t ecryptfs /tmp/.secret /mnt_a/secret/
>> $ mount -t ecryptfs /tmp/.secret /mnt_b/secret/
>> $ mkdir -p /mnt_a/secret/xxx
>> $ mkdir -p /mnt_b/secret/xxx
> 
> Note that the -p option is covering up the fact that /mnt_b/secret/xxx
> already exists. Remove that option and you should see this error:
> 
>   mkdir: cannot create directory ‘/mnt_b/secret/xxx’: File exists
> 
> This really isn't important other than understanding that the second
> mkdir it isn't needed.
> 
>> $ echo foo > /mnt_a/secret/xxx/test.txt
>> $ echo foo > /mnt_b/secret/xxx/test.txt
> 
> /mnt_b/secret/xxx/test.txt should already exist (it does for me, at
> least) so the same file is being written to twice in a row. Again, not
> really important other than to know that it isn't needed.
> 
>> $ rm -rf /mnt_a/secret/xxx
>> $ rm -rf /mnt_b/secret/xxx
> 
> The /mnt_b/secret/xxx dcache entry is stale here because the underlying
> file was removed by the first rm command in the /mnt_a/secret mount. The
> lower inode's nlink is 0 at this point and what should be happening
> here, I think, is that the eCryptfs dentry should be invalidated and the
> eCryptfs inode should be destroyed.
> 
> I think that the proper fix is to catch this condition in
> ecryptfs_d_revalidate(). I've started working on coming up with a patch
> for that but I'll need some more time to finish and test it.

So ecryptfs definitely supports mounting the same lower path multiple times?
What is the benefit of that behavior?

Thanks,
//richard

Re: [RFC][PATCH] ecryptfs: Allow only one instance per lower path

[Tyler Hicks]

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

On 2015-08-02 09:51:16, Richard Weinberger wrote:
> Am 02.08.2015 um 03:03 schrieb Tyler Hicks:
> > Thanks for the report and for the patch, Richard!
> > 
> > On 2015-07-31 12:23:10, Richard Weinberger wrote:
> >> Mounting the same lower path multiple times should not result
> >> into multiple ecryptfs instances, otherwise ecryptfs gets confused.
> >>
> >> A command sequence of:
> > 
> > An important detail that took me a while to realize is that /tmp should
> > be tmpfs in order to trigger the warnings below. I was unable to
> > reproduce the warnings with ext4 as the lower filesystem.
> 
> Hmm, I saw it with UBIFS found that it triggers with tmpfs too.
> I gave ext4 a quick try and yes, it behaves differently, I get
> a EIO upon the second unlink().
> 
> >> $ mount -t ecryptfs /tmp/.secret /mnt_a/secret/
> >> $ mount -t ecryptfs /tmp/.secret /mnt_b/secret/
> >> $ mkdir -p /mnt_a/secret/xxx
> >> $ mkdir -p /mnt_b/secret/xxx
> > 
> > Note that the -p option is covering up the fact that /mnt_b/secret/xxx
> > already exists. Remove that option and you should see this error:
> > 
> >   mkdir: cannot create directory ‘/mnt_b/secret/xxx’: File exists
> > 
> > This really isn't important other than understanding that the second
> > mkdir it isn't needed.
> > 
> >> $ echo foo > /mnt_a/secret/xxx/test.txt
> >> $ echo foo > /mnt_b/secret/xxx/test.txt
> > 
> > /mnt_b/secret/xxx/test.txt should already exist (it does for me, at
> > least) so the same file is being written to twice in a row. Again, not
> > really important other than to know that it isn't needed.
> > 
> >> $ rm -rf /mnt_a/secret/xxx
> >> $ rm -rf /mnt_b/secret/xxx
> > 
> > The /mnt_b/secret/xxx dcache entry is stale here because the underlying
> > file was removed by the first rm command in the /mnt_a/secret mount. The
> > lower inode's nlink is 0 at this point and what should be happening
> > here, I think, is that the eCryptfs dentry should be invalidated and the
> > eCryptfs inode should be destroyed.
> > 
> > I think that the proper fix is to catch this condition in
> > ecryptfs_d_revalidate(). I've started working on coming up with a patch
> > for that but I'll need some more time to finish and test it.
> 
> So ecryptfs definitely supports mounting the same lower path multiple times?
> What is the benefit of that behavior?

No, it doesn't support that in a way that provides consistency among all
of the eCryptfs mounts.

However, multiple mounts on the same lower path is not the cause of this
bug. The real issue is a stale dcache entry when the lower filesystem
has been modified without eCryptfs' knowing. I can trigger the same
warnings with only a single eCryptfs mount.

Tyler
> 
> Thanks,
> //richard

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

Re: [RFC][PATCH] ecryptfs: Allow only one instance per lower path

[Richard Weinberger]

Tyler,

Am 03.08.2015 um 07:27 schrieb Tyler Hicks:
>> So ecryptfs definitely supports mounting the same lower path multiple times?
>> What is the benefit of that behavior?
> 
> No, it doesn't support that in a way that provides consistency among all
> of the eCryptfs mounts.

Okay, then I'd argument to give my patch a try although it is not the solution
to the problem I've reported. :-)
If you don't mind I'll resend with a proper changelog.

> However, multiple mounts on the same lower path is not the cause of this
> bug. The real issue is a stale dcache entry when the lower filesystem
> has been modified without eCryptfs' knowing. I can trigger the same
> warnings with only a single eCryptfs mount.

Interesting, that renders the whole issue into a user triggerable kernel
bug. :-(

Thanks,
//richard

Re: [RFC][PATCH] ecryptfs: Allow only one instance per lower path

[Tyler Hicks]

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

On 2015-08-03 20:31:57, Richard Weinberger wrote:
> Tyler,
> 
> Am 03.08.2015 um 07:27 schrieb Tyler Hicks:
> >> So ecryptfs definitely supports mounting the same lower path multiple times?
> >> What is the benefit of that behavior?
> > 
> > No, it doesn't support that in a way that provides consistency among all
> > of the eCryptfs mounts.
> 
> Okay, then I'd argument to give my patch a try although it is not the solution
> to the problem I've reported. :-)
> If you don't mind I'll resend with a proper changelog.

That patch isn't correct since it assumes that all eCryptfs super blocks
are equal if the lower paths (and, ultimately, the lower inode) are
equal. However, the lower path is only one of many properties of an
eCryptfs superblock. For example, the second mount may have been
configured to use a different file encryption key.

Tyler

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

Re: [RFC][PATCH] ecryptfs: Allow only one instance per lower path

[Richard Weinberger]

Tyler,

Am 04.08.2015 um 01:07 schrieb Tyler Hicks:
>> Okay, then I'd argument to give my patch a try although it is not the solution
>> to the problem I've reported. :-)
>> If you don't mind I'll resend with a proper changelog.
> 
> That patch isn't correct since it assumes that all eCryptfs super blocks
> are equal if the lower paths (and, ultimately, the lower inode) are
> equal. However, the lower path is only one of many properties of an
> eCryptfs superblock. For example, the second mount may have been
> configured to use a different file encryption key.

How would this work if I mount /foo using AES to /mnt_a
and /foo again using 3DES to /mnt_b?
Wouldn't both ecrytpfs instances kill each other's files?

Thanks,
//richard

Re: [RFC][PATCH] ecryptfs: Allow only one instance per lower path

[Tyler Hicks]

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

On 2015-08-04 07:46:50, Richard Weinberger wrote:
> Tyler,
> 
> Am 04.08.2015 um 01:07 schrieb Tyler Hicks:
> >> Okay, then I'd argument to give my patch a try although it is not the solution
> >> to the problem I've reported. :-)
> >> If you don't mind I'll resend with a proper changelog.
> > 
> > That patch isn't correct since it assumes that all eCryptfs super blocks
> > are equal if the lower paths (and, ultimately, the lower inode) are
> > equal. However, the lower path is only one of many properties of an
> > eCryptfs superblock. For example, the second mount may have been
> > configured to use a different file encryption key.
> 
> How would this work if I mount /foo using AES to /mnt_a
> and /foo again using 3DES to /mnt_b?
> Wouldn't both ecrytpfs instances kill each other's files?

No, they shouldn't. Each file contains metadata that describes the
cipher, cipher mode, key signature, etc., that was used to encrypt the
file.

When the file is initially opened, the process must have the correct key
in the keyrings that it has access to. After that requirement has been
met, eCryptfs is smart enough to parse the metadata and use the correct
cipher and mode.

The mount options, such as ecryptfs_cipher, only specify what should be
used when creating new files.

Tyler

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

[PATCH] eCryptfs: Invalidate dcache entries when lower i_nlink is zero

[Tyler Hicks]

Consider eCryptfs dcache entries to be stale when the corresponding
lower inode's i_nlink count is zero. This solves a problem caused by the
lower inode being directly modified, without going through the eCryptfs
mount, leaving stale eCryptfs dentries cached and the eCryptfs inode's
i_nlink count not being cleared.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Reported-by: Richard Weinberger <richard@nod.at>
---
 fs/ecryptfs/dentry.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/ecryptfs/dentry.c b/fs/ecryptfs/dentry.c
index 8db0b46..63cd2c1 100644
--- a/fs/ecryptfs/dentry.c
+++ b/fs/ecryptfs/dentry.c
@@ -45,20 +45,20 @@
 static int ecryptfs_d_revalidate(struct dentry *dentry, unsigned int flags)
 {
 	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
-	int rc;
-
-	if (!(lower_dentry->d_flags & DCACHE_OP_REVALIDATE))
-		return 1;
+	int rc = 1;
 
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
 
-	rc = lower_dentry->d_op->d_revalidate(lower_dentry, flags);
+	if (lower_dentry->d_flags & DCACHE_OP_REVALIDATE)
+		rc = lower_dentry->d_op->d_revalidate(lower_dentry, flags);
+
 	if (d_really_is_positive(dentry)) {
-		struct inode *lower_inode =
-			ecryptfs_inode_to_lower(d_inode(dentry));
+		struct inode *inode = d_inode(dentry);
 
-		fsstack_copy_attr_all(d_inode(dentry), lower_inode);
+		fsstack_copy_attr_all(inode, ecryptfs_inode_to_lower(inode));
+		if (!inode->i_nlink)
+			return 0;
 	}
 	return rc;
 }
-- 
2.1.4