On Thu, Dec 30, 2021 at 03:04:34AM +0000, Al Viro wrote:

On Thu, Dec 30, 2021 at 02:17:23AM +0000, Al Viro wrote:

quoted

On Wed, Dec 29, 2021 at 12:30:01PM -0800, Stefan Roesch wrote:

quoted

+static int __io_setxattr_prep(struct io_kiocb *req,
+			const struct io_uring_sqe *sqe)
+{
+	struct io_xattr *ix = &req->xattr;
+	const char __user *name;
+	int ret;
+
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
+	if (unlikely(sqe->ioprio))
+		return -EINVAL;
+	if (unlikely(req->flags & REQ_F_FIXED_FILE))
+		return -EBADF;
+
+	ix->filename = NULL;
+	name = u64_to_user_ptr(READ_ONCE(sqe->addr));
+	ix->ctx.value = u64_to_user_ptr(READ_ONCE(sqe->addr2));
+	ix->ctx.kvalue = NULL;
+	ix->ctx.size = READ_ONCE(sqe->len);
+	ix->ctx.flags = READ_ONCE(sqe->xattr_flags);
+
+	ix->ctx.kname = kmalloc(sizeof(*ix->ctx.kname), GFP_KERNEL);
+	if (!ix->ctx.kname)
+		return -ENOMEM;
+
+	ret = setxattr_copy(name, &ix->ctx);
+	if (ret) {
+		kfree(ix->ctx.kname);
+		return ret;
+	}
+
+	req->flags |= REQ_F_NEED_CLEANUP;
+	return 0;
+}

OK, so you
	* allocate a buffer for xattr name
	* have setxattr_copy() copy the name in *and* memdup the contents
	* on failure, you have the buffer for xattr name freed and return
an error.  memdup'ed stuff is left for cleanup, presumably.

quoted

+static int io_setxattr_prep(struct io_kiocb *req,
+			const struct io_uring_sqe *sqe)
+{
+	struct io_xattr *ix = &req->xattr;
+	const char __user *path;
+	int ret;
+
+	ret = __io_setxattr_prep(req, sqe);
+	if (ret)
+		return ret;
+
+	path = u64_to_user_ptr(READ_ONCE(sqe->addr3));
+
+	ix->filename = getname_flags(path, LOOKUP_FOLLOW, NULL);
+	if (IS_ERR(ix->filename)) {
+		ret = PTR_ERR(ix->filename);
+		ix->filename = NULL;
+	}
+
+	return ret;
+}

... and here you use it and bring the pathname in.  Should the latter
step fail, you restore ->filename to NULL and return an error.

Could you explain what kind of magic could allow the caller to tell
whether ix->ctx.kname needs to be freed on error?  I don't see any way
that could possibly work...

FWIW, your calling conventions make no sense whatsoever.  OK, you have
a helper that does copyin of the arguments.  And it needs to be shared
between the syscall path (where you put the xattr name on stack) and
io_uring one (where you allocate it dynamically).  Why not simply move
the allocation into that helper, conditional upon the passed value being
NULL?  And leave it alone on any failure paths in that helper.

I had thought about that too when looking at Stefan's code at first but
then concluded that doesn't make sense since io_uring doesn't allocate
xattr_ctx dynamically either. It embedds it directly in io_xattrs which
itself isn't allocated dynamically either.

But I think the unconditional cleanup you proposed makes sense. If we
add a simple static inline helper to internal.h to cleanup xattr_ctx
once the caller is done we can use that in __io_setxattr() and
setxattr(): 

From 248cae031c21d3103c8ab46afd729aa46114019a Mon Sep 17 00:00:00 2001
From: Christian Brauner <redacted>
Date: Thu, 30 Dec 2021 11:02:57 +0100
Subject: [PATCH] UNTESTED

---
 fs/internal.h |  7 +++++++
 fs/io_uring.c | 11 +----------
 fs/xattr.c    | 10 ++++------
 3 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/fs/internal.h b/fs/internal.h
index 942b2005a2be..446dca46d845 100644
--- a/fs/internal.h
+++ b/fs/internal.h

@@ -228,5 +228,12 @@ ssize_t do_getxattr(struct user_namespace *mnt_userns,
 		    size_t size);
 
 int setxattr_copy(const char __user *name, struct xattr_ctx *ctx);
+static inline void setxattr_finish(struct xattr_ctx *ctx)
+{
+	kfree(ctx->kname);
+	kvfree(ctx->kvalue);
+	memset(ctx, 0, sizeof(struct xattr_ctx));
+}
+
 int do_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		struct xattr_ctx *ctx);

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 7204b8d593e4..2e30c7a87eb9 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c

@@ -4114,7 +4114,7 @@ static int __io_setxattr(struct io_kiocb *req, unsigned int issue_flags,
 		ret = do_setxattr(mnt_user_ns(path->mnt), path->dentry, &ix->ctx);
 		mnt_drop_write(path->mnt);
 	}
-
+	setxattr_finish(&ix->ctx);
 	return ret;
 }
 

@@ -4127,12 +4127,7 @@ static int io_fsetxattr(struct io_kiocb *req, unsigned int issue_flags)
 		return -EAGAIN;
 
 	ret = __io_setxattr(req, issue_flags, &req->file->f_path);
-
 	req->flags &= ~REQ_F_NEED_CLEANUP;
-	kfree(ix->ctx.kname);
-
-	if (ix->ctx.kvalue)
-		kvfree(ix->ctx.kvalue);
 	if (ret < 0)
 		req_set_fail(req);
 

@@ -4163,10 +4158,6 @@ static int io_setxattr(struct io_kiocb *req, unsigned int issue_flags)
 	putname(ix->filename);
 
 	req->flags &= ~REQ_F_NEED_CLEANUP;
-	kfree(ix->ctx.kname);
-
-	if (ix->ctx.kvalue)
-		kvfree(ix->ctx.kvalue);
 	if (ret < 0)
 		req_set_fail(req);
 

diff --git a/fs/xattr.c b/fs/xattr.c
index 3b6d683d07b9..0f4e067816bc 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c

@@ -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;
 }
 

-- 
2.30.2


> 
> Syscall user would set it pointing to local structure/string/whatnot.
> No freeing is needed there in any case.
> 
> io_uring one would set it to NULL and free the value left there on
> cleanup.  Again, same in all cases, error or no error.  Just make sure
> you have it zeroed *before* any failure exits (including those on req->flags,
> etc.)
> 
> While we are at it, syscall path needs to free the copied xattr contents
> anyway.  So screw freeing anything in that helper (both allocation failures
> and copyin ones); have all freeing done by caller, and make it unconditional
> there.  An error is usually a slow path; an error of that sort - definitely
> so.  IOW,
> 	1) call the helper, copying userland data into the buffers allocated
> by the helper
> 	2) if helper hasn't returned an error, do work
> 	3) free whatever the helper has allocated
> With (3) being unconditional.  It doesn't make any sense to have a separate
> early exit, especially since with your approach you end up paying the price
> on failure exits in the helper anyway.
> 
> 	error = setxattr_copy(...);
> 	if (likely(!error))
> 		error = do_setxattr(...);
> 	kvfree(...);
> 	return error;
> 
> would've been better for the syscall side as well.