On Thu, Sep 29, 2022 at 10:25:35AM +0200, Christoph Hellwig wrote:
> On Wed, Sep 28, 2022 at 06:08:37PM +0200, Christian Brauner wrote:
> > +static int setxattr_convert(struct user_namespace *mnt_userns, struct dentry *d,
> > + struct xattr_ctx *ctx)
> > {
> > - if (ctx->size && is_posix_acl_xattr(ctx->kname->name))
> > - posix_acl_fix_xattr_from_user(ctx->kvalue, ctx->size);
> > + struct posix_acl *acl;
> > +
> > + if (!ctx->size || !is_posix_acl_xattr(ctx->kname->name))
> > + return 0;
> > +
> > + /*
> > + * Note that posix_acl_from_xattr() uses GFP_NOFS when it probably
> > + * doesn't need to here.
> > + */
> > + acl = posix_acl_from_xattr(current_user_ns(), ctx->kvalue, ctx->size);
> > + if (IS_ERR(acl))
> > + return PTR_ERR(acl);
> > +
> > + ctx->acl = acl;
> > + return 0;
>
> why is this called setxattr_convert when it is clearly about ACLs only?
I think that's from Stefan's (Roesch) series to add xattr support to io_uring.
>
> > +
> > + error = setxattr_convert(mnt_userns, dentry, ctx);
> > + if (error)
> > + return error;
> > +
> > + if (is_posix_acl_xattr(ctx->kname->name))
> > + return vfs_set_acl(mnt_userns, dentry,
> > + ctx->kname->name, ctx->acl);
>
> Also instead of doing two checks for ACLs why not do just one? And then
> have a comment helper to convert and set which can live in posix_acl.c.
>
> No need to store anything in a context with that either.
>
> > @@ -610,6 +642,7 @@ setxattr(struct user_namespace *mnt_userns, struct dentry *d,
> > error = do_setxattr(mnt_userns, d, &ctx);
> >
> > kvfree(ctx.kvalue);
> > + posix_acl_release(ctx.acl);
> > return error;
>
> And I don't think there is any good reason to not release the ACL
> right after the call to vfs_set_acl. Which means there is no need to
> store anything in the ctx.
>
> > + if (is_posix_acl_xattr(ctx->kname->name)) {
> > + ctx->acl = vfs_get_acl(mnt_userns, d, ctx->kname->name);
> > + if (IS_ERR(ctx->acl))
> > + return PTR_ERR(ctx->acl);
> > +
> > + error = vfs_posix_acl_to_xattr(mnt_userns, d_inode(d), ctx->acl,
> > + ctx->kvalue, ctx->size);
> > + posix_acl_release(ctx->acl);
>
> An while this is just a small function body I still think splitting it
> into a helper and moving it to posix_acl.c would be a bit cleaner.
All good points. I'll see how workable this is.
