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

[Christian Brauner <brauner@kernel.org>]

On Fri, Sep 23, 2022 at 05:18:42PM +0200, Miklos Szeredi wrote:
> On Thu, 22 Sept 2022 at 17:18, Christian Brauner <brauner@kernel.org> 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@kernel.org [1]
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> > ---
> >  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.