Re: [PATCH 22/29] ovl: implement set acl method

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 22 Sept 2022 at 17:18, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> The current way of setting and getting posix acls through the generic
> xattr interface is error prone and type unsafe. The vfs needs to
> interpret and fixup posix acls before storing or reporting it to
> userspace. Various hacks exist to make this work. The code is hard to
> understand and difficult to maintain in it's current form. Instead of
> making this work by hacking posix acls through xattr handlers we are
> building a dedicated posix acl api around the get and set inode
> operations. This removes a lot of hackiness and makes the codepaths
> easier to maintain. A lot of background can be found in [1].
>
> In order to build a type safe posix api around get and set acl we need
> all filesystem to implement get and set acl.
>
> Now that we have added get and set acl inode operations that allow easy
> access to the dentry we give overlayfs it's own get and set acl inode
> operations.
>
> The set acl inode operation is duplicates most of the ovl posix acl
> xattr handler. The main difference being that the set acl inode
> operation relies on the new posix acl api. Once the vfs has been
> switched over the custom posix acl xattr handler will be removed
> completely.
>
> Note, until the vfs has been switched to the new posix acl api this
> patch is a non-functional change.
>
> Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@xxxxxxxxxx [1]
> Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> ---
>  fs/overlayfs/dir.c       |  1 +
>  fs/overlayfs/inode.c     | 81 ++++++++++++++++++++++++++++++++++++++++
>  fs/overlayfs/overlayfs.h | 17 +++++++++
>  3 files changed, 99 insertions(+)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index eb49d5d7b56f..0e817ebce92c 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -1313,6 +1313,7 @@ const struct inode_operations ovl_dir_inode_operations = {
>         .listxattr      = ovl_listxattr,
>         .get_inode_acl  = ovl_get_inode_acl,
>         .get_acl        = ovl_get_acl,
> +       .set_acl        = ovl_set_acl,
>         .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 dd11e13cd288..b0a19f9deaf1 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -596,6 +596,85 @@ struct posix_acl *ovl_get_acl(struct user_namespace *mnt_userns,
>         revert_creds(old_cred);
>         return acl;
>  }
> +
> +int ovl_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
> +               struct posix_acl *acl, int type)
> +{
> +       int err;
> +       struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> +       struct inode *inode = d_inode(dentry);
> +       struct dentry *upperdentry = ovl_dentry_upper(dentry);
> +       struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
> +       struct dentry *workdir = ovl_workdir(dentry);
> +       struct inode *realinode = ovl_inode_real(inode);
> +       struct path realpath;
> +       const struct cred *old_cred;
> +       const char *acl_name;
> +
> +       if (!IS_POSIXACL(d_inode(workdir)))
> +               return -EOPNOTSUPP;
> +       if (!realinode->i_op->set_acl)
> +               return -EOPNOTSUPP;
> +       if (type == ACL_TYPE_DEFAULT && !S_ISDIR(inode->i_mode))
> +               return acl ? -EACCES : 0;
> +       if (!inode_owner_or_capable(&init_user_ns, inode))
> +               return -EPERM;
> +
> +       /*
> +        * Check if sgid bit needs to be cleared (actual setacl operation will
> +        * be done with mounter's capabilities and so that won't do it for us).
> +        */
> +       if (unlikely(inode->i_mode & S_ISGID) && type == ACL_TYPE_ACCESS &&
> +           !in_group_p(inode->i_gid) &&
> +           !capable_wrt_inode_uidgid(&init_user_ns, inode, CAP_FSETID)) {
> +               struct iattr iattr = { .ia_valid = ATTR_KILL_SGID };
> +
> +               err = ovl_setattr(&init_user_ns, dentry, &iattr);
> +               if (err)
> +                       return err;
> +       }
> +

I'd split this function up here (same as was done in the original
xattr based one).

> +       err = ovl_want_write(dentry);
> +       if (err)
> +               goto out;
> +
> +       acl_name = posix_acl_xattr_name(type);

My bad, but this really deserves a comment:  /* If ACL is to be
removed from a lower file, check if it exists in the first place
before copying it up */

> +       if (!acl && !upperdentry) {
> +               struct posix_acl *real_acl;
> +
> +               ovl_path_lower(dentry, &realpath);
> +               old_cred = ovl_override_creds(dentry->d_sb);
> +               real_acl = vfs_get_acl(mnt_user_ns(realpath.mnt), realdentry,
> +                                      posix_acl_xattr_name(type));
> +               revert_creds(old_cred);
> +               posix_acl_release(real_acl);
> +               if (IS_ERR(real_acl))
> +                       goto out_drop_write;
> +       }
> +
> +       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);
> +       if (acl)
> +               err = ovl_do_set_acl(ofs, realdentry, acl_name, acl);
> +       else
> +               err = ovl_do_remove_acl(ofs, realdentry, acl_name);
> +       revert_creds(old_cred);
> +
> +       /* copy c/mtime */
> +       ovl_copyattr(inode);
> +
> +out_drop_write:
> +       ovl_drop_write(dentry);
> +out:
> +       return err;
> +}
>  #endif
>
>  int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags)
> @@ -772,6 +851,7 @@ static const struct inode_operations ovl_file_inode_operations = {
>         .listxattr      = ovl_listxattr,
>         .get_inode_acl  = ovl_get_inode_acl,
>         .get_acl        = ovl_get_acl,
> +       .set_acl        = ovl_set_acl,
>         .update_time    = ovl_update_time,
>         .fiemap         = ovl_fiemap,
>         .fileattr_get   = ovl_fileattr_get,
> @@ -793,6 +873,7 @@ static const struct inode_operations ovl_special_inode_operations = {
>         .listxattr      = ovl_listxattr,
>         .get_inode_acl  = ovl_get_inode_acl,
>         .get_acl        = ovl_get_acl,
> +       .set_acl        = ovl_set_acl,
>         .update_time    = ovl_update_time,
>  };
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 68a3030332e9..b2645baeba2f 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -8,6 +8,8 @@
>  #include <linux/uuid.h>
>  #include <linux/fs.h>
>  #include <linux/namei.h>
> +#include <linux/posix_acl.h>
> +#include <linux/posix_acl_xattr.h>
>  #include "ovl_entry.h"
>
>  #undef pr_fmt
> @@ -278,6 +280,18 @@ static inline int ovl_removexattr(struct ovl_fs *ofs, struct dentry *dentry,
>         return ovl_do_removexattr(ofs, dentry, ovl_xattr(ofs, ox));
>  }
>
> +static inline int ovl_do_set_acl(struct ovl_fs *ofs, struct dentry *dentry,
> +                                const char *acl_name, struct posix_acl *acl)
> +{
> +       return vfs_set_acl(ovl_upper_mnt_userns(ofs), dentry, acl_name, acl);
> +}
> +
> +static inline int ovl_do_remove_acl(struct ovl_fs *ofs, struct dentry *dentry,
> +                                   const char *acl_name)
> +{
> +       return vfs_remove_acl(ovl_upper_mnt_userns(ofs), dentry, acl_name);
> +}
> +
>  static inline int ovl_do_rename(struct ovl_fs *ofs, struct inode *olddir,
>                                 struct dentry *olddentry, struct inode *newdir,
>                                 struct dentry *newdentry, unsigned int flags)
> @@ -595,12 +609,15 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size);
>  struct posix_acl *ovl_get_inode_acl(struct inode *inode, int type, bool rcu);
>  struct posix_acl *ovl_get_acl(struct user_namespace *mnt_userns,
>                               struct dentry *dentry, int type);
> +int ovl_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
> +               struct posix_acl *acl, int type);
>  void ovl_idmap_posix_acl(struct inode *realinode,
>                          struct user_namespace *mnt_userns,
>                          struct posix_acl *acl);
>  #else
>  #define ovl_get_inode_acl      NULL
>  #define ovl_get_acl            NULL
> +#define ovl_set_acl            NULL
>  #endif
>
>  int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags);
> --
> 2.34.1
>



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux