On Thu, Dec 30, 2021 at 11:12:42AM +0100, Christian Brauner wrote:
> @@ -545,6 +545,7 @@ EXPORT_SYMBOL_GPL(vfs_removexattr);
> int setxattr_copy(const char __user *name, struct xattr_ctx *ctx)
> {
> int error;
> + struct xattr_ctx *new_ctx;
>
> if (ctx->flags & ~(XATTR_CREATE|XATTR_REPLACE))
> return -EINVAL;
> @@ -606,12 +607,9 @@ setxattr(struct user_namespace *mnt_userns, struct dentry *d,
> int error;
>
> error = setxattr_copy(name, &ctx);
> - if (error)
> - return error;
> -
> - error = do_setxattr(mnt_userns, d, &ctx);
> -
> - kvfree(ctx.kvalue);
> + if (!error)
> + error = do_setxattr(mnt_userns, d, &ctx);
> + setxattr_finish(&ctx);
> return error;
> }
Huh? Have you lost a chunk or two in there? The only modification of
setxattr_copy() in your delta is the introduction of an unused local
variable. Confused...
What I had in mind is something like this:
// same for getxattr and setxattr
static int xattr_name_from_user(const char __user *name, struct xattr_ctx *ctx)
{
int copied;
if (!ctx->xattr_name) {
ctx->xattr_name = kmalloc(XATTR_NAME_MAX + 1, GFP_KERNEL);
if (!ctx->xattr_name)
return -ENOMEM;
}
copied = strncpy_from_user(ctx->xattr_name, name, XATTR_NAME_MAX + 1);
if (copied < 0)
return copied; // copyin failure; almost always -EFAULT
if (copied == 0 || copied == XATTR_NAME_MAX + 1)
return -ERANGE;
return 0;
}
// freeing is up to the caller, whether we succeed or not
int setxattr_copy(const char __user *name, struct xattr_ctx *ctx)
{
int error;
if (ctx->flags & ~(XATTR_CREATE|XATTR_REPLACE))
return -EINVAL;
error = xattr_name_from_user(name, ctx);
if (error)
return error;
if (ctx->size) {
void *p;
if (ctx->size > XATTR_SIZE_MAX)
return -E2BIG;
p = vmemdup_user(ctx->value, ctx->size);
if (IS_ERR(p))
return PTR_ERR(p);
ctx->kvalue = p;
}
return 0;
}
with syscall side concluded with freeing ->kvalue (unconditionally), while
io_uring one - ->kvalue and ->xattr_name (also unconditionally). And to
hell with struct xattr_name - a string is a string.
However, what I really want to see is the answer to my question re control
flow and the place where we do copy the arguments from userland. Including
the pathname.
*IF* there's a subtle reason that has to be done from prep phase (and there
might very well be - figuring out the control flow in io_uring is bloody
painful), I would really like to see it spelled out, along with the explanation
of the reasons why statx() doesn't need anything of that sort.
If there's no such reasons, I would bloody well leave marshalling to the
payload, allowing to share a lot more with the syscall path. In that
case xattr_ctx only needs to carry the userland pointers/size/flags.
And all that "do we allocate the kernel copy of the name dynamically,
or does it live on stack" simply goes away.
Details, please.
