[PATCH 0/3] ecryptfs: fixes and port to private mounts

[PATCH 0/3] ecryptfs: fixes and port to private mounts

[Christian Brauner]

From: Christian Brauner <christian.brauner@ubuntu.com>

Hey,

Similar to what we do in overlayfs and now in cachefiles too ecryptfs
should rely on a private mount that can't change mount properties
underneath it and puts ecryptfs in full control (apart from changes that
affect the superblock of the underlying fs of course) over the mount it
is using to store its encrypted files in.

Thanks!
Christian

Christian Brauner (3):
  ecryptfs: remove unused helpers
  ecryptfs: use private mount in path
  ecryptfs: extend ro check to private mount

 fs/ecryptfs/ecryptfs_kernel.h | 12 ------------
 fs/ecryptfs/main.c            | 19 ++++++++++++++++++-
 2 files changed, 18 insertions(+), 13 deletions(-)


base-commit: e49d033bddf5b565044e2abe4241353959bc9120
-- 
2.27.0

[PATCH 1/3] ecryptfs: remove unused helpers

[Christian Brauner]

From: Christian Brauner <christian.brauner@ubuntu.com>

Remove two helpers that are unused.

Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Tyler Hicks <code@tyhicks.com>
Cc: ecryptfs@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/ecryptfs/ecryptfs_kernel.h | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index e6ac78c62ca4..463b2d99b554 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -496,12 +496,6 @@ ecryptfs_set_superblock_lower(struct super_block *sb,
 	((struct ecryptfs_sb_info *)sb->s_fs_info)->wsi_sb = lower_sb;
 }
 
-static inline struct ecryptfs_dentry_info *
-ecryptfs_dentry_to_private(struct dentry *dentry)
-{
-	return (struct ecryptfs_dentry_info *)dentry->d_fsdata;
-}
-
 static inline void
 ecryptfs_set_dentry_private(struct dentry *dentry,
 			    struct ecryptfs_dentry_info *dentry_info)
@@ -515,12 +509,6 @@ ecryptfs_dentry_to_lower(struct dentry *dentry)
 	return ((struct ecryptfs_dentry_info *)dentry->d_fsdata)->lower_path.dentry;
 }
 
-static inline struct vfsmount *
-ecryptfs_dentry_to_lower_mnt(struct dentry *dentry)
-{
-	return ((struct ecryptfs_dentry_info *)dentry->d_fsdata)->lower_path.mnt;
-}
-
 static inline struct path *
 ecryptfs_dentry_to_lower_path(struct dentry *dentry)
 {
-- 
2.27.0

[PATCH 2/3] ecryptfs: use private mount in path

[Christian Brauner]

From: Christian Brauner <christian.brauner@ubuntu.com>

Since [1] we support creating private mounts from a given path's
vfsmount. This makes them very suitable for any filesystem or
filesystem functionality that piggybacks on paths of another filesystem.
Overlayfs, cachefiles, and ecryptfs are three prime examples.

Since private mounts aren't attached in the filesystem they aren't
affected by mount property changes after ecryptfs makes use of them.
This seems a rather desirable property as the underlying path can't e.g.
suddenly go from read-write to read-only and in general it means that
ecryptfs is always in full control of the underlying mount after the
user has allowed it to be used (apart from operations that affect the
superblock of course).

Besides that it also makes things simpler for a variety of other vfs
features. One concrete example is fanotify. When the path->mnt of the
path that is used as a cache has been marked with FAN_MARK_MOUNT the
semantics get tricky as it isn't clear whether the watchers of path->mnt
should get notified about fsnotify events when files are created by
cachefilesd via path->mnt. Using a private mount let's us elegantly
handle this case too and aligns the behavior of stacks created by
overlayfs and cachefiles.

Reading through the codebase of ecryptfs it currently takes path->mnt
and then retrieves that path whenever it needs to perform operations in
the underlying filesystem. Simply drop the old path->mnt once we've
created a private mount and place the new private mnt into path->mnt.
This should be all that is needed to make this work since ecryptfs uses
the same lower path's vfsmount to construct the paths it uses to operate
on the underlying filesystem.

[1]: c771d683a62e ("vfs: introduce clone_private_mount()")
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Tyler Hicks <code@tyhicks.com>
Cc: ecryptfs@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/ecryptfs/main.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index cdf40a54a35d..9dcf9a0dd37b 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -476,6 +476,7 @@ static struct file_system_type ecryptfs_fs_type;
 static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags,
 			const char *dev_name, void *raw_data)
 {
+	struct vfsmount *mnt = NULL;
 	struct super_block *s;
 	struct ecryptfs_sb_info *sbi;
 	struct ecryptfs_mount_crypt_stat *mount_crypt_stat;
@@ -537,6 +538,14 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 		goto out_free;
 	}
 
+	mnt = clone_private_mount(&path);
+	if (IS_ERR(mnt)) {
+		rc = PTR_ERR(mnt);
+		mnt = NULL;
+		pr_warn("Failed to create private mount for ecryptfs\n");
+		goto out_free;
+	}
+
 	if (check_ruid && !uid_eq(d_inode(path.dentry)->i_uid, current_uid())) {
 		rc = -EPERM;
 		printk(KERN_ERR "Mount of device (uid: %d) not owned by "
@@ -592,6 +601,13 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 
 	/* ->kill_sb() will take care of root_info */
 	ecryptfs_set_dentry_private(s->s_root, root_info);
+
+	/* We've created a private clone of this mount above so drop it now. */
+	mntput(path.mnt);
+
+	/* Use our private mount from now on. */
+	path.mnt = mnt;
+
 	root_info->lower_path = path;
 
 	s->s_flags |= SB_ACTIVE;
@@ -599,6 +615,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 
 out_free:
 	path_put(&path);
+	mntput(mnt);
 out1:
 	deactivate_locked_super(s);
 out:
-- 
2.27.0

[PATCH 3/3] ecryptfs: extend ro check to private mount

[Christian Brauner]

From: Christian Brauner <christian.brauner@ubuntu.com>

So far ecryptfs only verified that the superblock wasn't read-only but
didn't check whether the mount was. This made sense when we did not use
a private mount because the read-only state could change at any point.

Now that we have a private mount and mount properties can't change
behind our back extend the read-only check to include the vfsmount.

The __mnt_is_readonly() helper will check both the mount and the
superblock.  Note that before we checked root->d_sb and now we check
mnt->mnt_sb but since we have a matching <vfsmount, dentry> pair here
this is only syntactical change, not a semantic one.

Overlayfs and cachefiles have been changed to check this as well.

Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Tyler Hicks <code@tyhicks.com>
Cc: ecryptfs@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/ecryptfs/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index 9dcf9a0dd37b..cdf37d856c62 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -569,7 +569,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 	 *   1) The lower mount is ro
 	 *   2) The ecryptfs_encrypted_view mount option is specified
 	 */
-	if (sb_rdonly(path.dentry->d_sb) || mount_crypt_stat->flags & ECRYPTFS_ENCRYPTED_VIEW_ENABLED)
+	if (__mnt_is_readonly(mnt) || mount_crypt_stat->flags & ECRYPTFS_ENCRYPTED_VIEW_ENABLED)
 		s->s_flags |= SB_RDONLY;
 
 	s->s_maxbytes = path.dentry->d_sb->s_maxbytes;
-- 
2.27.0

Re: [PATCH 2/3] ecryptfs: use private mount in path

[Al Viro]

On Fri, Apr 09, 2021 at 06:24:21PM +0200, Christian Brauner wrote:

> Reading through the codebase of ecryptfs it currently takes path->mnt
> and then retrieves that path whenever it needs to perform operations in
> the underlying filesystem. Simply drop the old path->mnt once we've
> created a private mount and place the new private mnt into path->mnt.
> This should be all that is needed to make this work since ecryptfs uses
> the same lower path's vfsmount to construct the paths it uses to operate
> on the underlying filesystem.

> +	mnt = clone_private_mount(&path);

Incidentally, why is that thing anything other than a trivial wrapper
for mnt_clone_internal() (if that - I'm not convinced that the check of
unbindable is the right thing to do here).  Miklos?

Re: [PATCH 2/3] ecryptfs: use private mount in path

[Christian Brauner]

On Sat, Apr 10, 2021 at 12:31:02AM +0000, Al Viro wrote:
> On Fri, Apr 09, 2021 at 06:24:21PM +0200, Christian Brauner wrote:
> 
> > Reading through the codebase of ecryptfs it currently takes path->mnt
> > and then retrieves that path whenever it needs to perform operations in
> > the underlying filesystem. Simply drop the old path->mnt once we've
> > created a private mount and place the new private mnt into path->mnt.
> > This should be all that is needed to make this work since ecryptfs uses
> > the same lower path's vfsmount to construct the paths it uses to operate
> > on the underlying filesystem.
> 
> > +	mnt = clone_private_mount(&path);
> 
> Incidentally, why is that thing anything other than a trivial wrapper
> for mnt_clone_internal() (if that - I'm not convinced that the check of
> unbindable is the right thing to do here).  Miklos?

The unbindable check is irrelevant at least for both ecryptfs and for
the corresponding cachefiles change I sent out since they don't care
about it.
In practice it doesn't matter to be honest. MS_UNBINDABLE is wildly
esoteric in userspace (We had a glaring bug with that some time ago that
went completely unnoticed for years.). Especially unlikely to be used
for a users home directory (ecryptfs) or /var/cache/fscache
(cachefiles). So even by leaving this check in it's very unlikely for
any regressions to appear.

I hadn't seen mnt_clone_internal() but it's different in so far as it
sets MNT_INTERNAL whereas clone_private_mount() uses MNT_NS_INTERNAL.
Which points me to another potential problem here:
clone_private_mount() seems to want kern_unmount() to be called instead
of just a simple mntput()?

If that's relevant then I think the unbindable check should probably
move out of clone_private_mount() and into overlayfs itself but the rest
be kept.

Re: [PATCH 2/3] ecryptfs: use private mount in path

[Miklos Szeredi]

On Sat, Apr 10, 2021 at 2:30 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Sat, Apr 10, 2021 at 12:31:02AM +0000, Al Viro wrote:
> > On Fri, Apr 09, 2021 at 06:24:21PM +0200, Christian Brauner wrote:
> >
> > > Reading through the codebase of ecryptfs it currently takes path->mnt
> > > and then retrieves that path whenever it needs to perform operations in
> > > the underlying filesystem. Simply drop the old path->mnt once we've
> > > created a private mount and place the new private mnt into path->mnt.
> > > This should be all that is needed to make this work since ecryptfs uses
> > > the same lower path's vfsmount to construct the paths it uses to operate
> > > on the underlying filesystem.
> >
> > > +   mnt = clone_private_mount(&path);
> >
> > Incidentally, why is that thing anything other than a trivial wrapper
> > for mnt_clone_internal() (if that - I'm not convinced that the check of
> > unbindable is the right thing to do here).  Miklos?
>
> The unbindable check is irrelevant at least for both ecryptfs and for
> the corresponding cachefiles change I sent out since they don't care
> about it.
> In practice it doesn't matter to be honest. MS_UNBINDABLE is wildly
> esoteric in userspace (We had a glaring bug with that some time ago that
> went completely unnoticed for years.). Especially unlikely to be used
> for a users home directory (ecryptfs) or /var/cache/fscache
> (cachefiles). So even by leaving this check in it's very unlikely for
> any regressions to appear.
>
> I hadn't seen mnt_clone_internal() but it's different in so far as it
> sets MNT_INTERNAL whereas clone_private_mount() uses MNT_NS_INTERNAL.
> Which points me to another potential problem here:
> clone_private_mount() seems to want kern_unmount() to be called instead
> of just a simple mntput()?

Yes, that's stated in a comment in the clone_private_mount() helper.

The difference is that short term mounts take a small penalty on each
mntput(), while longterm mounts take a fairly large penalty on
kern_unmount().  It's just a performance thing, AFAIK.

As for MS_UNBINDABLE, my recollection is that it was copy-pasted from
regular bind mount.  I agree that it can be moved to overlayfs (or
removed altogether, with some thought into what MS_UNBINDABLE actually
is used for).

Thanks,
Miklos

Re: [PATCH 1/3] ecryptfs: remove unused helpers

[Tyler Hicks]

On 2021-04-09 18:24:20, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> 
> Remove two helpers that are unused.
> 
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Tyler Hicks <code@tyhicks.com>
> Cc: ecryptfs@vger.kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

I'll pick this patch up now as it looks like it didn't make it into your
v2 of the port to private mounts. I'll review those patches separately.

Thanks!

Tyler

> ---
>  fs/ecryptfs/ecryptfs_kernel.h | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> index e6ac78c62ca4..463b2d99b554 100644
> --- a/fs/ecryptfs/ecryptfs_kernel.h
> +++ b/fs/ecryptfs/ecryptfs_kernel.h
> @@ -496,12 +496,6 @@ ecryptfs_set_superblock_lower(struct super_block *sb,
>  	((struct ecryptfs_sb_info *)sb->s_fs_info)->wsi_sb = lower_sb;
>  }
>  
> -static inline struct ecryptfs_dentry_info *
> -ecryptfs_dentry_to_private(struct dentry *dentry)
> -{
> -	return (struct ecryptfs_dentry_info *)dentry->d_fsdata;
> -}
> -
>  static inline void
>  ecryptfs_set_dentry_private(struct dentry *dentry,
>  			    struct ecryptfs_dentry_info *dentry_info)
> @@ -515,12 +509,6 @@ ecryptfs_dentry_to_lower(struct dentry *dentry)
>  	return ((struct ecryptfs_dentry_info *)dentry->d_fsdata)->lower_path.dentry;
>  }
>  
> -static inline struct vfsmount *
> -ecryptfs_dentry_to_lower_mnt(struct dentry *dentry)
> -{
> -	return ((struct ecryptfs_dentry_info *)dentry->d_fsdata)->lower_path.mnt;
> -}
> -
>  static inline struct path *
>  ecryptfs_dentry_to_lower_path(struct dentry *dentry)
>  {
> -- 
> 2.27.0
> 

Re: [PATCH 1/3] ecryptfs: remove unused helpers

[Al Viro]

On Sun, Apr 18, 2021 at 11:48:50PM -0500, Tyler Hicks wrote:
> On 2021-04-09 18:24:20, Christian Brauner wrote:
> > From: Christian Brauner <christian.brauner@ubuntu.com>
> > 
> > Remove two helpers that are unused.
> > 
> > Cc: Amir Goldstein <amir73il@gmail.com>
> > Cc: Tyler Hicks <code@tyhicks.com>
> > Cc: ecryptfs@vger.kernel.org
> > Cc: linux-fsdevel@vger.kernel.org
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> 
> I'll pick this patch up now as it looks like it didn't make it into your
> v2 of the port to private mounts. I'll review those patches separately.

FWIW, there's also a series in vfs.git #work.ecryptfs (posted Mar 20),
and that, AFAICS, duplicates 483bc7e82ccfc in there...

Re: [PATCH 1/3] ecryptfs: remove unused helpers

[Tyler Hicks]

On 2021-04-19 13:49:08, Al Viro wrote:
> On Sun, Apr 18, 2021 at 11:48:50PM -0500, Tyler Hicks wrote:
> > On 2021-04-09 18:24:20, Christian Brauner wrote:
> > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > 
> > > Remove two helpers that are unused.
> > > 
> > > Cc: Amir Goldstein <amir73il@gmail.com>
> > > Cc: Tyler Hicks <code@tyhicks.com>
> > > Cc: ecryptfs@vger.kernel.org
> > > Cc: linux-fsdevel@vger.kernel.org
> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > 
> > I'll pick this patch up now as it looks like it didn't make it into your
> > v2 of the port to private mounts. I'll review those patches separately.
> 
> FWIW, there's also a series in vfs.git #work.ecryptfs (posted Mar 20),
> and that, AFAICS, duplicates 483bc7e82ccfc in there...

Yeah, I noticed that after I pushed Christian's commit to my next
branch. I've fallen behind on eCryptfs patch review. :/

I plan to review vfs.git #work.ecryptfs in the next couple days. If
everything looks good, do you want me to take it via my tree or were you
planning on taking those yourself?

Tyler

Re: [PATCH 1/3] ecryptfs: remove unused helpers

[Christian Brauner]

On Mon, Apr 19, 2021 at 01:49:08PM +0000, Al Viro wrote:
> On Sun, Apr 18, 2021 at 11:48:50PM -0500, Tyler Hicks wrote:
> > On 2021-04-09 18:24:20, Christian Brauner wrote:
> > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > 
> > > Remove two helpers that are unused.
> > > 
> > > Cc: Amir Goldstein <amir73il@gmail.com>
> > > Cc: Tyler Hicks <code@tyhicks.com>
> > > Cc: ecryptfs@vger.kernel.org
> > > Cc: linux-fsdevel@vger.kernel.org
> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > 
> > I'll pick this patch up now as it looks like it didn't make it into your
> > v2 of the port to private mounts. I'll review those patches separately.
> 
> FWIW, there's also a series in vfs.git #work.ecryptfs (posted Mar 20),
> and that, AFAICS, duplicates 483bc7e82ccfc in there...

Yeah, this is why I dropped the patch in the combined series I sent out
last week.

Re: [PATCH 1/3] ecryptfs: remove unused helpers

[Al Viro]

On Mon, Apr 19, 2021 at 09:22:58AM -0500, Tyler Hicks wrote:
> On 2021-04-19 13:49:08, Al Viro wrote:
> > On Sun, Apr 18, 2021 at 11:48:50PM -0500, Tyler Hicks wrote:
> > > On 2021-04-09 18:24:20, Christian Brauner wrote:
> > > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > > 
> > > > Remove two helpers that are unused.
> > > > 
> > > > Cc: Amir Goldstein <amir73il@gmail.com>
> > > > Cc: Tyler Hicks <code@tyhicks.com>
> > > > Cc: ecryptfs@vger.kernel.org
> > > > Cc: linux-fsdevel@vger.kernel.org
> > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > 
> > > I'll pick this patch up now as it looks like it didn't make it into your
> > > v2 of the port to private mounts. I'll review those patches separately.
> > 
> > FWIW, there's also a series in vfs.git #work.ecryptfs (posted Mar 20),
> > and that, AFAICS, duplicates 483bc7e82ccfc in there...
> 
> Yeah, I noticed that after I pushed Christian's commit to my next
> branch. I've fallen behind on eCryptfs patch review. :/
> 
> I plan to review vfs.git #work.ecryptfs in the next couple days. If
> everything looks good, do you want me to take it via my tree or were you
> planning on taking those yourself?

Entirely up to you.  The only patch in there that might have some interplay
with VFS work is lock_parent() changes (#2/4) and the stuff it might be
a prereq for is not going to get ready until the next cycle - you are not
the only one with clogged queue ;-/

So even if you prefer to cherry-pick those one by one, I've no problem with
that; just tell me when to drop that branch and I'll do so.