On Fri, Sep 23, 2022 at 05:18:42PM +0200, Miklos Szeredi wrote: > 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). Ok, will do. > > > + 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 */ Ok, will add.
