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 >
