On Wed, Nov 29, 2023 at 11:50 PM Seth Forshee (DigitalOcean)
<sforshee@xxxxxxxxxx> wrote:
>
> Add handlers which read fs caps from the lower or upper filesystem and
> write/remove fs caps to the upper filesystem, performing copy-up as
> necessary.
>
> While it doesn't make sense to use fscaps on directories, nothing in the
> kernel actually prevents setting or getting fscaps xattrs for directory
> inodes. If we omit fscaps handlers in ovl_dir_inode_operations then the
> generic handlers will be used. These handlers will use the xattr inode
> operations, bypassing any idmapping on lower mounts, so fscaps handlers
> are also installed for ovl_dir_inode_operations.
>
> Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@xxxxxxxxxx>
> ---
> fs/overlayfs/dir.c | 3 ++
> fs/overlayfs/inode.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
> fs/overlayfs/overlayfs.h | 6 ++++
> 3 files changed, 93 insertions(+)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index aab3f5d93556..d9ab3c9ce10a 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -1303,6 +1303,9 @@ const struct inode_operations ovl_dir_inode_operations = {
> .get_inode_acl = ovl_get_inode_acl,
> .get_acl = ovl_get_acl,
> .set_acl = ovl_set_acl,
> + .get_fscaps = ovl_get_fscaps,
> + .set_fscaps = ovl_set_fscaps,
> + .remove_fscaps = ovl_remove_fscaps,
> .update_time = ovl_update_time,
> .fileattr_get = ovl_fileattr_get,
> .fileattr_set = ovl_fileattr_set,
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index c63b31a460be..82fc6e479d45 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -568,6 +568,87 @@ int ovl_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
> }
> #endif
>
> +int ovl_get_fscaps(struct mnt_idmap *idmap, struct dentry *dentry,
> + struct vfs_caps *caps)
> +{
> + int err;
> + const struct cred *old_cred;
> + struct path realpath;
> +
> + ovl_path_real(dentry, &realpath);
> + old_cred = ovl_override_creds(dentry->d_sb);
> + err = vfs_get_fscaps(mnt_idmap(realpath.mnt), realpath.dentry, caps);
> + revert_creds(old_cred);
> + return err;
> +}
> +
> +int ovl_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry,
> + const struct vfs_caps *caps, int flags)
> +{
> + int err;
> + struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> + struct dentry *upperdentry = ovl_dentry_upper(dentry);
> + struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
> + const struct cred *old_cred;
> +
> + err = ovl_want_write(dentry);
> + if (err)
> + goto out;
> +
> + if (!upperdentry) {
> + err = ovl_copy_up(dentry);
> + if (err)
> + goto out_drop_write;
> +
> + realdentry = ovl_dentry_upper(dentry);
> + }
> +
> + old_cred = ovl_override_creds(dentry->d_sb);
> + err = vfs_set_fscaps(ovl_upper_mnt_idmap(ofs), realdentry, caps, flags);
> + revert_creds(old_cred);
> +
> + /* copy c/mtime */
> + ovl_copyattr(d_inode(dentry));
> +
> +out_drop_write:
> + ovl_drop_write(dentry);
> +out:
> + return err;
> +}
> +
> +int ovl_remove_fscaps(struct mnt_idmap *idmap, struct dentry *dentry)
> +{
> + int err;
> + struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> + struct dentry *upperdentry = ovl_dentry_upper(dentry);
> + struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
> + const struct cred *old_cred;
> +
> + err = ovl_want_write(dentry);
> + if (err)
> + goto out;
> +
> + if (!upperdentry) {
> + err = ovl_copy_up(dentry);
> + if (err)
> + goto out_drop_write;
> +
> + realdentry = ovl_dentry_upper(dentry);
> + }
> +
This construct is peculiar.
Most of the operations just do this unconditionally:
err = ovl_copy_up(dentry);
if (err)
goto out_drop_write;
and then use ovl_dentry_upper(dentry) directly, because a modification
will always be done on the upper dentry, regardless of the state before
the operation started.
I was wondering where you copied this from and I found it right above
in ovl_set_or_remove_acl().
In that case, there was also no justification for this construct.
There is also no justification for open coding:
struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
when later on, ovl_path_lower(dentry, &realpath) is called anyway.
The only reason to do anything special in ovl_set_or_remove_acl() is:
/*
* If ACL is to be removed from a lower file, check if it exists in
* the first place before copying it up.
*/
Do you not want to do the same for ovl_remove_fscaps()?
Also, the comparison to remove_acl API bares the question,
why did you need to add a separate method for remove_fscaps?
Why not use set_fscaps(NULL), just like setxattr() and set_acl() APIs?
Thanks,
Amir.
