On Fri, Sep 23, 2022 at 05:38:34PM +0200, Miklos Szeredi wrote:
> On Thu, 22 Sept 2022 at 17:18, Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >
> > Now that posix acls have a proper api us it to copy them.
> >
> > All filesystems that can serve as lower or upper layers for overlayfs
> > have gained support for the new posix acl api in previous patches.
> > So switch all internal overlayfs codepaths for copying posix acls to the
> > new posix acl api.
> >
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> > ---
> > fs/overlayfs/copy_up.c | 9 +++++++++
> > fs/overlayfs/dir.c | 20 ++-----------------
> > fs/overlayfs/inode.c | 4 ++--
> > fs/overlayfs/overlayfs.h | 9 +++++++++
> > fs/overlayfs/super.c | 6 ++----
> > fs/overlayfs/util.c | 42 ++++++++++++++++++++++++++++++++++++++++
> > fs/xattr.c | 6 ------
> > include/linux/xattr.h | 6 ++++++
> > 8 files changed, 72 insertions(+), 30 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index fdde6c56cc3d..93e575021ca1 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -93,6 +93,15 @@ int ovl_copy_xattr(struct super_block *sb, struct path *oldpath, struct dentry *
> > error = 0;
> > continue; /* Discard */
> > }
> > +
> > + if (is_posix_acl_xattr(name)) {
> > + error = ovl_copy_acl(OVL_FS(sb), oldpath, new, name);
> > + if (!error)
> > + continue;
> > + /* POSIX ACLs must be copied. */
> > + break;
> > + }
> > +
> > retry:
> > size = ovl_do_getxattr(oldpath, name, value, value_size);
> > if (size == -ERANGE)
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 0e817ebce92c..cbb569d5d234 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -435,28 +435,12 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
> > }
> >
> > static int ovl_set_upper_acl(struct ovl_fs *ofs, struct dentry *upperdentry,
> > - const char *name, const struct posix_acl *acl)
> > + const char *acl_name, struct posix_acl *acl)
> > {
> > - void *buffer;
> > - size_t size;
> > - int err;
> > -
> > if (!IS_ENABLED(CONFIG_FS_POSIX_ACL) || !acl)
> > return 0;
> >
> > - size = posix_acl_xattr_size(acl->a_count);
> > - buffer = kmalloc(size, GFP_KERNEL);
> > - if (!buffer)
> > - return -ENOMEM;
> > -
> > - err = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> > - if (err < 0)
> > - goto out_free;
> > -
> > - err = ovl_do_setxattr(ofs, upperdentry, name, buffer, size, XATTR_CREATE);
> > -out_free:
> > - kfree(buffer);
> > - return err;
> > + return ovl_do_set_acl(ofs, upperdentry, acl_name, acl);
> > }
> >
> > static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index b0a19f9deaf1..c6cb62daa8c2 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -550,8 +550,8 @@ struct posix_acl *ovl_get_inode_acl(struct inode *inode, int type, bool rcu)
> > return clone;
> > }
> >
> > -static struct posix_acl *ovl_get_acl_path(const struct path *path,
> > - const char *acl_name)
> > +struct posix_acl *ovl_get_acl_path(const struct path *path,
> > + const char *acl_name)
> > {
> > struct posix_acl *real_acl, *clone;
> > struct user_namespace *mnt_userns;
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index b2645baeba2f..3528e5631cb2 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -436,6 +436,8 @@ static inline bool ovl_check_origin_xattr(struct ovl_fs *ofs,
> > int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry,
> > enum ovl_xattr ox, const void *value, size_t size,
> > int xerr);
> > +int ovl_copy_acl(struct ovl_fs *ofs, const struct path *path,
> > + struct dentry *dentry, const char *acl_name);
> > int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry);
> > bool ovl_inuse_trylock(struct dentry *dentry);
> > void ovl_inuse_unlock(struct dentry *dentry);
> > @@ -614,10 +616,17 @@ int ovl_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
> > void ovl_idmap_posix_acl(struct inode *realinode,
> > struct user_namespace *mnt_userns,
> > struct posix_acl *acl);
> > +struct posix_acl *ovl_get_acl_path(const struct path *path,
> > + const char *acl_name);
> > #else
> > #define ovl_get_inode_acl NULL
> > #define ovl_get_acl NULL
> > #define ovl_set_acl NULL
> > +static inline struct posix_acl *ovl_get_acl_path(const struct path *path,
> > + const char *acl_name)
> > +{
> > + return NULL;
> > +}
> > #endif
> >
> > int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags);
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 5da771b218d1..8a13319db1d3 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -812,13 +812,11 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
> > * allowed as upper are limited to "normal" ones, where checking
> > * for the above two errors is sufficient.
> > */
> > - err = ovl_do_removexattr(ofs, work,
> > - XATTR_NAME_POSIX_ACL_DEFAULT);
> > + err = ovl_do_remove_acl(ofs, work, XATTR_NAME_POSIX_ACL_DEFAULT);
> > if (err && err != -ENODATA && err != -EOPNOTSUPP)
> > goto out_dput;
> >
> > - err = ovl_do_removexattr(ofs, work,
> > - XATTR_NAME_POSIX_ACL_ACCESS);
> > + err = ovl_do_remove_acl(ofs, work, XATTR_NAME_POSIX_ACL_ACCESS);
> > if (err && err != -ENODATA && err != -EOPNOTSUPP)
> > goto out_dput;
> >
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 87f811c089e4..0246babce4d2 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -1117,3 +1117,45 @@ void ovl_copyattr(struct inode *inode)
> > inode->i_ctime = realinode->i_ctime;
> > i_size_write(inode, i_size_read(realinode));
> > }
> > +
> > +int ovl_copy_acl(struct ovl_fs *ofs, const struct path *path,
> > + struct dentry *dentry, const char *acl_name)
>
> The only caller is in copy_up.c, why move to util.c?
Ok, I'll leave this in util.c
>
>
> > +{
> > + int err;
> > + struct posix_acl *real_acl = NULL;
> > +
> > + real_acl = ovl_get_acl_path(path, acl_name);
> > + if (!real_acl)
> > + return 0;
>
> This looks subtle. The acl is converted back and forth between
> various mnt_userns representations, and I don't quite follow why this
> should result in the same thing as if the raw xattr was copied.
> Will it? Can this be made less subtle?
It's basically just like when you copy a file betweed idmapped mounts:
mount -o X-mount.idmap=<mapping1> --bind /source /source-idmapped
mount -o X-mount.idmap=<mapping2> --bind /target /target-idmapped
cp /source-idmapped/file1 /target-idmapped
where you need to take the source and target idmappings into account.
So basically like what ovl_set_attr() is doing just for acls. But I
think your proposal below will make this way less subtle.
>
> > +
> > + if (IS_ERR(real_acl)) {
> > + err = PTR_ERR(real_acl);
> > + if (err == -ENODATA || err == -EOPNOTSUPP)
> > + return 0;
> > + return err;
> > + }
> > +
> > + /*
> > + * If we didn't have to create a copy already because @path was on an
> > + * idmapped mount we need to do so if the upper layer is so we don't
> > + * alter the POSIX ACLs of the filesystem we retrieved them from.
> > + */
>
> I think we are better off copying ovl_get_acl_path() and cloning
> unconditionally.
Ok, sure.
