Re: [PATCH v3 17/19] ovl: handle idmappings in layer open helpers

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Mar 31, 2022 at 01:23:15PM +0200, Christian Brauner wrote:
> In earlier patches we already passed down the relevant upper or lower
> path to ovl_open_realfile(). Now let the open helpers actually take the
> idmapping of the relevant mount into account when checking permissions.
> This is needed to support idmapped base layers with overlay.
> 
> Cc: <linux-unionfs@vger.kernel.org>
> Tested-by: Giuseppe Scrivano <gscrivan@redhat.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
> /* v2 */
> unchanged
> 
> /* v3 */
> unchanged
> ---
>  fs/overlayfs/file.c | 7 +++++--
>  fs/overlayfs/util.c | 5 +++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 656c30bf20a6..7dd44f4e2757 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -42,6 +42,7 @@ static struct file *ovl_open_realfile(const struct file *file,
>  {
>  	struct inode *realinode = d_inode(realpath->dentry);
>  	struct inode *inode = file_inode(file);
> +	struct user_namespace *real_idmap;
>  	struct file *realfile;
>  	const struct cred *old_cred;
>  	int flags = file->f_flags | OVL_OPEN_FLAGS;
> @@ -51,12 +52,14 @@ static struct file *ovl_open_realfile(const struct file *file,
>  	if (flags & O_APPEND)
>  		acc_mode |= MAY_APPEND;
>  
> +
>  	old_cred = ovl_override_creds(inode->i_sb);
> -	err = inode_permission(&init_user_ns, realinode, MAY_OPEN | acc_mode);
> +	real_idmap = mnt_user_ns(realpath->mnt);
> +	err = inode_permission(real_idmap, realinode, MAY_OPEN | acc_mode);

Just a minor nit. Should we rename "real_idmap" to say "real_mnt_userns".
Becuase they are not idmaps. They are just pionters to user namespace
associated with that mount. And in all the vfs helpers you have used
the name "mnt_userns" anyway.

I see that downside is that name of the variable becomes longer. But
I feel "real_mnt_userns" bring more clarity while reading code.

Thanks
Vivek


>  	if (err) {
>  		realfile = ERR_PTR(err);
>  	} else {
> -		if (!inode_owner_or_capable(&init_user_ns, realinode))
> +		if (!inode_owner_or_capable(real_idmap, realinode))
>  			flags &= ~O_NOATIME;
>  
>  		realfile = open_with_fake_path(&file->f_path, flags, realinode,
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 79fae06ee10a..7dd2e5e6662a 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -523,6 +523,7 @@ bool ovl_is_whiteout(struct dentry *dentry)
>  struct file *ovl_path_open(struct path *path, int flags)
>  {
>  	struct inode *inode = d_inode(path->dentry);
> +	struct user_namespace *real_idmap = mnt_user_ns(path->mnt);
>  	int err, acc_mode;
>  
>  	if (flags & ~(O_ACCMODE | O_LARGEFILE))
> @@ -539,12 +540,12 @@ struct file *ovl_path_open(struct path *path, int flags)
>  		BUG();
>  	}
>  
> -	err = inode_permission(&init_user_ns, inode, acc_mode | MAY_OPEN);
> +	err = inode_permission(real_idmap, inode, acc_mode | MAY_OPEN);
>  	if (err)
>  		return ERR_PTR(err);
>  
>  	/* O_NOATIME is an optimization, don't fail if not permitted */
> -	if (inode_owner_or_capable(&init_user_ns, inode))
> +	if (inode_owner_or_capable(real_idmap, inode))
>  		flags |= O_NOATIME;
>  
>  	return dentry_open(path, flags, current_cred());
> -- 
> 2.32.0
>