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

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

[Richard Weinberger]

Mounting the same lower path multiple times should not result
into multiple ecryptfs instances, otherwise ecryptfs gets confused.

A command sequence of:

$ 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
$ echo foo > /mnt_a/secret/xxx/test.txt
$ echo foo > /mnt_b/secret/xxx/test.txt
$ rm -rf /mnt_a/secret/xxx
$ rm -rf /mnt_b/secret/xxx

results into:
[  125.536842] ------------[ cut here ]------------
[  125.537981] WARNING: CPU: 0 PID: 2100 at fs/inode.c:275 drop_nlink+0x41/0x50()
[  125.539694] Modules linked in:
[  125.540481] CPU: 0 PID: 2100 Comm: rm Not tainted 4.2.0-rc3+ #67
[  125.541909] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140816_022509-build35 04/01/2014
[  125.544603]  ffffffff81bd943b ffff88007ba67d08 ffffffff818b032f 0000000000000001
[  125.546522]  0000000000000000 ffff88007ba67d48 ffffffff810502b7 ffff88007ba67d0a
[  125.548443]  ffff88007caa7c58 ffff88007b59aad8 ffff88007cc3e780 0000000000000000
[  125.550224] Call Trace:
[  125.550772]  [<ffffffff818b032f>] dump_stack+0x45/0x57
[  125.551929]  [<ffffffff810502b7>] warn_slowpath_common+0x87/0xc0
[  125.553281]  [<ffffffff81050395>] warn_slowpath_null+0x15/0x20
[  125.554588]  [<ffffffff81178be1>] drop_nlink+0x41/0x50
[  125.555744]  [<ffffffff811252d8>] shmem_unlink+0x68/0x80
[  125.556950]  [<ffffffff8116b265>] vfs_unlink+0xd5/0x180
[  125.558126]  [<ffffffff81241d2f>] ecryptfs_do_unlink+0x6f/0x130
[  125.559456]  [<ffffffff81241dfd>] ecryptfs_unlink+0xd/0x10
[  125.560701]  [<ffffffff8116b265>] vfs_unlink+0xd5/0x180
[  125.561814]  [<ffffffff8116ece0>] do_unlinkat+0x240/0x290
[  125.562954]  [<ffffffff8116f5e6>] SyS_unlinkat+0x16/0x30
[  125.564090]  [<ffffffff818bac17>] entry_SYSCALL_64_fastpath+0x12/0x6a
[  125.565447] ---[ end trace 7d3a7839aa1d00d2 ]---
[  125.566432] ------------[ cut here ]------------
[  125.567415] WARNING: CPU: 0 PID: 2100 at fs/dcache.c:309 dentry_free+0x73/0x90()
[  125.568981] Modules linked in:
[  125.569668] CPU: 0 PID: 2100 Comm: rm Tainted: G        W       4.2.0-rc3+ #67
[  125.571194] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140816_022509-build35 04/01/2014
[  125.573538]  ffffffff81bd5e7b ffff88007ba67d38 ffffffff818b032f 0000000000000001
[  125.575111]  0000000000000000 ffff88007ba67d78 ffffffff810502b7 ffff88007cc3e780
[  125.576671]  ffff88007cc3e780 0000000000000000 ffff88007cc3e7d8 0000000000000000
[  125.578239] Call Trace:
[  125.578728]  [<ffffffff818b032f>] dump_stack+0x45/0x57
[  125.579768]  [<ffffffff810502b7>] warn_slowpath_common+0x87/0xc0
[  125.580977]  [<ffffffff81050395>] warn_slowpath_null+0x15/0x20
[  125.582152]  [<ffffffff81174943>] dentry_free+0x73/0x90
[  125.583200]  [<ffffffff81175183>] __dentry_kill+0x153/0x1d0
[  125.584323]  [<ffffffff8117537f>] dput+0x17f/0x1d0
[  125.585274]  [<ffffffff81168e25>] path_put+0x15/0x30
[  125.586230]  [<ffffffff81240f5a>] ecryptfs_d_release+0x1a/0x40
[  125.587335]  [<ffffffff8117515f>] __dentry_kill+0x12f/0x1d0
[  125.588400]  [<ffffffff8117537f>] dput+0x17f/0x1d0
[  125.589320]  [<ffffffff8116ec27>] do_unlinkat+0x187/0x290
[  125.590351]  [<ffffffff8116f5e6>] SyS_unlinkat+0x16/0x30
[  125.591359]  [<ffffffff818bac17>] entry_SYSCALL_64_fastpath+0x12/0x6a
[  125.592580] ---[ end trace 7d3a7839aa1d00d3 ]---
[  125.604089] ------------[ cut here ]------------
[  125.605005] WARNING: CPU: 0 PID: 0 at kernel/rcu/tree.c:2694 rcu_process_callbacks+0x4b7/0x570()
[  125.605075] Modules linked in:
[  125.605075] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W       4.2.0-rc3+ #67
[  125.605075] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140816_022509-build35 04/01/2014
[  125.605075]  ffffffff81bb9a07 ffff88007fc03e48 ffffffff818b032f 0000000000000000
[  125.605075]  0000000000000000 ffff88007fc03e88 ffffffff810502b7 ffff88007fc03e78
[  125.605075]  ffffffff81e3f180 0000000000000246 0000000000000000 0000000000000001
[  125.605075] Call Trace:
[  125.605075]  <IRQ>  [<ffffffff818b032f>] dump_stack+0x45/0x57
[  125.605075]  [<ffffffff810502b7>] warn_slowpath_common+0x87/0xc0
[  125.605075]  [<ffffffff81050395>] warn_slowpath_null+0x15/0x20
[  125.605075]  [<ffffffff810a1487>] rcu_process_callbacks+0x4b7/0x570
[  125.605075]  [<ffffffff81053eb4>] __do_softirq+0xd4/0x250
[  125.605075]  [<ffffffff81054226>] irq_exit+0x86/0x90
[  125.605075]  [<ffffffff810377f5>] smp_apic_timer_interrupt+0x45/0x60
[  125.605075]  [<ffffffff818bb9e8>] apic_timer_interrupt+0x68/0x70
[  125.605075]  <EOI>  [<ffffffff8100cf5b>] ? default_idle+0x1b/0xa0
[  125.605075]  [<ffffffff8100d4ba>] arch_cpu_idle+0xa/0x10
[  125.605075]  [<ffffffff81086f30>] default_idle_call+0x30/0x40
[  125.605075]  [<ffffffff81087199>] cpu_startup_entry+0x1f9/0x2e0
[  125.605075]  [<ffffffff818a9a87>] rest_init+0x77/0x80
[  125.605075]  [<ffffffff81f2dff5>] start_kernel+0x43e/0x44b
[  125.605075]  [<ffffffff81f2d99c>] ? set_init_arg+0x58/0x58
[  125.605075]  [<ffffffff81f2d5ad>] x86_64_start_reservations+0x2a/0x2c
[  125.605075]  [<ffffffff81f2d69b>] x86_64_start_kernel+0xec/0xf0
[  125.605075] ---[ end trace 7d3a7839aa1d00d4 ]---

Reuse the existing super block if the lower path is already mounted
to get rid of that issue.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
Tyler,

I'm not sure whether my fix is the correct solution.
Maybe ecryptfs supports mounting the same lower path mutliple times
and something else is broken.

Thanks,
//richard
---
 fs/ecryptfs/main.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index 4f4d047..bf6ad60 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -481,6 +481,15 @@ out:
 struct kmem_cache *ecryptfs_sb_info_cache;
 static struct file_system_type ecryptfs_fs_type;
 
+static int sb_test(struct super_block *sb, void *data)
+{
+	struct ecryptfs_dentry_info *info = sb->s_root->d_fsdata;
+	struct path *p2 = &info->lower_path;
+	struct path *p1 = data;
+
+	return p1->dentry->d_inode == p2->dentry->d_inode;
+}
+
 /**
  * ecryptfs_get_sb
  * @fs_type
@@ -514,15 +523,29 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 	}
 	mount_crypt_stat = &sbi->mount_crypt_stat;
 
-	s = sget(fs_type, NULL, set_anon_super, flags, NULL);
+	err = "Reading sb failed";
+	rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
+	if (rc) {
+		ecryptfs_printk(KERN_WARNING, "kern_path() failed\n");
+		goto out;
+	}
+
+	s = sget(fs_type, sb_test, set_anon_super, flags, &path);
 	if (IS_ERR(s)) {
 		rc = PTR_ERR(s);
-		goto out;
+		goto out_put;
+	}
+
+	if (s->s_root) {
+		path_put(&path);
+		ecryptfs_destroy_mount_crypt_stat(&sbi->mount_crypt_stat);
+		kmem_cache_free(ecryptfs_sb_info_cache, sbi);
+		return dget(s->s_root);
 	}
 
 	rc = bdi_setup_and_register(&sbi->bdi, "ecryptfs");
 	if (rc)
-		goto out1;
+		goto out_free;
 
 	ecryptfs_set_superblock_private(s, sbi);
 	s->s_bdi = &sbi->bdi;
@@ -532,12 +555,6 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 	s->s_op = &ecryptfs_sops;
 	s->s_d_op = &ecryptfs_dops;
 
-	err = "Reading sb failed";
-	rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
-	if (rc) {
-		ecryptfs_printk(KERN_WARNING, "kern_path() failed\n");
-		goto out1;
-	}
 	if (path.dentry->d_sb->s_type == &ecryptfs_fs_type) {
 		rc = -EINVAL;
 		printk(KERN_ERR "Mount on filesystem of type "
@@ -608,9 +625,9 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 	return dget(s->s_root);
 
 out_free:
-	path_put(&path);
-out1:
 	deactivate_locked_super(s);
+out_put:
+	path_put(&path);
 out:
 	if (sbi) {
 		ecryptfs_destroy_mount_crypt_stat(&sbi->mount_crypt_stat);
-- 
1.8.4.5

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

[Tyler Hicks]

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

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.

> $ 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.

Tyler

> 
> results into:
> [  125.536842] ------------[ cut here ]------------
> [  125.537981] WARNING: CPU: 0 PID: 2100 at fs/inode.c:275 drop_nlink+0x41/0x50()
> [  125.539694] Modules linked in:
> [  125.540481] CPU: 0 PID: 2100 Comm: rm Not tainted 4.2.0-rc3+ #67
> [  125.541909] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140816_022509-build35 04/01/2014
> [  125.544603]  ffffffff81bd943b ffff88007ba67d08 ffffffff818b032f 0000000000000001
> [  125.546522]  0000000000000000 ffff88007ba67d48 ffffffff810502b7 ffff88007ba67d0a
> [  125.548443]  ffff88007caa7c58 ffff88007b59aad8 ffff88007cc3e780 0000000000000000
> [  125.550224] Call Trace:
> [  125.550772]  [<ffffffff818b032f>] dump_stack+0x45/0x57
> [  125.551929]  [<ffffffff810502b7>] warn_slowpath_common+0x87/0xc0
> [  125.553281]  [<ffffffff81050395>] warn_slowpath_null+0x15/0x20
> [  125.554588]  [<ffffffff81178be1>] drop_nlink+0x41/0x50
> [  125.555744]  [<ffffffff811252d8>] shmem_unlink+0x68/0x80
> [  125.556950]  [<ffffffff8116b265>] vfs_unlink+0xd5/0x180
> [  125.558126]  [<ffffffff81241d2f>] ecryptfs_do_unlink+0x6f/0x130
> [  125.559456]  [<ffffffff81241dfd>] ecryptfs_unlink+0xd/0x10
> [  125.560701]  [<ffffffff8116b265>] vfs_unlink+0xd5/0x180
> [  125.561814]  [<ffffffff8116ece0>] do_unlinkat+0x240/0x290
> [  125.562954]  [<ffffffff8116f5e6>] SyS_unlinkat+0x16/0x30
> [  125.564090]  [<ffffffff818bac17>] entry_SYSCALL_64_fastpath+0x12/0x6a
> [  125.565447] ---[ end trace 7d3a7839aa1d00d2 ]---
> [  125.566432] ------------[ cut here ]------------
> [  125.567415] WARNING: CPU: 0 PID: 2100 at fs/dcache.c:309 dentry_free+0x73/0x90()
> [  125.568981] Modules linked in:
> [  125.569668] CPU: 0 PID: 2100 Comm: rm Tainted: G        W       4.2.0-rc3+ #67
> [  125.571194] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140816_022509-build35 04/01/2014
> [  125.573538]  ffffffff81bd5e7b ffff88007ba67d38 ffffffff818b032f 0000000000000001
> [  125.575111]  0000000000000000 ffff88007ba67d78 ffffffff810502b7 ffff88007cc3e780
> [  125.576671]  ffff88007cc3e780 0000000000000000 ffff88007cc3e7d8 0000000000000000
> [  125.578239] Call Trace:
> [  125.578728]  [<ffffffff818b032f>] dump_stack+0x45/0x57
> [  125.579768]  [<ffffffff810502b7>] warn_slowpath_common+0x87/0xc0
> [  125.580977]  [<ffffffff81050395>] warn_slowpath_null+0x15/0x20
> [  125.582152]  [<ffffffff81174943>] dentry_free+0x73/0x90
> [  125.583200]  [<ffffffff81175183>] __dentry_kill+0x153/0x1d0
> [  125.584323]  [<ffffffff8117537f>] dput+0x17f/0x1d0
> [  125.585274]  [<ffffffff81168e25>] path_put+0x15/0x30
> [  125.586230]  [<ffffffff81240f5a>] ecryptfs_d_release+0x1a/0x40
> [  125.587335]  [<ffffffff8117515f>] __dentry_kill+0x12f/0x1d0
> [  125.588400]  [<ffffffff8117537f>] dput+0x17f/0x1d0
> [  125.589320]  [<ffffffff8116ec27>] do_unlinkat+0x187/0x290
> [  125.590351]  [<ffffffff8116f5e6>] SyS_unlinkat+0x16/0x30
> [  125.591359]  [<ffffffff818bac17>] entry_SYSCALL_64_fastpath+0x12/0x6a
> [  125.592580] ---[ end trace 7d3a7839aa1d00d3 ]---
> [  125.604089] ------------[ cut here ]------------
> [  125.605005] WARNING: CPU: 0 PID: 0 at kernel/rcu/tree.c:2694 rcu_process_callbacks+0x4b7/0x570()
> [  125.605075] Modules linked in:
> [  125.605075] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W       4.2.0-rc3+ #67
> [  125.605075] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140816_022509-build35 04/01/2014
> [  125.605075]  ffffffff81bb9a07 ffff88007fc03e48 ffffffff818b032f 0000000000000000
> [  125.605075]  0000000000000000 ffff88007fc03e88 ffffffff810502b7 ffff88007fc03e78
> [  125.605075]  ffffffff81e3f180 0000000000000246 0000000000000000 0000000000000001
> [  125.605075] Call Trace:
> [  125.605075]  <IRQ>  [<ffffffff818b032f>] dump_stack+0x45/0x57
> [  125.605075]  [<ffffffff810502b7>] warn_slowpath_common+0x87/0xc0
> [  125.605075]  [<ffffffff81050395>] warn_slowpath_null+0x15/0x20
> [  125.605075]  [<ffffffff810a1487>] rcu_process_callbacks+0x4b7/0x570
> [  125.605075]  [<ffffffff81053eb4>] __do_softirq+0xd4/0x250
> [  125.605075]  [<ffffffff81054226>] irq_exit+0x86/0x90
> [  125.605075]  [<ffffffff810377f5>] smp_apic_timer_interrupt+0x45/0x60
> [  125.605075]  [<ffffffff818bb9e8>] apic_timer_interrupt+0x68/0x70
> [  125.605075]  <EOI>  [<ffffffff8100cf5b>] ? default_idle+0x1b/0xa0
> [  125.605075]  [<ffffffff8100d4ba>] arch_cpu_idle+0xa/0x10
> [  125.605075]  [<ffffffff81086f30>] default_idle_call+0x30/0x40
> [  125.605075]  [<ffffffff81087199>] cpu_startup_entry+0x1f9/0x2e0
> [  125.605075]  [<ffffffff818a9a87>] rest_init+0x77/0x80
> [  125.605075]  [<ffffffff81f2dff5>] start_kernel+0x43e/0x44b
> [  125.605075]  [<ffffffff81f2d99c>] ? set_init_arg+0x58/0x58
> [  125.605075]  [<ffffffff81f2d5ad>] x86_64_start_reservations+0x2a/0x2c
> [  125.605075]  [<ffffffff81f2d69b>] x86_64_start_kernel+0xec/0xf0
> [  125.605075] ---[ end trace 7d3a7839aa1d00d4 ]---
> 
> Reuse the existing super block if the lower path is already mounted
> to get rid of that issue.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> Tyler,
> 
> I'm not sure whether my fix is the correct solution.
> Maybe ecryptfs supports mounting the same lower path mutliple times
> and something else is broken.
> 
> Thanks,
> //richard
> ---
>  fs/ecryptfs/main.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> index 4f4d047..bf6ad60 100644
> --- a/fs/ecryptfs/main.c
> +++ b/fs/ecryptfs/main.c
> @@ -481,6 +481,15 @@ out:
>  struct kmem_cache *ecryptfs_sb_info_cache;
>  static struct file_system_type ecryptfs_fs_type;
>  
> +static int sb_test(struct super_block *sb, void *data)
> +{
> +	struct ecryptfs_dentry_info *info = sb->s_root->d_fsdata;
> +	struct path *p2 = &info->lower_path;
> +	struct path *p1 = data;
> +
> +	return p1->dentry->d_inode == p2->dentry->d_inode;
> +}
> +
>  /**
>   * ecryptfs_get_sb
>   * @fs_type
> @@ -514,15 +523,29 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  	}
>  	mount_crypt_stat = &sbi->mount_crypt_stat;
>  
> -	s = sget(fs_type, NULL, set_anon_super, flags, NULL);
> +	err = "Reading sb failed";
> +	rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
> +	if (rc) {
> +		ecryptfs_printk(KERN_WARNING, "kern_path() failed\n");
> +		goto out;
> +	}
> +
> +	s = sget(fs_type, sb_test, set_anon_super, flags, &path);
>  	if (IS_ERR(s)) {
>  		rc = PTR_ERR(s);
> -		goto out;
> +		goto out_put;
> +	}
> +
> +	if (s->s_root) {
> +		path_put(&path);
> +		ecryptfs_destroy_mount_crypt_stat(&sbi->mount_crypt_stat);
> +		kmem_cache_free(ecryptfs_sb_info_cache, sbi);
> +		return dget(s->s_root);
>  	}
>  
>  	rc = bdi_setup_and_register(&sbi->bdi, "ecryptfs");
>  	if (rc)
> -		goto out1;
> +		goto out_free;
>  
>  	ecryptfs_set_superblock_private(s, sbi);
>  	s->s_bdi = &sbi->bdi;
> @@ -532,12 +555,6 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  	s->s_op = &ecryptfs_sops;
>  	s->s_d_op = &ecryptfs_dops;
>  
> -	err = "Reading sb failed";
> -	rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
> -	if (rc) {
> -		ecryptfs_printk(KERN_WARNING, "kern_path() failed\n");
> -		goto out1;
> -	}
>  	if (path.dentry->d_sb->s_type == &ecryptfs_fs_type) {
>  		rc = -EINVAL;
>  		printk(KERN_ERR "Mount on filesystem of type "
> @@ -608,9 +625,9 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  	return dget(s->s_root);
>  
>  out_free:
> -	path_put(&path);
> -out1:
>  	deactivate_locked_super(s);
> +out_put:
> +	path_put(&path);
>  out:
>  	if (sbi) {
>  		ecryptfs_destroy_mount_crypt_stat(&sbi->mount_crypt_stat);
> -- 
> 1.8.4.5
> 

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

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