On Wed, Sep 25, 2024 at 12:01:01AM -0600, Jens Axboe wrote: > The normal policy is that anything that is read-only should remain > stable after ->prep() has been called, so that ->issue() can use it. > That means the application can keep it on-stack as long as it's valid > until io_uring_submit() returns. For structs/buffers that are copied to > after IO, those the application obviously need to keep around until they > see a completion for that request. So yes, for the xattr cases where the > struct is copied to at completion time, those do not need to be stable > after ->prep(), could be handled purely on the ->issue() side. Looks like io_fsetxattr() was missing audit_file()... Anyway, in a local branch I've added two helpers - int file_setxattr(struct file *file, struct xattr_ctx *ctx); int filename_setxattr(int dfd, struct filename *filename, struct xattr_ctx *ctx, unsigned int lookup_flags); and converted both fs/xattr.c and io_uring/xattr.c to those. Completely untested delta follows; it's _not_ in the final form, it misses getxattr side, etc. BTW, I think fs/internal.h is a very wrong place for that, as well as for do_mkdirat() et.al. include/linux/marshalled_syscalls.h, perhaps? diff --git a/fs/internal.h b/fs/internal.h index 8cf42b327e5e..e39f80201ff8 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -285,8 +285,9 @@ ssize_t do_getxattr(struct mnt_idmap *idmap, struct xattr_ctx *ctx); int setxattr_copy(const char __user *name, struct xattr_ctx *ctx); -int do_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, - struct xattr_ctx *ctx); +int file_setxattr(struct file *file, struct xattr_ctx *ctx); +int filename_setxattr(int dfd, struct filename *filename, + struct xattr_ctx *ctx, unsigned int lookup_flags); int may_write_xattr(struct mnt_idmap *idmap, struct inode *inode); #ifdef CONFIG_FS_POSIX_ACL diff --git a/fs/xattr.c b/fs/xattr.c index 0fc813cb005c..fc6409181c46 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -619,7 +619,7 @@ int setxattr_copy(const char __user *name, struct xattr_ctx *ctx) return error; } -int do_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, +static int do_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, struct xattr_ctx *ctx) { if (is_posix_acl_xattr(ctx->kname->name)) @@ -630,32 +630,31 @@ int do_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, ctx->kvalue, ctx->size, ctx->flags); } -static int path_setxattr(const char __user *pathname, - const char __user *name, const void __user *value, - size_t size, int flags, unsigned int lookup_flags) +int file_setxattr(struct file *f, struct xattr_ctx *ctx) +{ + int error = mnt_want_write_file(f); + + if (!error) { + audit_file(f); + error = do_setxattr(file_mnt_idmap(f), f->f_path.dentry, ctx); + mnt_drop_write_file(f); + } + return error; +} + +int filename_setxattr(int dfd, struct filename *filename, + struct xattr_ctx *ctx, unsigned int lookup_flags) { - struct xattr_name kname; - struct xattr_ctx ctx = { - .cvalue = value, - .kvalue = NULL, - .size = size, - .kname = &kname, - .flags = flags, - }; struct path path; int error; - error = setxattr_copy(name, &ctx); - if (error) - return error; - retry: - error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path); + error = filename_lookup(dfd, filename, lookup_flags, &path, NULL); if (error) goto out; error = mnt_want_write(path.mnt); if (!error) { - error = do_setxattr(mnt_idmap(path.mnt), path.dentry, &ctx); + error = do_setxattr(mnt_idmap(path.mnt), path.dentry, ctx); mnt_drop_write(path.mnt); } path_put(&path); @@ -665,6 +664,30 @@ static int path_setxattr(const char __user *pathname, } out: + putname(filename); + return error; +} + +static int path_setxattr(const char __user *pathname, + const char __user *name, const void __user *value, + size_t size, int flags, unsigned int lookup_flags) +{ + struct xattr_name kname; + struct xattr_ctx ctx = { + .cvalue = value, + .kvalue = NULL, + .size = size, + .kname = &kname, + .flags = flags, + }; + int error; + + error = setxattr_copy(name, &ctx); + if (error) + return error; + + error = filename_setxattr(AT_FDCWD, getname(pathname), + &ctx, lookup_flags); kvfree(ctx.kvalue); return error; } @@ -700,17 +723,11 @@ SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name, if (fd_empty(f)) return -EBADF; - audit_file(fd_file(f)); error = setxattr_copy(name, &ctx); if (error) return error; - error = mnt_want_write_file(fd_file(f)); - if (!error) { - error = do_setxattr(file_mnt_idmap(fd_file(f)), - fd_file(f)->f_path.dentry, &ctx); - mnt_drop_write_file(fd_file(f)); - } + error = file_setxattr(fd_file(f), &ctx); kvfree(ctx.kvalue); return error; } diff --git a/io_uring/xattr.c b/io_uring/xattr.c index 13e8d7d2cdc2..702d5981fd63 100644 --- a/io_uring/xattr.c +++ b/io_uring/xattr.c @@ -203,28 +203,14 @@ int io_fsetxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) return __io_setxattr_prep(req, sqe); } -static int __io_setxattr(struct io_kiocb *req, unsigned int issue_flags, - const struct path *path) -{ - struct io_xattr *ix = io_kiocb_to_cmd(req, struct io_xattr); - int ret; - - ret = mnt_want_write(path->mnt); - if (!ret) { - ret = do_setxattr(mnt_idmap(path->mnt), path->dentry, &ix->ctx); - mnt_drop_write(path->mnt); - } - - return ret; -} - int io_fsetxattr(struct io_kiocb *req, unsigned int issue_flags) { + struct io_xattr *ix = io_kiocb_to_cmd(req, struct io_xattr); int ret; WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK); - ret = __io_setxattr(req, issue_flags, &req->file->f_path); + ret = file_setxattr(req->file, &ix->ctx); io_xattr_finish(req, ret); return IOU_OK; } @@ -232,22 +218,11 @@ int io_fsetxattr(struct io_kiocb *req, unsigned int issue_flags) int io_setxattr(struct io_kiocb *req, unsigned int issue_flags) { struct io_xattr *ix = io_kiocb_to_cmd(req, struct io_xattr); - unsigned int lookup_flags = LOOKUP_FOLLOW; - struct path path; int ret; WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK); -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, &ix->ctx, LOOKUP_FOLLOW); io_xattr_finish(req, ret); return IOU_OK;