Re: [PATCH 5/9] replace do_setxattr() with saner helpers.

[Al Viro <viro@xxxxxxxxxxxxxxxxxx>]

On Tue, Oct 01, 2024 at 07:34:12PM -0600, Jens Axboe wrote:

> > -retry:
> > -	ret = filename_lookup(AT_FDCWD, ix->filename, lookup_flags, &path, NULL);
> > -	if (!ret) {
> > -		ret = __io_setxattr(req, issue_flags, &path);
> > -		path_put(&path);
> > -		if (retry_estale(ret, lookup_flags)) {
> > -			lookup_flags |= LOOKUP_REVAL;
> > -			goto retry;
> > -		}
> > -	}
> > -
> > +	ret = filename_setxattr(AT_FDCWD, ix->filename, LOOKUP_FOLLOW, &ix->ctx);
> >  	io_xattr_finish(req, ret);
> >  	return IOU_OK;
> 
> this looks like it needs an ix->filename = NULL, as
> filename_{s,g}xattr() drops the reference. The previous internal helper
> did not, and hence the cleanup always did it. But should work fine if
> ->filename is just zeroed.
> 
> Otherwise looks good. I've skimmed the other patches and didn't see
> anything odd, I'll take a closer look tomorrow.

Hmm...  I wonder if we would be better off with file{,name}_setxattr()
doing kvfree(cxt->kvalue) - it makes things easier both on the syscall
and on io_uring side.

I've added minimal fixes (zeroing ix->filename after filename_[sg]etxattr())
to 5/9 and 6/9 *and* added a followup calling conventions change at the end
of the branch.  See #work.xattr2 in the same tree; FWIW, the followup
cleanup is below; note that putname(ERR_PTR(-Ewhatever)) is an explicit
no-op, so there's no need to zero on getname() failures.

commit 67896be9ac99b3fdef82708dd06e720332c13cdc
Author: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Date:   Tue Oct 1 22:03:16 2024 -0400

    saner calling conventions for file{,name}_setxattr()
    
    Have them consume ctx->kvalue.  That simplifies both the path_setxattrat()
    and io_uring side of things - there io_xattr_finish() is just left to
    free the xattr name and that's it.
    
    Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>

diff --git a/fs/xattr.c b/fs/xattr.c
index 59cdb524412e..6bb656941bce 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -672,6 +672,7 @@ int file_setxattr(struct file *f, struct kernel_xattr_ctx *ctx)
 		error = do_setxattr(file_mnt_idmap(f), f->f_path.dentry, ctx);
 		mnt_drop_write_file(f);
 	}
+	kvfree(ctx->kvalue);
 	return error;
 }
 
@@ -697,6 +698,7 @@ int filename_setxattr(int dfd, struct filename *filename,
 	}
 
 out:
+	kvfree(ctx->kvalue);
 	putname(filename);
 	return error;
 }
@@ -731,14 +733,10 @@ static int path_setxattrat(int dfd, const char __user *pathname,
 	if (!filename) {
 		CLASS(fd, f)(dfd);
 		if (fd_empty(f))
-			error = -EBADF;
-		else
-			error = file_setxattr(fd_file(f), &ctx);
-	} else {
-		error = filename_setxattr(dfd, filename, lookup_flags, &ctx);
+			return -EBADF;
+		return file_setxattr(fd_file(f), &ctx);
 	}
-	kvfree(ctx.kvalue);
-	return error;
+	return filename_setxattr(dfd, filename, lookup_flags, &ctx);
 }
 
 SYSCALL_DEFINE6(setxattrat, int, dfd, const char __user *, pathname, unsigned int, at_flags,
diff --git a/io_uring/xattr.c b/io_uring/xattr.c
index 90277246dbea..9f3ea12f628d 100644
--- a/io_uring/xattr.c
+++ b/io_uring/xattr.c
@@ -35,9 +35,10 @@ void io_xattr_cleanup(struct io_kiocb *req)
 
 static void io_xattr_finish(struct io_kiocb *req, int ret)
 {
-	req->flags &= ~REQ_F_NEED_CLEANUP;
+	struct io_xattr *ix = io_kiocb_to_cmd(req, struct io_xattr);
 
-	io_xattr_cleanup(req);
+	kfree(ix->ctx.kname);
+	req->flags &= ~REQ_F_NEED_CLEANUP;
 	io_req_set_res(req, ret, 0);
 }
 
@@ -94,12 +95,9 @@ int io_getxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	path = u64_to_user_ptr(READ_ONCE(sqe->addr3));
 
 	ix->filename = getname(path);
-	if (IS_ERR(ix->filename)) {
-		ret = PTR_ERR(ix->filename);
-		ix->filename = NULL;
-	}
-
-	return ret;
+	if (IS_ERR(ix->filename))
+		return PTR_ERR(ix->filename);
+	return 0;
 }
 
 int io_fgetxattr(struct io_kiocb *req, unsigned int issue_flags)
@@ -122,7 +120,6 @@ int io_getxattr(struct io_kiocb *req, unsigned int issue_flags)
 	WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK);
 
 	ret = filename_getxattr(AT_FDCWD, ix->filename, LOOKUP_FOLLOW, &ix->ctx);
-	ix->filename = NULL;
 	io_xattr_finish(req, ret);
 	return IOU_OK;
 }
@@ -172,12 +169,9 @@ int io_setxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	path = u64_to_user_ptr(READ_ONCE(sqe->addr3));
 
 	ix->filename = getname(path);
-	if (IS_ERR(ix->filename)) {
-		ret = PTR_ERR(ix->filename);
-		ix->filename = NULL;
-	}
-
-	return ret;
+	if (IS_ERR(ix->filename))
+		return PTR_ERR(ix->filename);
+	return 0;
 }
 
 int io_fsetxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
@@ -205,7 +199,6 @@ int io_setxattr(struct io_kiocb *req, unsigned int issue_flags)
 	WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK);
 
 	ret = filename_setxattr(AT_FDCWD, ix->filename, LOOKUP_FOLLOW, &ix->ctx);
-	ix->filename = NULL;
 	io_xattr_finish(req, ret);
 	return IOU_OK;
 }