Re: [PATCH 6/7] ecryptfs: switch to using a private mount

[Tyler Hicks <code@tyhicks.com>]

On 2021-04-14 14:37:50, Christian Brauner wrote:
> 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
> ecryptfs 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.
> 
> This change comes with a proper simplification in how ecryptfs currently
> handles the lower_path it stashes as private information in its
> dentries. Currently it always does:
> 
>         ecryptfs_set_dentry_private(dentry, dentry_info);
>         dentry_info->lower_path.mnt = mntget(path->mnt);
>         dentry_info->lower_path.dentry = lower_dentry;
> 
> and then during .d_relase() in ecryptfs_d_release():
> 
>         path_put(&p->lower_path);
> 
> which is odd since afaict path->mnt is guaranteed to be the mnt stashed
> during ecryptfs_mount():
> 
>         ecryptfs_set_dentry_private(s->s_root, root_info);
>         root_info->lower_path = path;
> 
> So that mntget() seems somewhat pointless but there might be reasons
> that I'm missing in how the interpose logic for ecryptfs works.
> 
> While switching to a long-term private mount via clone_private_mount()
> let's get rid of the gratuitous mntget() and mntput()/path_put().
> Instead, stash away the private mount in ecryptfs' s_fs_info and call
> kern_unmount() in .kill_sb() so we only take the mntput() hit once.
> 
> I've added a WARN_ON_ONCE() into ecryptfs_lookup_interpose() triggering
> if the stashed private mount and the path's mount don't match. I think
> that would be a proper bug even without that clone_private_mount()
> change in this patch.
> 
> [1]: c771d683a62e ("vfs: introduce clone_private_mount()")
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Tyler Hicks <code@tyhicks.com>
> Cc: Miklos Szeredi <mszeredi@redhat.com>
> Cc: ecryptfs@vger.kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

This patch and the following one both look technically correct to me. I
do want to spend a little time manually testing these changes, though.
I'm hoping to have that done by end of day Tuesday.

Tyler

> ---
>  fs/ecryptfs/dentry.c          |  6 +++++-
>  fs/ecryptfs/ecryptfs_kernel.h |  9 +++++++++
>  fs/ecryptfs/inode.c           |  5 ++++-
>  fs/ecryptfs/main.c            | 29 ++++++++++++++++++++++++-----
>  4 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ecryptfs/dentry.c b/fs/ecryptfs/dentry.c
> index 44606f079efb..e5edafa165d4 100644
> --- a/fs/ecryptfs/dentry.c
> +++ b/fs/ecryptfs/dentry.c
> @@ -67,7 +67,11 @@ static void ecryptfs_d_release(struct dentry *dentry)
>  {
>  	struct ecryptfs_dentry_info *p = dentry->d_fsdata;
>  	if (p) {
> -		path_put(&p->lower_path);
> +		/*
> +		 * p->lower_path.mnt is a private mount which will be released
> +		 * when the superblock shuts down so we only need to dput here.
> +		 */
> +		dput(p->lower_path.dentry);
>  		call_rcu(&p->rcu, ecryptfs_dentry_free_rcu);
>  	}
>  }
> diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> index e6ac78c62ca4..f89d0f7bb3fe 100644
> --- a/fs/ecryptfs/ecryptfs_kernel.h
> +++ b/fs/ecryptfs/ecryptfs_kernel.h
> @@ -352,6 +352,7 @@ struct ecryptfs_mount_crypt_stat {
>  struct ecryptfs_sb_info {
>  	struct super_block *wsi_sb;
>  	struct ecryptfs_mount_crypt_stat mount_crypt_stat;
> +	struct vfsmount *mnt;
>  };
>  
>  /* file private data. */
> @@ -496,6 +497,14 @@ ecryptfs_set_superblock_lower(struct super_block *sb,
>  	((struct ecryptfs_sb_info *)sb->s_fs_info)->wsi_sb = lower_sb;
>  }
>  
> +static inline void
> +ecryptfs_set_superblock_lower_mnt(struct super_block *sb,
> +				  struct vfsmount *mnt)
> +{
> +	struct ecryptfs_sb_info *sbi = sb->s_fs_info;
> +	sbi->mnt = mnt;
> +}
> +
>  static inline struct ecryptfs_dentry_info *
>  ecryptfs_dentry_to_private(struct dentry *dentry)
>  {
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 18e9285fbb4c..204df4bf476d 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -324,6 +324,7 @@ static struct dentry *ecryptfs_lookup_interpose(struct dentry *dentry,
>  				     struct dentry *lower_dentry)
>  {
>  	struct path *path = ecryptfs_dentry_to_lower_path(dentry->d_parent);
> +	struct ecryptfs_sb_info *sb_info = ecryptfs_superblock_to_private(dentry->d_sb);
>  	struct inode *inode, *lower_inode;
>  	struct ecryptfs_dentry_info *dentry_info;
>  	int rc = 0;
> @@ -339,7 +340,9 @@ static struct dentry *ecryptfs_lookup_interpose(struct dentry *dentry,
>  	BUG_ON(!d_count(lower_dentry));
>  
>  	ecryptfs_set_dentry_private(dentry, dentry_info);
> -	dentry_info->lower_path.mnt = mntget(path->mnt);
> +	/* Warn if we somehow ended up with an unexpected path. */
> +	WARN_ON_ONCE(path->mnt != sb_info->mnt);
> +	dentry_info->lower_path.mnt = path->mnt;
>  	dentry_info->lower_path.dentry = lower_dentry;
>  
>  	/*
> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> index cdf40a54a35d..3ba2c0f349a3 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;
>  	struct super_block *s;
>  	struct ecryptfs_sb_info *sbi;
>  	struct ecryptfs_mount_crypt_stat *mount_crypt_stat;
> @@ -537,6 +538,16 @@ 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);
> +		pr_warn("Failed to create private mount for ecryptfs\n");
> +		goto out_free;
> +	}
> +
> +	/* Record our long-term lower mount. */
> +	ecryptfs_set_superblock_lower_mnt(s, mnt);
> +
>  	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 "
> @@ -590,9 +601,15 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  	if (!root_info)
>  		goto out_free;
>  
> +	/* Use our private mount from now on. */
> +	root_info->lower_path.mnt = mnt;
> +	root_info->lower_path.dentry = dget(path.dentry);
> +
> +	/* We created a private clone of this mount above so drop the path. */
> +	path_put(&path);
> +
>  	/* ->kill_sb() will take care of root_info */
>  	ecryptfs_set_dentry_private(s->s_root, root_info);
> -	root_info->lower_path = path;
>  
>  	s->s_flags |= SB_ACTIVE;
>  	return dget(s->s_root);
> @@ -619,11 +636,13 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  static void ecryptfs_kill_block_super(struct super_block *sb)
>  {
>  	struct ecryptfs_sb_info *sb_info = ecryptfs_superblock_to_private(sb);
> +
>  	kill_anon_super(sb);
> -	if (!sb_info)
> -		return;
> -	ecryptfs_destroy_mount_crypt_stat(&sb_info->mount_crypt_stat);
> -	kmem_cache_free(ecryptfs_sb_info_cache, sb_info);
> +	if (sb_info) {
> +		kern_unmount(sb_info->mnt);
> +		ecryptfs_destroy_mount_crypt_stat(&sb_info->mount_crypt_stat);
> +		kmem_cache_free(ecryptfs_sb_info_cache, sb_info);
> +	}
>  }
>  
>  static struct file_system_type ecryptfs_fs_type = {
> -- 
> 2.27.0
>